Merge pull request #962 from weaveworks/no-pointer-math

Remove pointer math (comparison) from render caching, as it is unreliable

app/benchmark_internal_test.go

@@ -51,7 +51,9 @@ func BenchmarkTopologyList(b *testing.B) {
 		Form: url.Values{},
 	}
 	for i := 0; i < b.N; i++ {
+		b.StopTimer()
 		render.ResetCache()
+		b.StartTimer()
 		topologyRegistry.renderTopologies(report, request)
 	}
 }

render/benchmark_test.go

@@ -62,7 +62,9 @@ func benchmarkRender(b *testing.B, r render.Renderer) {
 	b.ResetTimer()
 
 	for i := 0; i < b.N; i++ {
+		b.StopTimer()
 		render.ResetCache()
+		b.StartTimer()
 		benchmarkRenderResult = r.Render(report)
 		if len(benchmarkRenderResult) == 0 {
 			b.Errorf("Rendered topology contained no nodes")
@@ -80,7 +82,9 @@ func benchmarkStats(b *testing.B, r render.Renderer) {
 
 	for i := 0; i < b.N; i++ {
 		// No way to tell if this was successful :(
+		b.StopTimer()
 		render.ResetCache()
+		b.StartTimer()
 		benchmarkStatsResult = r.Stats(report)
 	}
 }

render/memoise.go

@@ -2,7 +2,7 @@ package render
 
 import (
 	"fmt"
-	"reflect"
+	"math/rand"
 
 	"github.com/bluele/gcache"
 
@@ -13,23 +13,22 @@ var renderCache = gcache.New(100).LRU().Build()
 
 type memoise struct {
 	Renderer
+	id string
 }
 
 // Memoise wraps the renderer in a loving embrace of caching
-func Memoise(r Renderer) Renderer { return &memoise{r} }
+func Memoise(r Renderer) Renderer {
+	return &memoise{
+		Renderer: r,
+		id:       fmt.Sprintf("%x", rand.Int63()),
+	}
+}
 
 // Render produces a set of RenderableNodes given a Report.
 // Ideally, it just retrieves it from the cache, otherwise it calls through to
 // `r` and stores the result.
 func (m *memoise) Render(rpt report.Report) RenderableNodes {
-	key := ""
-	v := reflect.ValueOf(m.Renderer)
-	switch v.Kind() {
-	case reflect.Ptr, reflect.Func:
-		key = fmt.Sprintf("%s-%x", rpt.ID, v.Pointer())
-	default:
-		return m.Renderer.Render(rpt)
-	}
+	key := fmt.Sprintf("%s-%s", rpt.ID, m.id)
 	if result, err := renderCache.Get(key); err == nil {
 		return result.(RenderableNodes)
 	}

render/memoise_test.go

@@ -48,4 +48,13 @@ func TestMemoise(t *testing.T) {
 	if calls != 2 {
 		t.Errorf("Expected renderer to have been called again for a different report")
 	}
+
+	render.ResetCache()
+	result4 := m.Render(rpt1)
+	if !reflect.DeepEqual(result1, result4) {
+		t.Errorf("Expected original result to be returned: %s", test.Diff(result1, result4))
+	}
+	if calls != 3 {
+		t.Errorf("Expected renderer to have been called again after cache reset")
+	}
 }

render/render_test.go

@@ -13,12 +13,8 @@ type mockRenderer struct {
 	render.RenderableNodes
 }
 
-func (m mockRenderer) Render(rpt report.Report) render.RenderableNodes {
-	return m.RenderableNodes
-}
-func (m mockRenderer) Stats(rpt report.Report) render.Stats {
-	return render.Stats{}
-}
+func (m mockRenderer) Render(rpt report.Report) render.RenderableNodes { return m.RenderableNodes }
+func (m mockRenderer) Stats(rpt report.Report) render.Stats            { return render.Stats{} }
 
 func TestReduceRender(t *testing.T) {
 	renderer := render.Reduce([]render.Renderer{