From 71d3126c82e2a0bab67a93fd919e7a3566a91513 Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Fri, 6 May 2016 17:54:57 +0100 Subject: [PATCH 1/2] Limit merge cache to 200 entries and expire entries old than merge window. --- app/collector.go | 2 +- app/merger.go | 11 +++++++---- app/merger_test.go | 9 +++++---- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/app/collector.go b/app/collector.go index a33e11cff..f9dc48293 100644 --- a/app/collector.go +++ b/app/collector.go @@ -78,7 +78,7 @@ func NewCollector(window time.Duration) Collector { waitableCondition: waitableCondition{ waiters: map[chan struct{}]struct{}{}, }, - merger: NewSmartMerger(), + merger: NewSmartMerger(window), } } diff --git a/app/merger.go b/app/merger.go index 975b31d3b..69d4297d7 100644 --- a/app/merger.go +++ b/app/merger.go @@ -4,6 +4,7 @@ import ( "fmt" "math" "sort" + "time" "github.com/bluele/gcache" "github.com/spaolacci/murmur3" @@ -41,9 +42,13 @@ type smartMerger struct { // NewSmartMerger makes a Merger which merges together reports, caching intermediate merges // to accelerate future merges. Idea is to cache pair-wise merged reports, forming a merge // tree. Merging a new report into this tree should be log(N). -func NewSmartMerger() Merger { +func NewSmartMerger(window time.Duration) Merger { return &smartMerger{ - cache: gcache.New(1000).LRU().Build(), + cache: gcache.New(200). + LRU(). + Expiration(window). + EnableGC(2 * time.Second). + Build(), } } @@ -94,11 +99,9 @@ func (s *smartMerger) Merge(reports []report.Report) report.Report { // two reports is cached. merge := func(left, right *node) *node { id := hash(left.rpt.ID, right.rpt.ID) - if result, err := s.cache.Get(id); err == nil { return result.(*node) } - n := &node{ id: id, rpt: report.MakeReport().Merge(left.rpt).Merge(right.rpt), diff --git a/app/merger_test.go b/app/merger_test.go index 06abc7505..d5d4e88f3 100644 --- a/app/merger_test.go +++ b/app/merger_test.go @@ -4,6 +4,7 @@ import ( "fmt" "math/rand" "testing" + "time" "github.com/weaveworks/scope/app" "github.com/weaveworks/scope/report" @@ -28,7 +29,7 @@ func TestMerger(t *testing.T) { AddNode(report.MakeNode("bar")). AddNode(report.MakeNode("baz")) - for _, merger := range []app.Merger{app.MakeDumbMerger(), app.NewSmartMerger()} { + for _, merger := range []app.Merger{app.MakeDumbMerger(), app.NewSmartMerger(10 * time.Second)} { // Test the empty list case if have := merger.Merge([]report.Report{}); !reflect.DeepEqual(have, report.MakeReport()) { t.Errorf("Bad merge: %s", test.Diff(have, want)) @@ -62,18 +63,18 @@ func TestSmartMerger(t *testing.T) { want := report.MakeReport() want.Endpoint.AddNode(report.MakeNode("foo")) - merger := app.NewSmartMerger() + merger := app.NewSmartMerger(10 * time.Second) if have := merger.Merge(reports); !reflect.DeepEqual(have, want) { t.Errorf("Bad merge: %s", test.Diff(have, want)) } } func BenchmarkSmartMerger(b *testing.B) { - benchmarkMerger(b, app.NewSmartMerger(), false) + benchmarkMerger(b, app.NewSmartMerger(10*time.Second), false) } func BenchmarkSmartMergerWithoutCaching(b *testing.B) { - benchmarkMerger(b, app.NewSmartMerger(), true) + benchmarkMerger(b, app.NewSmartMerger(10*time.Second), true) } func BenchmarkDumbMerger(b *testing.B) { From 2dae03501e741c6a5b841964ef13db3ed544c905 Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Mon, 9 May 2016 10:08:14 +0100 Subject: [PATCH 2/2] Remove the caching --- app/collector.go | 2 +- app/merger.go | 33 ++++++++------------------------- app/merger_test.go | 27 +++++---------------------- 3 files changed, 14 insertions(+), 48 deletions(-) diff --git a/app/collector.go b/app/collector.go index f9dc48293..a33e11cff 100644 --- a/app/collector.go +++ b/app/collector.go @@ -78,7 +78,7 @@ func NewCollector(window time.Duration) Collector { waitableCondition: waitableCondition{ waiters: map[chan struct{}]struct{}{}, }, - merger: NewSmartMerger(window), + merger: NewSmartMerger(), } } diff --git a/app/merger.go b/app/merger.go index 69d4297d7..f901d2e41 100644 --- a/app/merger.go +++ b/app/merger.go @@ -4,7 +4,6 @@ import ( "fmt" "math" "sort" - "time" "github.com/bluele/gcache" "github.com/spaolacci/murmur3" @@ -39,17 +38,11 @@ type smartMerger struct { cache gcache.Cache } -// NewSmartMerger makes a Merger which merges together reports, caching intermediate merges -// to accelerate future merges. Idea is to cache pair-wise merged reports, forming a merge -// tree. Merging a new report into this tree should be log(N). -func NewSmartMerger(window time.Duration) Merger { - return &smartMerger{ - cache: gcache.New(200). - LRU(). - Expiration(window). - EnableGC(2 * time.Second). - Build(), - } +// NewSmartMerger makes a Merger which merges together reports as +// a binary tree of reports. Speed up comes from the fact that +// most merges are between small reports. +func NewSmartMerger() Merger { + return smartMerger{} } type node struct { @@ -71,11 +64,7 @@ func hash(ids ...string) uint64 { return id.Sum64() } -func (s *smartMerger) ClearCache() { - s.cache.Purge() -} - -func (s *smartMerger) Merge(reports []report.Report) report.Report { +func (s smartMerger) Merge(reports []report.Report) report.Report { // Start with a sorted list of leaves. // Note we must dedupe reports with the same ID to ensure the // algorithm below doesn't go into an infinite loop. This is @@ -98,16 +87,10 @@ func (s *smartMerger) Merge(reports []report.Report) report.Report { // Define how to merge two nodes together. The result of merging // two reports is cached. merge := func(left, right *node) *node { - id := hash(left.rpt.ID, right.rpt.ID) - if result, err := s.cache.Get(id); err == nil { - return result.(*node) - } - n := &node{ - id: id, + return &node{ + id: hash(left.rpt.ID, right.rpt.ID), rpt: report.MakeReport().Merge(left.rpt).Merge(right.rpt), } - s.cache.Set(id, n) - return n } // Define how to reduce n nodes to 1. diff --git a/app/merger_test.go b/app/merger_test.go index d5d4e88f3..94227594a 100644 --- a/app/merger_test.go +++ b/app/merger_test.go @@ -4,7 +4,6 @@ import ( "fmt" "math/rand" "testing" - "time" "github.com/weaveworks/scope/app" "github.com/weaveworks/scope/report" @@ -29,7 +28,7 @@ func TestMerger(t *testing.T) { AddNode(report.MakeNode("bar")). AddNode(report.MakeNode("baz")) - for _, merger := range []app.Merger{app.MakeDumbMerger(), app.NewSmartMerger(10 * time.Second)} { + for _, merger := range []app.Merger{app.MakeDumbMerger(), app.NewSmartMerger()} { // Test the empty list case if have := merger.Merge([]report.Report{}); !reflect.DeepEqual(have, report.MakeReport()) { t.Errorf("Bad merge: %s", test.Diff(have, want)) @@ -63,27 +62,23 @@ func TestSmartMerger(t *testing.T) { want := report.MakeReport() want.Endpoint.AddNode(report.MakeNode("foo")) - merger := app.NewSmartMerger(10 * time.Second) + merger := app.NewSmartMerger() if have := merger.Merge(reports); !reflect.DeepEqual(have, want) { t.Errorf("Bad merge: %s", test.Diff(have, want)) } } func BenchmarkSmartMerger(b *testing.B) { - benchmarkMerger(b, app.NewSmartMerger(10*time.Second), false) -} - -func BenchmarkSmartMergerWithoutCaching(b *testing.B) { - benchmarkMerger(b, app.NewSmartMerger(10*time.Second), true) + benchmarkMerger(b, app.NewSmartMerger()) } func BenchmarkDumbMerger(b *testing.B) { - benchmarkMerger(b, app.MakeDumbMerger(), false) + benchmarkMerger(b, app.MakeDumbMerger()) } const numHosts = 15 -func benchmarkMerger(b *testing.B, merger app.Merger, clearCache bool) { +func benchmarkMerger(b *testing.B, merger app.Merger) { makeReport := func() report.Report { rpt := report.MakeReport() for i := 0; i < 100; i++ { @@ -96,12 +91,6 @@ func benchmarkMerger(b *testing.B, merger app.Merger, clearCache bool) { for i := 0; i < numHosts*5; i++ { reports = append(reports, makeReport()) } - merger.Merge(reports) // prime the cache - if clearable, ok := merger.(interface { - ClearCache() - }); ok && clearCache { - clearable.ClearCache() - } b.ReportAllocs() b.ResetTimer() @@ -113,11 +102,5 @@ func benchmarkMerger(b *testing.B, merger app.Merger, clearCache bool) { } merger.Merge(reports) - - if clearable, ok := merger.(interface { - ClearCache() - }); ok && clearCache { - clearable.ClearCache() - } } }