Revert "metrics: use sync.map in registry (ethereum#27159)"

This reverts commit dd2f5b3.

metrics/registry.go

@@ -45,17 +45,21 @@ type Registry interface {
 
 	// Unregister the metric with the given name.
 	Unregister(string)
+
+	// Unregister all metrics.  (Mostly for testing.)
+	UnregisterAll()
 }
 
-// The standard implementation of a Registry uses sync.map
+// The standard implementation of a Registry is a mutex-protected map
 // of names to metrics.
 type StandardRegistry struct {
-	metrics sync.Map
+	metrics map[string]interface{}
+	mutex   sync.Mutex
 }
 
 // Create a new registry.
 func NewRegistry() Registry {
-	return &StandardRegistry{}
+	return &StandardRegistry{metrics: make(map[string]interface{})}
 }
 
 // Call the given function for each registered metric.
@@ -67,57 +71,45 @@ func (r *StandardRegistry) Each(f func(string, interface{})) {
 
 // Get the metric by the given name or nil if none is registered.
 func (r *StandardRegistry) Get(name string) interface{} {
-	item, _ := r.metrics.Load(name)
-	return item
+	r.mutex.Lock()
+	defer r.mutex.Unlock()
+	return r.metrics[name]
 }
 
 // Gets an existing metric or creates and registers a new one. Threadsafe
 // alternative to calling Get and Register on failure.
 // The interface can be the metric to register if not found in registry,
 // or a function returning the metric for lazy instantiation.
 func (r *StandardRegistry) GetOrRegister(name string, i interface{}) interface{} {
-	// fast path
-	cached, ok := r.metrics.Load(name)
-	if ok {
-		return cached
+	r.mutex.Lock()
+	defer r.mutex.Unlock()
+	if metric, ok := r.metrics[name]; ok {
+		return metric
 	}
 	if v := reflect.ValueOf(i); v.Kind() == reflect.Func {
 		i = v.Call(nil)[0].Interface()
 	}
-	item, _, ok := r.loadOrRegister(name, i)
-	if !ok {
-		return i
-	}
-	return item
+	r.register(name, i)
+	return i
 }
 
 // Register the given metric under the given name.  Returns a DuplicateMetric
 // if a metric by the given name is already registered.
 func (r *StandardRegistry) Register(name string, i interface{}) error {
-	// fast path
-	_, ok := r.metrics.Load(name)
-	if ok {
-		return DuplicateMetric(name)
-	}
-
-	if v := reflect.ValueOf(i); v.Kind() == reflect.Func {
-		i = v.Call(nil)[0].Interface()
-	}
-	_, loaded, _ := r.loadOrRegister(name, i)
-	if loaded {
-		return DuplicateMetric(name)
-	}
-	return nil
+	r.mutex.Lock()
+	defer r.mutex.Unlock()
+	return r.register(name, i)
 }
 
 // Run all registered healthchecks.
 func (r *StandardRegistry) RunHealthchecks() {
-	r.metrics.Range(func(key, value any) bool {
-		if h, ok := value.(Healthcheck); ok {
+	r.mutex.Lock()
+	defer r.mutex.Unlock()
+	for _, i := range r.metrics {
+		if h, ok := i.(Healthcheck); ok {
 			h.Check()
 		}
-		return true
-	})
+	}
 }
 
 // GetAll metrics in the Registry
@@ -185,31 +177,45 @@ func (r *StandardRegistry) GetAll() map[string]map[string]interface{} {
 
 // Unregister the metric with the given name.
 func (r *StandardRegistry) Unregister(name string) {
+	r.mutex.Lock()
+	defer r.mutex.Unlock()
 	r.stop(name)
-	r.metrics.LoadAndDelete(name)
+	delete(r.metrics, name)
+}
+
+// Unregister all metrics.  (Mostly for testing.)
+func (r *StandardRegistry) UnregisterAll() {
+	r.mutex.Lock()
+	defer r.mutex.Unlock()
+	for name := range r.metrics {
+		r.stop(name)
+		delete(r.metrics, name)
+	}
 }
 
-func (r *StandardRegistry) loadOrRegister(name string, i interface{}) (interface{}, bool, bool) {
+func (r *StandardRegistry) register(name string, i interface{}) error {
+	if _, ok := r.metrics[name]; ok {
+		return DuplicateMetric(name)
+	}
 	switch i.(type) {
 	case Counter, CounterFloat64, Gauge, GaugeFloat64, Healthcheck, Histogram, Meter, Timer, ResettingTimer:
-	default:
-		return nil, false, false
+		r.metrics[name] = i
 	}
-	item, loaded := r.metrics.LoadOrStore(name, i)
-	return item, loaded, true
+	return nil
 }
 
 func (r *StandardRegistry) registered() map[string]interface{} {
-	metrics := make(map[string]interface{})
-	r.metrics.Range(func(key, value any) bool {
-		metrics[key.(string)] = value
-		return true
-	})
+	r.mutex.Lock()
+	defer r.mutex.Unlock()
+	metrics := make(map[string]interface{}, len(r.metrics))
+	for name, i := range r.metrics {
+		metrics[name] = i
+	}
 	return metrics
 }
 
 func (r *StandardRegistry) stop(name string) {
-	if i, ok := r.metrics.Load(name); ok {
+	if i, ok := r.metrics[name]; ok {
 		if s, ok := i.(Stoppable); ok {
 			s.Stop()
 		}
@@ -302,6 +308,11 @@ func (r *PrefixedRegistry) Unregister(name string) {
 	r.underlying.Unregister(realName)
 }
 
+// Unregister all metrics.  (Mostly for testing.)
+func (r *PrefixedRegistry) UnregisterAll() {
+	r.underlying.UnregisterAll()
+}
+
 var (
 	DefaultRegistry    = NewRegistry()
 	EphemeralRegistry  = NewRegistry()

metrics/registry_test.go

@@ -1,7 +1,6 @@
 package metrics
 
 import (
-	"sync"
 	"testing"
 )
 
@@ -14,30 +13,6 @@ func BenchmarkRegistry(b *testing.B) {
 	}
 }
 
-func BenchmarkRegistryGetOrRegisterParallel_8(b *testing.B) {
-	benchmarkRegistryGetOrRegisterParallel(b, 8)
-}
-
-func BenchmarkRegistryGetOrRegisterParallel_32(b *testing.B) {
-	benchmarkRegistryGetOrRegisterParallel(b, 32)
-}
-
-func benchmarkRegistryGetOrRegisterParallel(b *testing.B, amount int) {
-	r := NewRegistry()
-	b.ResetTimer()
-	var wg sync.WaitGroup
-	for i := 0; i < amount; i++ {
-		wg.Add(1)
-		go func() {
-			for i := 0; i < b.N; i++ {
-				r.GetOrRegister("foo", NewMeter)
-			}
-			wg.Done()
-		}()
-	}
-	wg.Wait()
-}
-
 func TestRegistry(t *testing.T) {
 	r := NewRegistry()
 	r.Register("foo", NewCounter())