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

memdb: Implement btree.Item on item directly, in addition to *item #186

Closed
ValarDragon opened this issue Sep 3, 2021 · 9 comments · Fixed by #254
Closed

memdb: Implement btree.Item on item directly, in addition to *item #186

ValarDragon opened this issue Sep 3, 2021 · 9 comments · Fixed by #254

Comments

@ValarDragon
Copy link
Contributor

ValarDragon commented Sep 3, 2021

Currently we force every btree item to be *item, not just item. This in turn causes every set to require new heap allocations, which turns out to be a significant overhead. These allocations cause lots of memory to need to be garbage collected, and this ends up often being the bottleneck, not the actual btree searching operations.

In Osmosis, we get that we have ~no item spent in the btree operations, its all spent in allocating the newPair onto Heap. Heres a screenshot of a relevant flamegraph output here:
Screenshot 2021-09-03 at 12 28 35 PM

@tac0turtle
Copy link
Contributor

Hey, I can help in getting a release out to fix this, if you are willing to make a PR. I can push a branch for you to target as master has diverged too much to be used for this

@ValarDragon
Copy link
Contributor Author

smh stale bot

@tac0turtle tac0turtle reopened this Sep 29, 2021
@github-actions github-actions bot removed the Stale label Sep 30, 2021
@github-actions github-actions bot added the Stale label Oct 11, 2021
@ValarDragon
Copy link
Contributor Author

Can we please remove stale bot issue closing =/ Its not even that long of a wait atm

@tac0turtle tac0turtle reopened this Oct 16, 2021
@tac0turtle
Copy link
Contributor

yea ill fix

@tac0turtle tac0turtle removed the Stale label Oct 16, 2021
@faddat
Copy link
Contributor

faddat commented May 7, 2022

What's the status of this one?

I guess I want to include it in the notinal tracking issue

@ValarDragon do you think this is needed still?

@ValarDragon
Copy link
Contributor Author

Yeah this is still useful

@faddat
Copy link
Contributor

faddat commented May 7, 2022

great!

@faddat
Copy link
Contributor

faddat commented May 7, 2022

So, fair to say this one is more important than #188 ?

that’s good because ah, wow

Pain is a teacher I guess?

@faddat
Copy link
Contributor

faddat commented May 19, 2022

Seems set :)

creachadair pushed a commit that referenced this issue May 19, 2022
Per #186, this may help reduce allocation overhead.

Co-authored-by: romelukaku <gundamaster5@gmail.com>
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 a pull request may close this issue.

3 participants