-
Notifications
You must be signed in to change notification settings - Fork 272
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
perf: use a reusable bytes.Buffer to speed up MutableTree.String #456
perf: use a reusable bytes.Buffer to speed up MutableTree.String #456
Conversation
e72c428
to
2e4d11a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
I'm a bit skeptical of the sync pool being helpful here since I don't get why were going to multiple calls to string.
However, this change is clearly much more helpful than not having it! And it'll be pretty easy to see if theres any overheads due to sync pool in the future
@odeke-em can you update this branch with master then I can merge it |
The prior code used a naive string concatenation str += "..." which is very slow and inefficient especially when iterating over all the keys, but also it performed an unnecessary byteslice->string conversion for all arguments inside (nodeDB).traverse* using str += fmt.Sprintf("%s: %x\n", string(key), value) notice the `string(key)` passed into fmt.Sprintf("%s"? At bare minimum that code should just simply be: str += fmt.Sprintf("%s: %x\n", key, value) per https://twitter.com/orijtech/status/1462100381803102213?s=20 which saves a whole lot of cycles and RAM. This change uses: * (*bytes.Buffer) in combination with fmt.Fprintf * A sync.Pool with (*bytes.Buffer) values to reuse buffers and the results are profound: ```shell $ benchstat before.txt after.txt name old time/op new time/op delta TreeString-8 111ms ± 8% 4ms ± 4% -96.01% (p=0.000 n=20+20) name old alloc/op new alloc/op delta TreeString-8 734MB ± 0% 2MB ± 1% -99.72% (p=0.000 n=20+20) name old allocs/op new allocs/op delta TreeString-8 37.0k ± 0% 28.7k ± 0% -22.40% (p=0.000 n=20+20) ``` Fixes cosmos#455
2e4d11a
to
d2df784
Compare
Done, thank you @marbar3778! Thank you @ValarDragon @marbar3778 for the reviews! |
Probably we could optimize it even further with |
The prior code used a naive string concatenation
str += "..."
which is very slow and inefficient especially when iterating over
all the keys, but also it performed an unnecessary byteslice->string
conversion for all arguments inside (nodeDB).traverse* using
notice the
string(key)
passed into fmt.Sprintf("%s"?At bare minimum that code should just simply be:
per https://twitter.com/orijtech/status/1462100381803102213?s=20
which saves a whole lot of cycles and RAM.
This change uses:
and the results are profound:
Fixes #455