Merge pull request #233 from marcusking01/registry-mutex

Changed registry's mutex to be RWMutex for performance gains

registry.go

@@ -54,7 +54,7 @@ type Registry interface {
 // of names to metrics.
 type StandardRegistry struct {
 	metrics map[string]interface{}
-	mutex   sync.Mutex
+	mutex   sync.RWMutex
 }
 
 // Create a new registry.
@@ -71,8 +71,8 @@ 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{} {
-	r.mutex.Lock()
-	defer r.mutex.Unlock()
+	r.mutex.RLock()
+	defer r.mutex.RUnlock()
 	return r.metrics[name]
 }
 
@@ -81,6 +81,15 @@ func (r *StandardRegistry) Get(name string) interface{} {
 // 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{} {
+	// access the read lock first which should be re-entrant
+	r.mutex.RLock()
+	metric, ok := r.metrics[name]
+	r.mutex.RUnlock()
+	if ok {
+		return metric
+	}
+
+	// only take the write lock if we'll be modifying the metrics map
 	r.mutex.Lock()
 	defer r.mutex.Unlock()
 	if metric, ok := r.metrics[name]; ok {
@@ -103,8 +112,8 @@ func (r *StandardRegistry) Register(name string, i interface{}) error {
 
 // Run all registered healthchecks.
 func (r *StandardRegistry) RunHealthchecks() {
-	r.mutex.Lock()
-	defer r.mutex.Unlock()
+	r.mutex.RLock()
+	defer r.mutex.RUnlock()
 	for _, i := range r.metrics {
 		if h, ok := i.(Healthcheck); ok {
 			h.Check()

registry_test.go

@@ -1,6 +1,7 @@
 package metrics
 
 import (
+	"sync"
 	"testing"
 )
 
@@ -13,6 +14,16 @@ func BenchmarkRegistry(b *testing.B) {
 	}
 }
 
+func BenchmarkRegistryParallel(b *testing.B) {
+	r := NewRegistry()
+	b.ResetTimer()
+	b.RunParallel(func(pb *testing.PB) {
+		for pb.Next() {
+			r.GetOrRegister("foo", NewCounter())
+		}
+	})
+}
+
 func TestRegistry(t *testing.T) {
 	r := NewRegistry()
 	r.Register("foo", NewCounter())
@@ -301,5 +312,52 @@ func TestWalkRegistries(t *testing.T) {
 	if "prefix.prefix2." != prefix {
 		t.Fatal(prefix)
 	}
+}
+
+func TestConcurrentRegistryAccess(t *testing.T) {
+	r := NewRegistry()
+
+	counter := NewCounter()
+
+	signalChan := make(chan struct{})
+
+	var wg sync.WaitGroup
+	for i := 0; i < 10; i++ {
+		wg.Add(1)
+		go func(dowork chan struct{}) {
+			defer wg.Done()
+			iface := r.GetOrRegister("foo", counter)
+			retCounter, ok := iface.(Counter)
+			if !ok {
+				t.Fatal("Expected a Counter type")
+			}
+			if retCounter != counter {
+				t.Fatal("Counter references don't match")
+			}
+		}(signalChan)
+	}
+
+	close(signalChan) // Closing will cause all go routines to execute at the same time
+	wg.Wait()         // Wait for all go routines to do their work
 
+	// At the end of the test we should still only have a single "foo" Counter
+	i := 0
+	r.Each(func(name string, iface interface{}) {
+		i++
+		if "foo" != name {
+			t.Fatal(name)
+		}
+		if _, ok := iface.(Counter); !ok {
+			t.Fatal(iface)
+		}
+	})
+	if 1 != i {
+		t.Fatal(i)
+	}
+	r.Unregister("foo")
+	i = 0
+	r.Each(func(string, interface{}) { i++ })
+	if 0 != i {
+		t.Fatal(i)
+	}
 }