From a0a60ca079f211cbb4cfddd99fdc6cf352931744 Mon Sep 17 00:00:00 2001 From: Paul Bellamy Date: Mon, 15 Feb 2016 12:28:25 +0000 Subject: [PATCH 1/2] 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 e6df6bb77..f5baa87e2 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 52bb3c004..8ebad7011 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 6bee0c647..0b93f8b34 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 fe9948555..f7a8f10e8 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 a57f99be5..6128f7de5 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 4ff601a00..d4781bd80 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 c7fe54047..31af25731 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{} From 0379fc4cf3b96734321c5981f482230c410607f8 Mon Sep 17 00:00:00 2001 From: Paul Bellamy Date: Wed, 17 Feb 2016 17:08:14 +0000 Subject: [PATCH 2/2] generate an ID for each memoiser so they can share cache Makes cache clearing easier, and then we don't need to add `ResetCache()` to the Renderer interface. --- app/benchmark_internal_test.go | 5 ++--- render/benchmark_test.go | 4 ++-- render/memoise.go | 24 +++++++++++++++++------- render/memoise_test.go | 3 +-- render/render.go | 8 -------- render/render_test.go | 1 - render/selectors.go | 3 --- 7 files changed, 22 insertions(+), 26 deletions(-) diff --git a/app/benchmark_internal_test.go b/app/benchmark_internal_test.go index f5baa87e2..8acb0feb2 100644 --- a/app/benchmark_internal_test.go +++ b/app/benchmark_internal_test.go @@ -9,6 +9,7 @@ import ( "github.com/ugorji/go/codec" + "github.com/weaveworks/scope/render" "github.com/weaveworks/scope/report" "github.com/weaveworks/scope/test/fixture" ) @@ -51,9 +52,7 @@ func BenchmarkTopologyList(b *testing.B) { } for i := 0; i < b.N; i++ { b.StopTimer() - topologyRegistry.walk(func(t APITopologyDesc) { - t.renderer.ResetCache() - }) + render.ResetCache() b.StartTimer() topologyRegistry.renderTopologies(report, request) } diff --git a/render/benchmark_test.go b/render/benchmark_test.go index 8ebad7011..84afbb758 100644 --- a/render/benchmark_test.go +++ b/render/benchmark_test.go @@ -63,7 +63,7 @@ func benchmarkRender(b *testing.B, r render.Renderer) { for i := 0; i < b.N; i++ { b.StopTimer() - r.ResetCache() + render.ResetCache() b.StartTimer() benchmarkRenderResult = r.Render(report) if len(benchmarkRenderResult) == 0 { @@ -83,7 +83,7 @@ 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() - r.ResetCache() + render.ResetCache() b.StartTimer() benchmarkStatsResult = r.Stats(report) } diff --git a/render/memoise.go b/render/memoise.go index 0b93f8b34..052c49328 100644 --- a/render/memoise.go +++ b/render/memoise.go @@ -1,33 +1,43 @@ package render import ( + "fmt" + "math/rand" + "github.com/bluele/gcache" "github.com/weaveworks/scope/report" ) +var renderCache = gcache.New(100).LRU().Build() + type memoise struct { Renderer - cache gcache.Cache + id string } // Memoise wraps the renderer in a loving embrace of caching -func Memoise(r Renderer) Renderer { return &memoise{r, gcache.New(10).LRU().Build()} } +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 { - if result, err := m.cache.Get(rpt.ID); err == nil { + key := fmt.Sprintf("%s-%s", rpt.ID, m.id) + if result, err := renderCache.Get(key); err == nil { return result.(RenderableNodes) } output := m.Renderer.Render(rpt) - m.cache.Set(rpt.ID, output) + renderCache.Set(key, output) return output } // ResetCache blows away the rendered node cache. -func (m *memoise) ResetCache() { - m.cache.Purge() - m.Renderer.ResetCache() +func ResetCache() { + renderCache.Purge() } diff --git a/render/memoise_test.go b/render/memoise_test.go index f7a8f10e8..5b1ea0424 100644 --- a/render/memoise_test.go +++ b/render/memoise_test.go @@ -13,7 +13,6 @@ 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 @@ -50,7 +49,7 @@ func TestMemoise(t *testing.T) { t.Errorf("Expected renderer to have been called again for a different report") } - m.ResetCache() + render.ResetCache() result4 := m.Render(rpt1) if !reflect.DeepEqual(result1, result4) { t.Errorf("Expected original result to be returned: %s", test.Diff(result1, result4)) diff --git a/render/render.go b/render/render.go index 6128f7de5..a57f99be5 100644 --- a/render/render.go +++ b/render/render.go @@ -8,7 +8,6 @@ import ( type Renderer interface { Render(report.Report) RenderableNodes Stats(report.Report) Stats - ResetCache() } // Stats is the type returned by Renderer.Stats @@ -50,13 +49,6 @@ 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 d4781bd80..7ac6924f5 100644 --- a/render/render_test.go +++ b/render/render_test.go @@ -15,7 +15,6 @@ type mockRenderer struct { 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 31af25731..c7fe54047 100644 --- a/render/selectors.go +++ b/render/selectors.go @@ -18,9 +18,6 @@ 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{}