Optimise merge where one side is a subset of the other #3253

bboreham · 2018-07-04T07:55:30Z

This is beneficial because Scope very often merges multiple versions of the same node over time that have exactly the same labels, tags, etc., so we detect this and avoid copying.
This PR covers just three data structures - StringSet, StringLatestMap and NodeControlDataLatestMap - which benefit the most in profiles.

All three now use the same loop structure to merge. The loop is coded twice: once as far as we can get without changing the result, and then again after allocating space for a new return value. If we got all the way to the end without changing anything, then return what we already have.

StringSet is mainly used in Sets where it benefits to avoid assigning to its map, so add an unchanged return.

Before (best of five):

BenchmarkReportMerge-2   	       3	 415559420 ns/op	110109765 B/op	  887819 allocs/op
BenchmarkReportUpgrade-2   	      20	  74962687 ns/op	 4983769 B/op	   22949 allocs/op

After:

BenchmarkReportMerge-2   	       5	 285569184 ns/op	63896876 B/op	  475350 allocs/op
BenchmarkReportUpgrade-2   	      20	  74528873 ns/op	 4856011 B/op	   18995 allocs/op

report/latest_map_generated.go

+		case j >= len(n) || m[i].key < n[j].key:
+			i++
+		case m[i].key == n[j].key:
+			if m[i].Timestamp.Before(n[j].Timestamp) {


report/string_set.go

+
+	for i < len(s) {
+		switch {
+		case j >= len(other) || s[i] < other[j]:


rade · 2018-07-04T08:40:38Z

I suggest cherry-picking de47a0e (Less verbose string dump) onto master, since it's a bit of a distraction in this PR.

Add a test case where one input is longer than the other; also run each case both ways round. Omit the case which was already the reverse of another.

So that we have equal chance of merging older into newer or vice-versa

bboreham · 2018-07-11T19:27:10Z

This PR is ready for review again.

rade

A couple of niggles which would be worth addressing. Other than that, LGTM.

report/latest_map_generated.go

+	if len(n) > lenm {
+		m, n, lenm = n, m, len(n) //swap so m is always at least as long as n
+	} else if len(n) == lenm && m[0].Timestamp.Before(n[0].Timestamp) {
+		m, n = n, m // swap equal-length arrays so first element of m is newer


Swap the two maps so we are merging older into later.

If we run out of things to look at in the other map, return quickly. Also move the equal-key case above the less-than case, since maps with equal keys are the common case when merging.

Make StringSet.Merge() work like StringLatestMap.Merge()

bboreham added the performance Excessive resource usage and latency; usually a bug or chore label Jul 4, 2018

rade reviewed Jul 4, 2018

View reviewed changes

report/string_set.go Outdated

for i < len(s) {

switch {

case j >= len(other) || s[i] < other[j]:

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

bboreham added 2 commits July 9, 2018 19:19

Extend TestLatestMapMerge()

d60f303

Add a test case where one input is longer than the other; also run each case both ways round. Omit the case which was already the reverse of another.

Shuffle reports before merging, in benchmark

a06d82c

So that we have equal chance of merging older into newer or vice-versa

bboreham force-pushed the optimise-merge-subset branch 2 times, most recently from 3886809 to 4ddce89 Compare July 9, 2018 22:12

bboreham mentioned this pull request Jul 11, 2018

Remove First and Last data members from Metrics structs #3266

Merged

rade approved these changes Jul 11, 2018

View reviewed changes

bboreham added 6 commits July 12, 2018 19:50

Don't copy LatestMap if merging a subset

43a0a7e

Optimise merge of LatestMaps with same keys, different timestamps

c91fd6c

Swap the two maps so we are merging older into later.

Early return from LatestMap merge loop

dac008b

If we run out of things to look at in the other map, return quickly. Also move the equal-key case above the less-than case, since maps with equal keys are the common case when merging.

Don't copy StringSet if merging a subset

8e305f1

Make StringSet.Merge() work like StringLatestMap.Merge()

Make TestSetsMerge() check both ways round

d37611a

Test Sets.Add()

692214b

bboreham force-pushed the optimise-merge-subset branch from 4ddce89 to 692214b Compare July 12, 2018 20:01

rade approved these changes Jul 12, 2018

View reviewed changes

bboreham merged commit 9ff89c8 into master Jul 13, 2018

bboreham deleted the optimise-merge-subset branch July 13, 2018 07:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimise merge where one side is a subset of the other #3253

Optimise merge where one side is a subset of the other #3253

bboreham commented Jul 4, 2018 •

edited

Loading

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

rade commented Jul 4, 2018

bboreham commented Jul 11, 2018

rade left a comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

Optimise merge where one side is a subset of the other #3253

Optimise merge where one side is a subset of the other #3253

Conversation

bboreham commented Jul 4, 2018 • edited Loading

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

rade commented Jul 4, 2018

bboreham commented Jul 11, 2018

rade left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

bboreham commented Jul 4, 2018 •

edited

Loading