Avoid once.Do under the lock; restore the RWMutex (#404)

CHANGELOG.md

@@ -8,20 +8,25 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ## Unreleased
 
-## [1.13.2](https://github.com/lightstep/otel-launcher-go/releases/tag/v1.13.2) - 2022-02-28)
+## [1.13.3](https://github.com/lightstep/otel-launcher-go/releases/tag/v1.13.3) - 2023-03-01)
+
+- Fixes performance regressions introduced in version 1.13.1 and 1.12.1.
+  [#404](https://github.com/lightstep/otel-launcher-go/pull/404)
+
+## [1.13.2](https://github.com/lightstep/otel-launcher-go/releases/tag/v1.13.2) - 2023-02-28)
 
 - Adds performance setting `InactiveCollectionPeriods` allowing
   control over how fast records are removed from memory. [#396](https://github.com/lightstep/otel-launcher-go/pull/396)
 
-## [1.13.1](https://github.com/lightstep/otel-launcher-go/releases/tag/v1.13.1) - 2022-02-15)
+## [1.13.1](https://github.com/lightstep/otel-launcher-go/releases/tag/v1.13.1) - 2023-02-15)
 
 - Adds performance improvements for concurrent use of synchronous
   instruments. [#384](https://github.com/lightstep/otel-launcher-go/pull/384)
 - Adds `WithPerformance()` and `IgnoreCollisions` setting which offers
   around 10% faster operations in exchange for safety and correctness.  This
   setting is off by default. [#384](https://github.com/lightstep/otel-launcher-go/pull/384)
 
-## [1.13.0](https://github.com/lightstep/otel-launcher-go/releases/tag/v1.13.0) - 2022-02-15)
+## [1.13.0](https://github.com/lightstep/otel-launcher-go/releases/tag/v1.13.0) - 2023-02-15)
 
 - Updates OTel-Go version dependencies to `[email protected]`, `[email protected]`, 
   `[email protected]`, `[email protected]`:
@@ -31,7 +36,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
     deprecated APIs.  The next minor version of this repository will
 	update the dependency to `v0.36.0` or later.  [#381](https://github.com/lightstep/otel-launcher-go/pull/381)
 
-## [1.12.1](https://github.com/lightstep/otel-launcher-go/releases/tag/v1.12.1) - 2022-02-13)
+## [1.12.1](https://github.com/lightstep/otel-launcher-go/releases/tag/v1.12.1) - 2023-02-13)
 
 - Replace a RWMutex with Mutex. [#378](https://github.com/lightstep/otel-launcher-go/pull/378)
 - Eliminate redundant GC cpu-time metric from lightstep/instrumentation/cputime as

VERSION

@@ -1 +1 @@
-1.13.2
+1.13.3

go.mod

@@ -3,8 +3,8 @@ module github.com/lightstep/otel-launcher-go
 go 1.18
 
 require (
-	github.com/lightstep/otel-launcher-go/lightstep/sdk/metric v1.13.2
-	github.com/lightstep/otel-launcher-go/pipelines v1.13.2
+	github.com/lightstep/otel-launcher-go/lightstep/sdk/metric v1.13.3
+	github.com/lightstep/otel-launcher-go/pipelines v1.13.3
 	github.com/sethvargo/go-envconfig v0.8.3
 	github.com/stretchr/testify v1.8.1
 	go.opentelemetry.io/otel v1.12.0
@@ -23,7 +23,7 @@ require (
 	github.com/golang/protobuf v1.5.2 // indirect
 	github.com/grpc-ecosystem/grpc-gateway/v2 v2.7.0 // indirect
 	github.com/lightstep/go-expohisto v1.0.0 // indirect
-	github.com/lightstep/otel-launcher-go/lightstep/instrumentation v1.13.2 // indirect
+	github.com/lightstep/otel-launcher-go/lightstep/instrumentation v1.13.3 // indirect
 	github.com/lufia/plan9stats v0.0.0-20211012122336-39d0f177ccd0 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/power-devops/perfstat v0.0.0-20210106213030-5aafc221ea8c // indirect

launcher/version.go

@@ -14,4 +14,4 @@
 
 package launcher
 
-const version = "1.13.2"
+const version = "1.13.3"

lightstep/sdk/metric/example/go.mod

@@ -3,7 +3,7 @@ module github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/example
 go 1.18
 
 require (
-	github.com/lightstep/otel-launcher-go/lightstep/sdk/metric v1.13.2
+	github.com/lightstep/otel-launcher-go/lightstep/sdk/metric v1.13.3
 	github.com/lightstep/otel-launcher-go/pipelines v1.8.0
 	go.opentelemetry.io/proto/otlp v0.19.0
 )

lightstep/sdk/metric/internal/syncstate/sync.go

@@ -54,7 +54,7 @@ type Observer struct {
 	compiled viewstate.Instrument
 
 	// lock protects current.
-	lock sync.Mutex
+	lock sync.RWMutex
 
 	// current is protected by lock.
 	current map[uint64]*record
@@ -209,12 +209,11 @@ type record struct {
 	// this field is unused when Performance.IgnoreCollisions is true.
 	next *record
 
-	// once governs access to `attrsUnsafe` and
-	// `accumulatorsUnsafe`.  The caller that created the `record`
-	// must call once.Do(initialize) on its own code path, although
-	// another goroutine might actually perform the
-	// initialization.  This is arranged with the use of
-	// readAccumulator() and readAttributes().
+	// once governs access to `accumulatorsUnsafe`.  The caller
+	// that created the `record` must call once.Do(initialize) on
+	// its own code path, although another goroutine might
+	// actually perform the initialization.  This is arranged with
+	// the use of readAccumulator().
 	once sync.Once
 
 	// accumulatorUnsafe can be a multi-accumulator if there
@@ -226,18 +225,22 @@ type record struct {
 	// to ensure that once.Do(initialize) is called.
 	accumulatorUnsafe viewstate.Accumulator
 
-	// attrsUnsafe is set in acquireUninitialized by the caller that
-	// creates the provisional new record, after not finding it in
-	// acquireRead.
+	// attrsListUnsafe is used as a temporary to calculate the
+	// attrsSet, which depends on whether collisions are checked.
 	//
-	// These attributes are in user-specified order and may contain
-	// duplicates.  When the record is initialized, this field is
-	// set to a copy of the attribute set that first observed the
-	// fingerprint.
+	// if IgnoreCollisions is true, this value is non-nil from the
+	// point of construction until once.Do(initialize) is called
+	// in the Update or Collect code path.  After once.Do(initialize),
+	// this field is nil.
 	//
-	// When IgnoreCollisions is true, this field is used as a temporary
-	// in building a new attribute set, then set to nil.
-	attrsUnsafe attribute.Sortable
+	// if IgnoreCollisions is false, this value is set to a copy
+	// of the original input attribute list, for comparison,
+	// insife computeAttrsUnderLock.
+	attrsListUnsafe attribute.Sortable
+
+	// attrsSet is the attributeSet computed from the input attributes.
+	// when the accumulator has been computed, this is set to nil.
+	attrsSet attribute.Set
 }
 
 // normalCollect equals conditionalCollect(false), is named
@@ -274,13 +277,6 @@ func (rec *record) conditionalCollect(release bool) bool {
 	return true
 }
 
-// readAttributes gets a copy of the attributes matching a fingerprint after
-// once.Do(initialize).
-func (rec *record) readAttributes() []attribute.KeyValue {
-	rec.once.Do(rec.initialize)
-	return []attribute.KeyValue(rec.attrsUnsafe)
-}
-
 // readAttributes gets the accumulator for this record after once.Do(initialize).
 func (rec *record) readAccumulator() viewstate.Accumulator {
 	rec.once.Do(rec.initialize)
@@ -293,19 +289,30 @@ func (rec *record) readAccumulator() viewstate.Accumulator {
 // behavior of this method depends on IgnoreCollisions, as documented in the
 // corresponding "unsafe" fields.
 func (rec *record) initialize() {
+	if rec.inst.performance.IgnoreCollisions {
+		rec.attrsSet = attribute.NewSetWithSortable(rec.attrsListUnsafe, &rec.attrsListUnsafe)
+	}
 
-	var aset attribute.Set
+	rec.accumulatorUnsafe = rec.inst.compiled.NewAccumulator(rec.attrsSet)
+	rec.attrsSet = attribute.Set{}
+}
 
+// computeAttrsUnderLock sets the attribute.Set that will be used to
+// construct the accumulator.
+func (rec *record) computeAttrsUnderLock(attrs []attribute.KeyValue) {
 	if rec.inst.performance.IgnoreCollisions {
-		aset = attribute.NewSetWithSortable(rec.attrsUnsafe, &rec.attrsUnsafe)
-	} else {
-		acpy := make(attribute.Sortable, len(rec.attrsUnsafe))
-		copy(acpy, rec.attrsUnsafe)
-		aset = attribute.NewSetWithSortable(acpy, &rec.attrsUnsafe)
-		rec.attrsUnsafe = acpy
+		// The work of NewSetWithSortable is deferred until
+		// once.Do(initialize) outside of the lock.
+		rec.attrsListUnsafe = attrs
+		return
 	}
 
-	rec.accumulatorUnsafe = rec.inst.compiled.NewAccumulator(aset)
+	acpy := make(attribute.Sortable, len(attrs))
+	copy(acpy, attrs)
+	rec.attrsSet = attribute.NewSetWithSortable(acpy, &rec.attrsListUnsafe)
+
+	// Note the next assignment has to follow NewSetWithSortable(), which clears the field.
+	rec.attrsListUnsafe = acpy
 }
 
 func (inst *Observer) ObserveInt64(ctx context.Context, num int64, attrs ...attribute.KeyValue) {
@@ -434,14 +441,14 @@ func attributesEqual(a, b []attribute.KeyValue) bool {
 
 // acquireRead acquires the read lock and searches for a `*record`.
 func acquireRead(inst *Observer, fp uint64, attrs []attribute.KeyValue) *record {
-	inst.lock.Lock()
-	defer inst.lock.Unlock()
+	inst.lock.RLock()
+	defer inst.lock.RUnlock()
 
 	rec := inst.current[fp]
 
 	// Potentially test for hash collisions.
 	if !inst.performance.IgnoreCollisions {
-		for rec != nil && !attributesEqual(attrs, rec.readAttributes()) {
+		for rec != nil && !attributesEqual(attrs, rec.attrsListUnsafe) {
 			rec = rec.next
 		}
 	}
@@ -473,10 +480,10 @@ func acquireUninitialized[N number.Any](inst *Observer, attrs []attribute.KeyVal
 // locate a record.
 func acquireNotfound[N number.Any](inst *Observer, fp uint64, attrs []attribute.KeyValue) *record {
 	newRec := &record{
-		inst:        inst,
-		refMapped:   newRefcountMapped(),
-		attrsUnsafe: attrs,
+		inst:      inst,
+		refMapped: newRefcountMapped(),
 	}
+	newRec.computeAttrsUnderLock(attrs)
 
 	for {
 		acquired, loaded := acquireWrite(inst, fp, newRec)
@@ -500,7 +507,7 @@ func acquireWrite(inst *Observer, fp uint64, newRec *record) (*record, bool) {
 
 	for oldRec := inst.current[fp]; oldRec != nil; oldRec = oldRec.next {
 
-		if inst.performance.IgnoreCollisions || attributesEqual(oldRec.readAttributes(), newRec.attrsUnsafe) {
+		if inst.performance.IgnoreCollisions || attributesEqual(oldRec.attrsListUnsafe, newRec.attrsListUnsafe) {
 			if oldRec.refMapped.ref() {
 				return oldRec, true
 			}

lightstep/sdk/metric/internal/syncstate/sync_test.go

@@ -56,6 +56,11 @@ var (
 		IgnoreCollisions:          false,
 		InactiveCollectionPeriods: 1,
 	}
+
+	unsafePerf = sdkinstrument.Performance{
+		IgnoreCollisions:          true,
+		InactiveCollectionPeriods: 1,
+	}
 )
 
 func deltaUpdate[N number.Any](old, new N) N {
@@ -107,6 +112,11 @@ func TestSyncStateCumulativeConcurrencyFloatFiltered(t *testing.T) {
 }
 
 func testSyncStateConcurrency[N number.Any, Traits number.Traits[N]](t *testing.T, update func(old, new N) N, vopts ...view.Option) {
+	t.Run("unsafe_collisions", func(t *testing.T) { testSyncStateConcurrencyWithPerf[N, Traits](t, unsafePerf, update, vopts...) })
+	t.Run("safe_collisions", func(t *testing.T) { testSyncStateConcurrencyWithPerf[N, Traits](t, safePerf, update, vopts...) })
+}
+
+func testSyncStateConcurrencyWithPerf[N number.Any, Traits number.Traits[N]](t *testing.T, perf sdkinstrument.Performance, update func(old, new N) N, vopts ...view.Option) {
 	// Note: prior to
 	// https://github.com/lightstep/otel-launcher-go/pull/206 this
 	// code was able to reproduce the race condition handled in
@@ -148,7 +158,7 @@ func testSyncStateConcurrency[N number.Any, Traits number.Traits[N]](t *testing.
 		pipes[vci], _ = vcs[vci].Compile(desc)
 	}
 
-	inst := New(desc, safePerf, nil, pipes)
+	inst := New(desc, perf, nil, pipes)
 	require.NotNil(t, inst)
 
 	ctx, cancel := context.WithCancel(context.Background())

pipelines/go.mod

@@ -51,8 +51,8 @@ require (
 )
 
 require (
-	github.com/lightstep/otel-launcher-go/lightstep/instrumentation v1.13.2
-	github.com/lightstep/otel-launcher-go/lightstep/sdk/metric v1.13.2
+	github.com/lightstep/otel-launcher-go/lightstep/instrumentation v1.13.3
+	github.com/lightstep/otel-launcher-go/lightstep/sdk/metric v1.13.3
 	go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc v0.35.0
 )