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

optimisation: allocate less memory in LatestMap merging #2956

Merged
merged 4 commits into from
Dec 5, 2017

Conversation

rade
Copy link
Member

@rade rade commented Dec 2, 2017

Most maps we merge have the same keys, or at least one set of keys is the subset of the other. Therefore, allocate a result slice capable of holding only the max of number keys, rather than the sum.

rade added 2 commits December 2, 2017 11:29
so they can share code and are easier to run in combination.

We take advantage of the code sharing by generalising the report
rendering benchmarks to read & merge reports from a dir.
@rade rade requested a review from bboreham December 2, 2017 12:47
@rade
Copy link
Member Author

rade commented Dec 2, 2017

benchmarks, using 10s worth of reports, best results of four runs of

go test -tags 'netgo unsafe' -run=x -cpu=2 -bench='ReportMerge|Topology' --bench-report-path=/tmp/prod-20171129-10s/

master

BenchmarkReportMerge-2           	       1	1610884076 ns/op	492567472 B/op	 2609989 allocs/op
BenchmarkTopologyList-2          	       3	 465342980 ns/op	85119557 B/op	  761973 allocs/op
BenchmarkTopologyHosts-2         	       3	 342374733 ns/op	61127706 B/op	  573150 allocs/op
BenchmarkTopologyControllers-2   	       5	 264158336 ns/op	43622580 B/op	  401689 allocs/op
BenchmarkTopologyPods-2          	       5	 230099526 ns/op	38982995 B/op	  361313 allocs/op
BenchmarkTopologyContainers-2    	      10	 174144297 ns/op	27472680 B/op	  247819 allocs/op
BenchmarkTopologyProcesses-2     	      10	 105820331 ns/op	12242231 B/op	   98481 allocs/op

branch

BenchmarkReportMerge-2           	       1	1528383324 ns/op	407973696 B/op	 2630831 allocs/op
BenchmarkTopologyList-2          	       3	 447941278 ns/op	84772469 B/op	  761716 allocs/op
BenchmarkTopologyHosts-2         	       3	 339453287 ns/op	60685725 B/op	  571771 allocs/op
BenchmarkTopologyControllers-2   	       5	 268746637 ns/op	43351414 B/op	  401385 allocs/op
BenchmarkTopologyPods-2          	       5	 226778097 ns/op	38736998 B/op	  361290 allocs/op
BenchmarkTopologyContainers-2    	      10	 160692930 ns/op	27233416 B/op	  247852 allocs/op
BenchmarkTopologyProcesses-2     	      10	 104344096 ns/op	12242851 B/op	   98480 allocs/op

So we allocate ~17% less memory during merging.

I included the rendering benchmarks because rendering also merges nodes, so I wanted to check that we didn't negatively impact performance.

rade added 2 commits December 2, 2017 13:04
Most maps we merge have the same keys, or at least one set of keys is
the subset of the other. Therefore, allocate a result slice capable of
holding only the max of number keys, rather than the sum.
@rade rade force-pushed the sizing-merge-slice branch from 7134135 to 4162b5d Compare December 2, 2017 13:13
@bboreham
Copy link
Collaborator

bboreham commented Dec 4, 2017

Did you run this with memory profiling? That would show how many times the append() causes an alloc after this change.

@rade
Copy link
Member Author

rade commented Dec 4, 2017

Surely the increase in the figure should be 263083-2609989.

@rade
Copy link
Member Author

rade commented Dec 4, 2017

I don't see any allocations in the appends when running BenchmarkReportMerge:

$ go test -tags 'netgo unsafe' -run=x -cpu=2 -memprofile=/tmp/mem.out -memprofilerate=111 -bench='ReportMerge' --bench-report-path=/tmp/prod-20171129-10s/
BenchmarkReportMerge-2   	       1	9429282808 ns/op	398308736 B/op	 2595429 allocs/op
PASS
$ go tool pprof -alloc_objects -list=StringLatestMap.Merge /tmp/mem.out
Total: 8517392
ROUTINE ======================== github.com/weaveworks/scope/report.StringLatestMap.Merge in /home/matthias/go/src/github.com/weaveworks/scope/report/latest_map_generated.go
    132977     132977 (flat, cum)  1.56% of Total
         .          .     53:	}
         .          .     54:	l := len(m.entries)
         .          .     55:	if len(n.entries) > l {
         .          .     56:		l = len(n.entries)
         .          .     57:	}
    132977     132977     58:	out := make([]stringLatestEntry, 0, l)
         .          .     59:
         .          .     60:	i, j := 0, 0
         .          .     61:	for i < len(m.entries) {
         .          .     62:		switch {
         .          .     63:		case j >= len(n.entries) || m.entries[i].key < n.entries[j].key:

Same for NodeControlDataLatestMap.Merge

To check that I wasn't doing anything wrong in the profiling, I changed the code to allocate l-1 instead of l. That does result in allocations in append.

So now I mystified why my the benchmark results show an increase in the allocs/op.

@rade rade merged commit f5b38a0 into master Dec 5, 2017
@rade rade deleted the sizing-merge-slice branch December 25, 2017 10:12
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