Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-implement LatestMap as a sorted slice for better performance #2870

Merged
merged 4 commits into from
Oct 11, 2017

Conversation

bboreham
Copy link
Collaborator

@bboreham bboreham commented Sep 27, 2017

Part of fixing #2866

Benchmarks

Before

$ go test -tags netgo -bench=LatestMap
BenchmarkLatestMapMerge-2    	    3000	    403395 ns/op	  559041 B/op	    4993 allocs/op
BenchmarkLatestMapEncode-2   	    5000	    238557 ns/op	   98768 B/op	    1014 allocs/op
BenchmarkLatestMapDecode-2   	    5000	    317857 ns/op	  164944 B/op	    4004 allocs/op

After

$ go test -tags netgo -bench=LatestMap
BenchmarkLatestMapMerge-2    	   30000	     53852 ns/op	  131072 B/op	       1 allocs/op
BenchmarkLatestMapEncode-2   	   10000	    216475 ns/op	   98768 B/op	    1014 allocs/op
BenchmarkLatestMapDecode-2   	    5000	    287478 ns/op	   70492 B/op	    2005 allocs/op

}
for ; j < len(n.entries); j++ {
out = append(out, n.entries[j])
}

This comment was marked as abuse.

This comment was marked as abuse.

@rade
Copy link
Member

rade commented Oct 10, 2017

iirc rendering is quite heavy on LatestMap lookups.

I'd like to see some comparative results for pumping real reports through various stages of processing. I might have a go at that myself, since I did quite a lot of that in the past using

prog/scope --mode=app --weave=false --app.collector=file:/some-report.json

and then timing something like

time curl -so /dev/null http://localhost:4040/api/topology/hosts

extras/copyreport is also handy for benchmarking.

@rade
Copy link
Member

rade commented Oct 11, 2017

This PR makes no difference to rendering performance. Which I suppose is a good thing.

I grabbed a current Weave Cloud production report from the UI and ran

prog/scope --mode=app --weave=false --app.collector=file:/home/matthias/tmp/prod-on-prod-report.json

and then

time curl -so /dev/null http://localhost:4040/api/topology/hosts

I repeated this for a few other topology types, taking care to restart scope every time since otherwise rendering hits the internal gcache.

old new
hosts 1.3 1.3
kube-controllers 1.0 1.0
pods 1.0 1.0
containers 1.3 1.3
processes 0.3 0.3

where old=master and new=this-branch.

@rade
Copy link
Member

rade commented Oct 11, 2017

I don't see much difference when copying a report, perhaps because it's dominated by writing:

extras/copyreport/copyreport ~/tmp/prod-on-prod-report.json ~/tmp/prod-on-prod-report.msgpack.gz
time extras/copyreport/copyreport ~/tmp/prod-on-prod-report.msgpack.gz /tmp/report.msgpack.gz

old=2.6s, new=2.5s

I then applied

index 434edcd8..5bc5b135 100644
--- a/app/collector.go
+++ b/app/collector.go
@@ -248,6 +248,7 @@ func NewFileCollector(path string, window time.Duration) (Collector, error) {
                timestamps []time.Time
                reports    []report.Report
        )
+       start := time.Now()
        allTimestamped := true
        if err := filepath.Walk(path,
                func(p string, info os.FileInfo, err error) error {
@@ -272,12 +273,12 @@ func NewFileCollector(path string, window time.Duration) (Collector, error) {
                }); err != nil {
                return nil, err
        }
-       if len(reports) > 1 && allTimestamped {
-               collector := NewCollector(window)
-               go replay(collector, timestamps, reports)
-               return collector, nil
-       }
-       return StaticCollector(NewSmartMerger().Merge(reports).Upgrade()), nil
+       fmt.Println("read in", time.Since(start))
+       m := NewSmartMerger()
+       start = time.Now()
+       r := m.Merge(reports)
+       fmt.Println("merged in", time.Since(start))
+       return StaticCollector(r.Upgrade()), nil
 }
 
 func timestampFromFilepath(path string) (time.Time, error) {

and ran

prog/scope --mode=app --weave=false --app.collector=file:/home/matthias/tmp/reports2

where the specified directory contains 30s worth of Weave Cloud prod reports from a few months ago, in msgpack.gz format. I did five runs and averaged the results:

old new
read 14.0 13.0
merge 6.0 4.9

So that's a reasonable improvement in read performance and a significant improvement in merge performance.

Let's get this PR merged!

@bboreham
Copy link
Collaborator Author

I can recommend the Go benchmark support for this kind of experiment - you call ResetTimer() after initialization and it will increase the loop count until a certain time has been exceeded.

@bboreham bboreham merged commit 7a116eb into master Oct 11, 2017
@dlespiau dlespiau deleted the sliced-maps branch November 2, 2017 12:28
@rade rade mentioned this pull request Nov 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants