perf(txindex): Lower allocation overhead of txIndex matchRange by ValarDragon · Pull Request #2839 · cometbft/cometbft

ValarDragon · 2024-04-18T05:42:52Z

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

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 - All existing tests still apply
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments
Title follows the Conventional Commits spec

melekes

Thanks @ValarDragon ❤️

melekes · 2024-04-18T06:01:13Z

state/txindex/kv/kv.go

 LOOP:
 	for ; it.Valid(); it.Next() {
-		if !isTagKey(it.Key()) {
+		// TODO: We need to make a function for getting it.Key() as a byte slice with no copies.


Would you mind opening an issue for this?

Sure! Do you want me to delete the comment as well?

cometbft-db issue opened: cometbft/cometbft-db#156

Sure! Do you want me to delete the comment as well?

no

ValarDragon · 2024-04-18T11:49:27Z

Sorry for adding another commit after review 😅 . I noticed in the heap profile another source of memory overhead. Its much much lower in magnitude, but the fix was easy so figured I'd still add it before doing a second round of post-fix re-benchmarking.

This commit removes the Key call within setTmpHashes and most allocations from ExtractHeightFromKey, yielding another ~5GB of allocation reduction

cason

Thanks for this.

I couldn't find (no test performed, only by reading) operational differences, while the changes probably reduce the memory footprint.

cason · 2024-04-23T16:10:28Z

state/txindex/kv/kv.go

+		return 0, errors.New("second last separator not found")
+	}

-	return strconv.ParseInt(parts[len(parts)-2], 10, 64)


How did this work until now?

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. --- #### PR checklist - [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 (cherry picked from commit e75267f)

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. --- #### PR checklist - [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 (cherry picked from commit e75267f) # Conflicts: # state/txindex/kv/kv.go

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. --- #### PR checklist - [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 (cherry picked from commit e75267f) # Conflicts: # .changelog/v0.37.5/improvements/2839-tx_index-lower-heap-allocation.md # state/txindex/kv/kv.go

…ort #2839) (#2882) 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. --- #### PR checklist - [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 #2839 done by [Mergify](https://mergify.com). Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

…ort #2839) (#2883) 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. --- #### PR checklist - [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 #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>

…ort #2839) (#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. --- #### PR checklist - [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 #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>

…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](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>

#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>

#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)

#27) (#31) * 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) Co-authored-by: Adam Tucker <adam@osmosis.team>

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

Lower allocation overhead of txIndex matchRange

00c2450

ValarDragon requested a review from a team as a code owner April 18, 2024 05:42

ValarDragon requested a review from a team April 18, 2024 05:42

Add changelog

8af2e86

melekes approved these changes Apr 18, 2024

View reviewed changes

melekes added backport-to-v0.37.x backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x labels Apr 18, 2024

adizere added this to the 2024-Q2 milestone Apr 18, 2024

cason added storage P:storage-optimization Priority: Give operators greater control over storage and storage optimization labels Apr 18, 2024

Lower allocations of setMatchRange and extractHeightFromKey

d8d921d

cason approved these changes Apr 23, 2024

View reviewed changes

melekes added this pull request to the merge queue Apr 24, 2024

Merged via the queue into cometbft:main with commit e75267f Apr 24, 2024

mergify bot mentioned this pull request Apr 24, 2024

perf(txindex): Lower allocation overhead of txIndex matchRange (backport #2839) #2882

Merged

4 tasks

This was referenced Apr 24, 2024

perf(txindex): Lower allocation overhead of txIndex matchRange (backport #2839) #2883

Merged

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

Merged

czarcas7ic mentioned this pull request Apr 29, 2024

perf(txindex): Lower allocation overhead of txIndex matchRange (backp… osmosis-labs/cometbft#27

Merged

7 tasks

mergify bot mentioned this pull request Apr 30, 2024

perf(txindex): Lower allocation overhead of txIndex matchRange (backp… (backport #27) osmosis-labs/cometbft#31

Merged

7 tasks

faddat mentioned this pull request Oct 15, 2024

chore: use latest cometbft-db in v0.38.x #4296

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(txindex): Lower allocation overhead of txIndex matchRange#2839

perf(txindex): Lower allocation overhead of txIndex matchRange#2839
melekes merged 3 commits intocometbft:mainfrom
osmosis-labs:dev/lower_allocation_of_match_range

ValarDragon commented Apr 18, 2024 •

edited

Loading

Uh oh!

melekes left a comment

Uh oh!

melekes Apr 18, 2024

Uh oh!

ValarDragon Apr 18, 2024

Uh oh!

ValarDragon Apr 18, 2024

Uh oh!

melekes Apr 18, 2024

Uh oh!

ValarDragon commented Apr 18, 2024

Uh oh!

cason left a comment

Uh oh!

cason Apr 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ValarDragon commented Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

melekes Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

ValarDragon Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

ValarDragon Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

melekes Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

ValarDragon commented Apr 18, 2024

Uh oh!

cason left a comment

Choose a reason for hiding this comment

Uh oh!

cason Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ValarDragon commented Apr 18, 2024 •

edited

Loading