convertVarIntToBytes: use reusable bytes array not expensive, discarded bytes.Buffer #352
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ed bytes.Buffer Noticed while auditing and profiling dependencies of cosmos-sdk, that convertVarIntToBytes, while reusing already implemented code, it was expensively creating a bytes.Buffer (40B on 64-bit architectures) returning a result and discarding it, yet that code was called 3 times successively at least. By reusing a byte array (not a slice, to ensure bounds checks eliminations by the compiler), we are able to dramatically improve performance, taking it from ~4µs down to 850ns (~4.5X reduction), reduce allocations by >=~80% in every dimension: ```shell $ benchstat before.txt after.txt name old time/op new time/op delta ConvertLeafOp-8 3.90µs ± 1% 0.85µs ± 4% -78.12% (p=0.000 n=10+10) name old alloc/op new alloc/op delta ConvertLeafOp-8 5.18kB ± 0% 0.77kB ± 0% -85.19% (p=0.000 n=10+10) name old allocs/op new allocs/op delta ConvertLeafOp-8 120 ± 0% 24 ± 0% -80.00% (p=0.000 n=10+10) ``` Fixes #344
|
@erikgrinaker please take a look, I’ve addressed your comments. Thank you! |
|
Thanks @odeke-em! |
|
Thank you @erikgrinaker for the review! Could you please help me cut a new release too? I ask because it would be nice for us to get the cosmos-sdk team to use the latest code as well as the changes you made over the weekend fixing identified issues. |
|
We cut 0.15.2 on Monday, the only change which isn't included is this one. |
|
Gotcha, thanks for the update. If you are able to, I kindly request that you help me cut a point release with this code included, as when I (memory and cpu) profile gaiad, iavl code shows up, and am investigating. |
4 tasks
|
Thank you @erikgrinaker! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Noticed while auditing and profiling dependencies of cosmos-sdk, that
convertVarIntToBytes, while reusing already implemented code, it
was expensively creating a bytes.Buffer (40B on 64-bit architectures)
returning a result and discarding it, yet that code was called
3 times successively at least.
By reusing a byte array (not a slice, to ensure bounds checks
eliminations by the compiler), we are able to dramatically improve
performance, taking it from ~4µs down to 850ns (~4.5X reduction),
reduce allocations by >=~80% in every dimension:
Fixes #344