Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make a second DB "IterKey" API that returns a key to the caller that can potentially mutate #156

Closed
ValarDragon opened this issue Apr 18, 2024 · 1 comment · Fixed by #168
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@ValarDragon
Copy link

Currently the Iterator.Key() API makes a copy of the key it gets from the database. This is because the database's iterator returns something it will mutate on the subsequent .Next() call for heap efficiency. This extra copy causes very large heap allocation (and time overheads) to query serving nodes, and a 1% time overhead to the entire state machine time for Osmosis.

On a heap allocation profile of a query serving Osmosis RPC node over an hour, it has 450 gigabytes allocated from this API. On spot-check, none of the big ones need this copying behavior. (160GB removed from a tendermint update, but the remaining 290GB are still from this API)

image

In the state machine, we see 1% of state machine execution time is blocked on copying this key, again in situations where I don't think we need any of this either.
image


Proposal: Add a new method KeyMut() to the interface for Iterator. The caller should not mutate this key, and the expectation is that the key may get mutated on the next .Next() call.

I'm not stoked about the naming of this method, so happy for better ideas

@melekes
Copy link
Contributor

melekes commented Jul 10, 2024

Do we even modify the key in CometBFT? Because the contract says CONTRACT: readonly []byte

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants