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

perf(txindex): Lower allocation overhead of txIndex matchRange (backp… (backport #27) #31

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Apr 30, 2024

…ort cometbft#2839) (cometbft#2884)

In Osmosis we see massive amounts of heap pressure/allocations coming from txIndex matchRange. (Screenshot below from ~1 hour of heap profiling)

image

This PR is expected to fully compatibly drop this down by a factor of 3. It:

  • Does not get Key() twice (160GB allocation saved)
  • Uses no heap allocations for isTagKey (120GB saved)
  • Does not string cast or do strings.Split in parsing the value (~400GB expected saved)
  • Reuses the big.Int (24GB saved)

The remaining RAM overhead from .Key() needs a cometbft-db API change. The remaining RAM overhead from extracting the value can be saved with an unsafe call for casting the output to string with no heap allocation, but we can do that in a separate PR.




PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments

This is an automatic backport of pull request #27 done by [Mergify](https://mergify.com).

#27)

* perf(txindex): Lower allocation overhead of txIndex matchRange (backport cometbft#2839) (cometbft#2884)

In Osmosis we see massive amounts of heap pressure/allocations coming
from txIndex matchRange. (Screenshot below from ~1 hour of heap
profiling)

![image](https://github.com/cometbft/cometbft/assets/6440154/bf2dfe89-56f0-4824-815b-c5822d20568b)

This PR is expected to fully compatibly drop this down by a factor of 3.
It:
- Does not get Key() twice (160GB allocation saved)
- Uses no heap allocations for isTagKey (120GB saved)
- Does not string cast or do strings.Split in parsing the value (~400GB
expected saved)
- Reuses the big.Int (24GB saved)

The remaining RAM overhead from .Key() needs a cometbft-db API change.
The remaining RAM overhead from extracting the value can be saved with
an unsafe call for casting the output to string with no heap allocation,
but we can do that in a separate PR.

---

- [x] Tests written/updated - All existing tests still apply
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request cometbft#2839 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

* add changelog

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
(cherry picked from commit efd1ea2)
@czarcas7ic czarcas7ic merged commit f9bf103 into osmo-v24/v0.37.4 Apr 30, 2024
16 checks passed
@mergify mergify bot deleted the mergify/bp/osmo-v24/v0.37.4/pr-27 branch April 30, 2024 15:27
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.

1 participant