Iterate on a proposed performance improvement for counters

Original discussion see
https://github.com/prometheus/client_golang/pull/362 .

Assuming that the most frequently used method of a `Gauge` is `Set`
and the most frequently used method of a `Conuter` is `Inc`, this
separates the implementation of both metric types. `Inc` and integral
`Add` of a counter is now handled in a separate `uint64`. This would
create a race in `Set`, but luckily, there is no `Set` anymore in a
counter.

All attempts to solve above race (to use the same idea for a `Gauge`)
slow down `Set`, So we just stick with the old implementation
(formerly `value`) for `Gauge`.

prometheus/counter.go

@@ -15,6 +15,10 @@ package prometheus
 
 import (
 	"errors"
+	"math"
+	"sync/atomic"
+
+	dto "github.com/prometheus/client_model/go"
 )
 
 // Counter is a Metric that represents a single numerical value that only ever
@@ -42,6 +46,14 @@ type Counter interface {
 type CounterOpts Opts
 
 // NewCounter creates a new Counter based on the provided CounterOpts.
+//
+// The returned implementation tracks the counter value in two separate
+// variables, a float64 and a uint64. The latter is used to track calls of the
+// Inc method and calls of the Add method with a value that can be represented
+// as a uint64. This allows atomic increments of the counter with optimal
+// performance. (It is common to have an Inc call in very hot execution paths.)
+// Both internal tracking values are added up in the Write method. This has to
+// be taken into account when it comes to precision and overflow behavior.
 func NewCounter(opts CounterOpts) Counter {
 	desc := NewDesc(
 		BuildFQName(opts.Namespace, opts.Subsystem, opts.Name),
@@ -49,20 +61,58 @@ func NewCounter(opts CounterOpts) Counter {
 		nil,
 		opts.ConstLabels,
 	)
-	result := &counter{value: value{desc: desc, valType: CounterValue, labelPairs: desc.constLabelPairs}}
+	result := &counter{desc: desc, labelPairs: desc.constLabelPairs}
 	result.init(result) // Init self-collection.
 	return result
 }
 
 type counter struct {
-	value
+	// valBits contains the bits of the represented float64 value, while
+	// valInt stores values that are exact integers. Both have to go first
+	// in the struct to guarantee alignment for atomic operations.
+	// http://golang.org/pkg/sync/atomic/#pkg-note-BUG
+	valBits uint64
+	valInt  uint64
+
+	selfCollector
+	desc *Desc
+
+	labelPairs []*dto.LabelPair
+}
+
+func (c *counter) Desc() *Desc {
+	return c.desc
 }
 
 func (c *counter) Add(v float64) {
 	if v < 0 {
 		panic(errors.New("counter cannot decrease in value"))
 	}
-	c.value.Add(v)
+	ival := uint64(v)
+	if float64(ival) == v {
+		atomic.AddUint64(&c.valInt, ival)
+		return
+	}
+
+	for {
+		oldBits := atomic.LoadUint64(&c.valBits)
+		newBits := math.Float64bits(math.Float64frombits(oldBits) + v)
+		if atomic.CompareAndSwapUint64(&c.valBits, oldBits, newBits) {
+			return
+		}
+	}
+}
+
+func (c *counter) Inc() {
+	atomic.AddUint64(&c.valInt, 1)
+}
+
+func (c *counter) Write(out *dto.Metric) error {
+	fval := math.Float64frombits(atomic.LoadUint64(&c.valBits))
+	ival := atomic.LoadUint64(&c.valInt)
+	val := fval + float64(ival)
+
+	return populateMetric(CounterValue, val, c.labelPairs, out)
 }
 
 // CounterVec is a Collector that bundles a set of Counters that all share the
@@ -85,11 +135,10 @@ func NewCounterVec(opts CounterOpts, labelNames []string) *CounterVec {
 	)
 	return &CounterVec{
 		metricVec: newMetricVec(desc, func(lvs ...string) Metric {
-			result := &counter{value: value{
+			if len(lvs) != len(desc.variableLabels) {
-				desc:       desc,
+				panic(errInconsistentCardinality)
-				valType:    CounterValue,
+			}
-				labelPairs: makeLabelPairs(desc, lvs),
+			result := &counter{desc: desc, labelPairs: makeLabelPairs(desc, lvs)}
-			}}
 			result.init(result) // Init self-collection.
 			return result
 		}),

prometheus/counter_test.go

@@ -31,23 +31,23 @@ func TestCounterAdd(t *testing.T) {
 	if expected, got := 0.0, math.Float64frombits(counter.valBits); expected != got {
 		t.Errorf("Expected %f, got %f.", expected, got)
 	}
-	if expected, got := int64(1), counter.valInt; expected != got {
+	if expected, got := uint64(1), counter.valInt; expected != got {
-		t.Errorf("Expected %f, got %f.", expected, got)
+		t.Errorf("Expected %d, got %d.", expected, got)
 	}
 	counter.Add(42)
 	if expected, got := 0.0, math.Float64frombits(counter.valBits); expected != got {
 		t.Errorf("Expected %f, got %f.", expected, got)
 	}
-	if expected, got := int64(43), counter.valInt; expected != got {
+	if expected, got := uint64(43), counter.valInt; expected != got {
-		t.Errorf("Expected %f, got %f.", expected, got)
+		t.Errorf("Expected %d, got %d.", expected, got)
 	}
 
 	counter.Add(24.42)
 	if expected, got := 24.42, math.Float64frombits(counter.valBits); expected != got {
 		t.Errorf("Expected %f, got %f.", expected, got)
 	}
-	if expected, got := int64(43), counter.valInt; expected != got {
+	if expected, got := uint64(43), counter.valInt; expected != got {
-		t.Errorf("Expected %f, got %f.", expected, got)
+		t.Errorf("Expected %d, got %d.", expected, got)
 	}
 
 	if expected, got := "counter cannot decrease in value", decreaseCounter(counter).Error(); expected != got {
@@ -137,15 +137,15 @@ func TestCounterAddInf(t *testing.T) {
 	if expected, got := 0.0, math.Float64frombits(counter.valBits); expected != got {
 		t.Errorf("Expected %f, got %f.", expected, got)
 	}
-	if expected, got := int64(1), counter.valInt; expected != got {
+	if expected, got := uint64(1), counter.valInt; expected != got {
-		t.Errorf("Expected %f, got %f.", expected, got)
+		t.Errorf("Expected %d, got %d.", expected, got)
 	}
 
 	counter.Add(math.Inf(1))
 	if expected, got := math.Inf(1), math.Float64frombits(counter.valBits); expected != got {
 		t.Errorf("valBits expected %f, got %f.", expected, got)
 	}
-	if expected, got := int64(1), counter.valInt; expected != got {
+	if expected, got := uint64(1), counter.valInt; expected != got {
 		t.Errorf("valInts expected %d, got %d.", expected, got)
 	}
 
@@ -153,7 +153,7 @@ func TestCounterAddInf(t *testing.T) {
 	if expected, got := math.Inf(1), math.Float64frombits(counter.valBits); expected != got {
 		t.Errorf("Expected %f, got %f.", expected, got)
 	}
-	if expected, got := int64(2), counter.valInt; expected != got {
+	if expected, got := uint64(2), counter.valInt; expected != got {
 		t.Errorf("Expected %d, got %d.", expected, got)
 	}
 
@@ -171,20 +171,20 @@ func TestCounterAddLarge(t *testing.T) {
 		Help: "test help",
 	}).(*counter)
 
-	// large overflows the underlying type and should therefore be stored in valBits
+	// large overflows the underlying type and should therefore be stored in valBits.
-	large := float64(math.MaxInt64 + 1)
+	large := float64(math.MaxUint64 + 1)
 	counter.Add(large)
 	if expected, got := large, math.Float64frombits(counter.valBits); expected != got {
 		t.Errorf("valBits expected %f, got %f.", expected, got)
 	}
-	if expected, got := int64(0), counter.valInt; expected != got {
+	if expected, got := uint64(0), counter.valInt; expected != got {
 		t.Errorf("valInts expected %d, got %d.", expected, got)
 	}
 
 	m := &dto.Metric{}
 	counter.Write(m)
 
-	if expected, got := fmt.Sprintf("counter:<value:%0.15e > ", large), m.String(); expected != got {
+	if expected, got := fmt.Sprintf("counter:<value:%0.16e > ", large), m.String(); expected != got {
 		t.Errorf("expected %q, got %q", expected, got)
 	}
 }
@@ -199,7 +199,7 @@ func TestCounterAddSmall(t *testing.T) {
 	if expected, got := small, math.Float64frombits(counter.valBits); expected != got {
 		t.Errorf("valBits expected %f, got %f.", expected, got)
 	}
-	if expected, got := int64(0), counter.valInt; expected != got {
+	if expected, got := uint64(0), counter.valInt; expected != got {
 		t.Errorf("valInts expected %d, got %d.", expected, got)
 	}
 

prometheus/gauge.go

@@ -13,6 +13,14 @@
 
 package prometheus
 
+import (
+	"math"
+	"sync/atomic"
+	"time"
+
+	dto "github.com/prometheus/client_model/go"
+)
+
 // Gauge is a Metric that represents a single numerical value that can
 // arbitrarily go up and down.
 //
@@ -48,13 +56,74 @@ type Gauge interface {
 type GaugeOpts Opts
 
 // NewGauge creates a new Gauge based on the provided GaugeOpts.
+//
+// The returned implementation is optimized for a fast Set method. If you have a
+// choice for managing the value of a Gauge via Set vs. Inc/Dec/Add/Sub, pick
+// the former. For example, the Inc method of the returned Gauge is slower than
+// the Inc method of a Counter returned by NewCounter. This matches the typical
+// scenarios for Gauges and Counters, where the former tends to be Set-heavy and
+// the latter Inc-heavy.
 func NewGauge(opts GaugeOpts) Gauge {
-	return newValue(NewDesc(
+	desc := NewDesc(
 		BuildFQName(opts.Namespace, opts.Subsystem, opts.Name),
 		opts.Help,
 		nil,
 		opts.ConstLabels,
-	), GaugeValue, 0)
+	)
+	result := &gauge{desc: desc, labelPairs: desc.constLabelPairs}
+	result.init(result) // Init self-collection.
+	return result
+}
+
+type gauge struct {
+	// valBits contains the bits of the represented float64 value. It has
+	// to go first in the struct to guarantee alignment for atomic
+	// operations.  http://golang.org/pkg/sync/atomic/#pkg-note-BUG
+	valBits uint64
+
+	selfCollector
+
+	desc       *Desc
+	labelPairs []*dto.LabelPair
+}
+
+func (g *gauge) Desc() *Desc {
+	return g.desc
+}
+
+func (g *gauge) Set(val float64) {
+	atomic.StoreUint64(&g.valBits, math.Float64bits(val))
+}
+
+func (g *gauge) SetToCurrentTime() {
+	g.Set(float64(time.Now().UnixNano()) / 1e9)
+}
+
+func (g *gauge) Inc() {
+	g.Add(1)
+}
+
+func (g *gauge) Dec() {
+	g.Add(-1)
+}
+
+func (g *gauge) Add(val float64) {
+	for {
+		oldBits := atomic.LoadUint64(&g.valBits)
+		newBits := math.Float64bits(math.Float64frombits(oldBits) + val)
+		if atomic.CompareAndSwapUint64(&g.valBits, oldBits, newBits) {
+			return
+		}
+	}
+}
+
+func (g *gauge) Sub(val float64) {
+	g.Add(val * -1)
+}
+
+func (g *gauge) Write(out *dto.Metric) error {
+	val := math.Float64frombits(atomic.LoadUint64(&g.valBits))
+	return populateMetric(GaugeValue, val, g.labelPairs, out)
 }
 
 // GaugeVec is a Collector that bundles a set of Gauges that all share the same
@@ -77,7 +146,12 @@ func NewGaugeVec(opts GaugeOpts, labelNames []string) *GaugeVec {
 	)
 	return &GaugeVec{
 		metricVec: newMetricVec(desc, func(lvs ...string) Metric {
-			return newValue(desc, GaugeValue, 0, lvs...)
+			if len(lvs) != len(desc.variableLabels) {
+				panic(errInconsistentCardinality)
+			}
+			result := &gauge{desc: desc, labelPairs: makeLabelPairs(desc, lvs)}
+			result.init(result) // Init self-collection.
+			return result
 		}),
 	}
 }

prometheus/gauge_test.go

@@ -83,7 +83,7 @@ func TestGaugeConcurrency(t *testing.T) {
 		}
 		start.Done()
 
-		if expected, got := <-result, math.Float64frombits(gge.(*value).valBits); math.Abs(expected-got) > 0.000001 {
+		if expected, got := <-result, math.Float64frombits(gge.(*gauge).valBits); math.Abs(expected-got) > 0.000001 {
 			t.Fatalf("expected approx. %f, got %f", expected, got)
 			return false
 		}
@@ -147,7 +147,7 @@ func TestGaugeVecConcurrency(t *testing.T) {
 		start.Done()
 
 		for i := range sStreams {
-			if expected, got := <-results[i], math.Float64frombits(gge.WithLabelValues(string('A'+i)).(*value).valBits); math.Abs(expected-got) > 0.000001 {
+			if expected, got := <-results[i], math.Float64frombits(gge.WithLabelValues(string('A'+i)).(*gauge).valBits); math.Abs(expected-got) > 0.000001 {
 				t.Fatalf("expected approx. %f, got %f", expected, got)
 				return false
 			}

prometheus/go_collector_test.go

@@ -89,37 +89,33 @@ func TestGCCollector(t *testing.T) {
 	for {
 		select {
 		case metric := <-ch:
-			switch m := metric.(type) {
+			pb := &dto.Metric{}
-			case *constSummary, *value:
+			metric.Write(pb)
-				pb := &dto.Metric{}
+			if pb.GetSummary() == nil {
-				m.Write(pb)
+				continue
-				if pb.GetSummary() == nil {
-					continue
-				}
-
-				if len(pb.GetSummary().Quantile) != 5 {
-					t.Errorf("expected 4 buckets, got %d", len(pb.GetSummary().Quantile))
-				}
-				for idx, want := range []float64{0.0, 0.25, 0.5, 0.75, 1.0} {
-					if *pb.GetSummary().Quantile[idx].Quantile != want {
-						t.Errorf("bucket #%d is off, got %f, want %f", idx, *pb.GetSummary().Quantile[idx].Quantile, want)
-					}
-				}
-				if first {
-					first = false
-					oldGC = *pb.GetSummary().SampleCount
-					oldPause = *pb.GetSummary().SampleSum
-					close(waitc)
-					continue
-				}
-				if diff := *pb.GetSummary().SampleCount - oldGC; diff != 1 {
-					t.Errorf("want 1 new garbage collection run, got %d", diff)
-				}
-				if diff := *pb.GetSummary().SampleSum - oldPause; diff <= 0 {
-					t.Errorf("want moar pause, got %f", diff)
-				}
-				return
 			}
+			if len(pb.GetSummary().Quantile) != 5 {
+				t.Errorf("expected 4 buckets, got %d", len(pb.GetSummary().Quantile))
+			}
+			for idx, want := range []float64{0.0, 0.25, 0.5, 0.75, 1.0} {
+				if *pb.GetSummary().Quantile[idx].Quantile != want {
+					t.Errorf("bucket #%d is off, got %f, want %f", idx, *pb.GetSummary().Quantile[idx].Quantile, want)
+				}
+			}
+			if first {
+				first = false
+				oldGC = *pb.GetSummary().SampleCount
+				oldPause = *pb.GetSummary().SampleSum
+				close(waitc)
+				continue
+			}
+			if diff := *pb.GetSummary().SampleCount - oldGC; diff != 1 {
+				t.Errorf("want 1 new garbage collection run, got %d", diff)
+			}
+			if diff := *pb.GetSummary().SampleSum - oldPause; diff <= 0 {
+				t.Errorf("want moar pause, got %f", diff)
+			}
+			return
 		case <-time.After(1 * time.Second):
 			t.Fatalf("expected collect timed out")
 		}

prometheus/value.go

@@ -15,10 +15,7 @@ package prometheus
 
 import (
 	"fmt"
-	"math"
 	"sort"
-	"sync/atomic"
-	"time"
 
 	dto "github.com/prometheus/client_model/go"
 
@@ -36,95 +33,6 @@ const (
 	UntypedValue
 )
 
-// value is a generic metric for simple values. It implements Metric, Collector,
-// Counter, Gauge, and Untyped. Its effective type is determined by
-// ValueType. This is a low-level building block used by the library to back the
-// implementations of Counter, Gauge, and Untyped.
-type value struct {
-	// valBits contains the bits of the represented float64 value. It has
-	// to go first in the struct to guarantee alignment for atomic
-	// operations.  http://golang.org/pkg/sync/atomic/#pkg-note-BUG
-	valBits uint64
-	// valInt is used to store values that are exact integers
-	valInt int64
-
-	selfCollector
-
-	desc       *Desc
-	valType    ValueType
-	labelPairs []*dto.LabelPair
-}
-
-// newValue returns a newly allocated value with the given Desc, ValueType,
-// sample value and label values. It panics if the number of label
-// values is different from the number of variable labels in Desc.
-func newValue(desc *Desc, valueType ValueType, val float64, labelValues ...string) *value {
-	if len(labelValues) != len(desc.variableLabels) {
-		panic(errInconsistentCardinality)
-	}
-	result := &value{
-		desc:       desc,
-		valType:    valueType,
-		valBits:    math.Float64bits(val),
-		labelPairs: makeLabelPairs(desc, labelValues),
-	}
-	result.init(result)
-	return result
-}
-
-func (v *value) Desc() *Desc {
-	return v.desc
-}
-
-func (v *value) Set(val float64) {
-	atomic.StoreUint64(&v.valBits, math.Float64bits(val))
-}
-
-func (v *value) SetToCurrentTime() {
-	v.Set(float64(time.Now().UnixNano()) / 1e9)
-}
-
-// add adjusts the underlying int64
-func (v *value) add(delta int64) {
-	atomic.AddInt64(&v.valInt, delta)
-}
-
-func (v *value) Inc() {
-	v.add(1)
-}
-
-func (v *value) Dec() {
-	v.add(-1)
-}
-
-func (v *value) Add(val float64) {
-	ival := int64(val)
-	if float64(ival) == val {
-		v.add(ival)
-		return
-	}
-
-	for {
-		oldBits := atomic.LoadUint64(&v.valBits)
-		newBits := math.Float64bits(math.Float64frombits(oldBits) + val)
-		if atomic.CompareAndSwapUint64(&v.valBits, oldBits, newBits) {
-			return
-		}
-	}
-}
-
-func (v *value) Sub(val float64) {
-	v.Add(val * -1)
-}
-
-func (v *value) Write(out *dto.Metric) error {
-	fval := math.Float64frombits(atomic.LoadUint64(&v.valBits))
-	ival := atomic.LoadInt64(&v.valInt)
-	val := fval + float64(ival)
-
-	return populateMetric(v.valType, val, v.labelPairs, out)
-}
-
 // valueFunc is a generic metric for simple values retrieved on collect time
 // from a function. It implements Metric and Collector. Its effective type is
 // determined by ValueType. This is a low-level building block used by the