From a0a60ca079f211cbb4cfddd99fdc6cf352931744 Mon Sep 17 00:00:00 2001 From: Paul Bellamy Date: Mon, 15 Feb 2016 12:28:25 +0000 Subject: [PATCH] Remove pointer math (comparison) from render caching, as it is unreliable --- app/benchmark_internal_test.go | 7 +++++-- render/benchmark_test.go | 8 ++++++-- render/memoise.go | 25 +++++++------------------ render/memoise_test.go | 10 ++++++++++ render/render.go | 8 ++++++++ render/render_test.go | 9 +++------ render/selectors.go | 3 +++ 7 files changed, 42 insertions(+), 28 deletions(-) diff --git a/app/benchmark_internal_test.go b/app/benchmark_internal_test.go index e6df6bb771..f5baa87e2c 100644 --- a/app/benchmark_internal_test.go +++ b/app/benchmark_internal_test.go @@ -9,7 +9,6 @@ import ( "github.com/ugorji/go/codec" - "github.com/weaveworks/scope/render" "github.com/weaveworks/scope/report" "github.com/weaveworks/scope/test/fixture" ) @@ -51,7 +50,11 @@ func BenchmarkTopologyList(b *testing.B) { Form: url.Values{}, } for i := 0; i < b.N; i++ { - render.ResetCache() + b.StopTimer() + topologyRegistry.walk(func(t APITopologyDesc) { + t.renderer.ResetCache() + }) + b.StartTimer() topologyRegistry.renderTopologies(report, request) } } diff --git a/render/benchmark_test.go b/render/benchmark_test.go index 52bb3c0047..8ebad70115 100644 --- a/render/benchmark_test.go +++ b/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++ { - render.ResetCache() + b.StopTimer() + r.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 :( - render.ResetCache() + b.StopTimer() + r.ResetCache() + b.StartTimer() benchmarkStatsResult = r.Stats(report) } } diff --git a/render/memoise.go b/render/memoise.go index 6bee0c647c..0b93f8b34d 100644 --- a/render/memoise.go +++ b/render/memoise.go @@ -1,44 +1,33 @@ package render import ( - "fmt" - "reflect" - "github.com/bluele/gcache" "github.com/weaveworks/scope/report" ) -var renderCache = gcache.New(100).LRU().Build() - type memoise struct { Renderer + cache gcache.Cache } // 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{r, gcache.New(10).LRU().Build()} } // 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) - } - if result, err := renderCache.Get(key); err == nil { + if result, err := m.cache.Get(rpt.ID); err == nil { return result.(RenderableNodes) } output := m.Renderer.Render(rpt) - renderCache.Set(key, output) + m.cache.Set(rpt.ID, output) return output } // ResetCache blows away the rendered node cache. -func ResetCache() { - renderCache.Purge() +func (m *memoise) ResetCache() { + m.cache.Purge() + m.Renderer.ResetCache() } diff --git a/render/memoise_test.go b/render/memoise_test.go index fe99485550..f7a8f10e8b 100644 --- a/render/memoise_test.go +++ b/render/memoise_test.go @@ -13,6 +13,7 @@ type renderFunc func(r report.Report) render.RenderableNodes func (f renderFunc) Render(r report.Report) render.RenderableNodes { return f(r) } func (f renderFunc) Stats(r report.Report) render.Stats { return render.Stats{} } +func (f renderFunc) ResetCache() {} func TestMemoise(t *testing.T) { calls := 0 @@ -48,4 +49,13 @@ func TestMemoise(t *testing.T) { if calls != 2 { t.Errorf("Expected renderer to have been called again for a different report") } + + m.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") + } } diff --git a/render/render.go b/render/render.go index a57f99be50..6128f7de53 100644 --- a/render/render.go +++ b/render/render.go @@ -8,6 +8,7 @@ import ( type Renderer interface { Render(report.Report) RenderableNodes Stats(report.Report) Stats + ResetCache() } // Stats is the type returned by Renderer.Stats @@ -49,6 +50,13 @@ func (r *Reduce) Stats(rpt report.Report) Stats { return result } +// ResetCache blows away the rendered node cache. +func (r *Reduce) ResetCache() { + for _, renderer := range *r { + renderer.ResetCache() + } +} + // Map is a Renderer which produces a set of RenderableNodes from the set of // RenderableNodes produced by another Renderer. type Map struct { diff --git a/render/render_test.go b/render/render_test.go index 4ff601a00e..d4781bd807 100644 --- a/render/render_test.go +++ b/render/render_test.go @@ -13,12 +13,9 @@ 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 (m mockRenderer) ResetCache() {} func TestReduceRender(t *testing.T) { renderer := render.Reduce([]render.Renderer{ diff --git a/render/selectors.go b/render/selectors.go index c7fe54047c..31af257316 100644 --- a/render/selectors.go +++ b/render/selectors.go @@ -18,6 +18,9 @@ func (t TopologySelector) Stats(r report.Report) Stats { return Stats{} } +// ResetCache implements Renderer +func (t TopologySelector) ResetCache() {} + // MakeRenderableNodes converts a topology to a set of RenderableNodes func MakeRenderableNodes(t report.Topology) RenderableNodes { result := RenderableNodes{}