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

Map decode optimisations #2364

Merged
merged 4 commits into from
Mar 23, 2017
Merged

Map decode optimisations #2364

merged 4 commits into from
Mar 23, 2017

Conversation

bboreham
Copy link
Collaborator

This is branched after #2351, so look at that one first.

Here I apply the the map-reading code taken from LatestMap to other uses of ps.Map; this removes the indirection via a Go map and consequent copying.

Also call CodecDecodeSelf() instead of Decode() where possible: this avoids a runtime type lookup, so goes a little faster. I had to get a little funky with providing an implementation for the parser to run before the real codec code is generated; that is the purpose of dummySelfer.

Bonus: having less recursion through Decode() makes it easier to interpret profiles.

@bboreham bboreham force-pushed the map-decode-optimisations branch from f239b71 to 46d1705 Compare March 21, 2017 10:10
@2opremio
Copy link
Contributor

What's the performance improvement?

@bboreham
Copy link
Collaborator Author

bboreham commented Mar 21, 2017

Same test as in #2351:

With this PR, commit 46d1705:

$ go test -run TestDecode
ok  	github.com/weaveworks/scope/report	1.197s
ok  	github.com/weaveworks/scope/report	1.363s
ok  	github.com/weaveworks/scope/report	1.200s
ok  	github.com/weaveworks/scope/report	1.308s

Compare against 1.652:
That's about a 27% speed-up, fastest to fastest.

@bboreham
Copy link
Collaborator Author

Note CI failed because you run the unit-tests before running the code-generation step.

@2opremio
Copy link
Contributor

Uhm, could the new code generation dependency have messed up coveralls?

@bboreham
Copy link
Collaborator Author

The problem is that you weren't testing the right code before.
The coverage of report.codecgen.co is at 37%, and it wasn't included before, so that drags the average down.
Arguably this is meaningless, since most of the code paths in that file are for situations we don't expect. But I don't know what you can do about that.

@2opremio
Copy link
Contributor

OK, fair enough

@bboreham bboreham force-pushed the map-decode-optimisations branch from 1d71e21 to 3d5bc63 Compare March 22, 2017 12:34
@bboreham
Copy link
Collaborator Author

Rebased so we don't see the two commits already merged. Sorry, should have done that earlier.

This avoids a runtime type lookup, so goes a little faster.
Also having less recursion makes it easier to interpret profiles.
@bboreham bboreham force-pushed the map-decode-optimisations branch from 04acaa5 to f629ccb Compare March 23, 2017 12:25
@bboreham
Copy link
Collaborator Author

bboreham commented Mar 23, 2017

Tests failed sometimes with a "panic: Not run on same source!" message from cover:

I suspect this is caused by codecgen embedding a random number into the code, and this can be different between the two CircleCI lanes.
More specifically, the cover tool cares about the length of the number, which can be either 3 digits or 4 digits if left to chance.
So I pushed another commit fixing this number for CircleCI builds.

Also rebased just now.

'codecgen' embeds a random integer in each identifier; this means code
coverage across different CircleCI lanes may not match.
Here we force the integer to 23 on every CircleCI build so they always match.
@bboreham bboreham force-pushed the map-decode-optimisations branch from f629ccb to 97dda94 Compare March 23, 2017 13:47
@bboreham bboreham merged commit 329e540 into master Mar 23, 2017
@rade rade deleted the map-decode-optimisations branch July 5, 2017 13:10
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