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

Reduce TrieNode allocations #5457

Merged
merged 7 commits into from
Mar 22, 2023
Merged

Reduce TrieNode allocations #5457

merged 7 commits into from
Mar 22, 2023

Conversation

Scooletz
Copy link
Contributor

@Scooletz Scooletz commented Mar 20, 2023

This PR removes 10% managed allocations from the node. It does it by moving StorageRoot from TrieNode as an item of an array and getting rid of RlpStream from TrieNode and delegating its functionality to Rlp.ValueDecoderContext

Tests

Benchmarks

Reused PatriciaTreeBenchmarks that provides a coarse grained measurement of various operations.

BenchmarkDotNet=v0.13.5, OS=Windows 11 (10.0.22621.1413/22H2/2022Update/SunValley2)
12th Gen Intel Core i9-12900HK, 1 CPU, 20 logical and 14 physical cores
.NET SDK=7.0.100
  [Host]     : .NET 7.0.0 (7.0.22.51805), X64 RyuJIT AVX2
  DefaultJob : .NET 7.0.0 (7.0.22.51805), X64 RyuJIT AVX2
Method Mean Error StdDev Median Gen0 Gen1 Allocated
master 245.4 us 8.13 us 23.84 us 254.4 us 12.4512 3.1738 155.43 KB
this PR 181.5 us 9.02 us 26.59 us 193.7 us 10.2539 2.6855 126.38 KB

Running node

Version Memory allocation - min Memory allocation - max Memory allocation - avg New Payload Execution Time - Mean New Payload Execution Time - Stdev
master 4.11 MB/s 21.8 MB/s 5.46 MB/s 891 496
this PR 3.90 MB/s 12.5 MB/s 4.80 MB/s 779 408
**Image** comparing of all the metrics

image

Changes

  • StorageRoot is now stored as _data[2] only in leafs
  • RlpStreamReader used instead of the object of RlpStream

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Remarks

Another approach to the TrieNode optimization.

@Scooletz Scooletz added the trie label Mar 20, 2023
@Scooletz Scooletz mentioned this pull request Mar 20, 2023
12 tasks
@Scooletz Scooletz changed the title Trie node optimization Reduce TrieNode allocations Mar 21, 2023
@Scooletz
Copy link
Contributor Author

Maybe I can change the implementation to use Rlp.ValueDecoderContext?


public byte[]? Data { get; }

public int Position { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to micro op, I guess you can set this to a byte. Or it does not matter and overhead of casting it is higher.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll leave as is. The stack is the stack one int or byte wont' make that much change. Also, I'll default to Rlp.ValueContext for now.

@Scooletz
Copy link
Contributor Author

Doing a full sync on teku mainnet. Once it's synced and working will confirm and merge if the results are consistent.

@Scooletz Scooletz marked this pull request as ready for review March 21, 2023 15:05
@Scooletz
Copy link
Contributor Author

The teku mainnet has been synced with this and now is operational. Waiting for a few hours of running and will be merging if nothing contradicts the findings above.

@Scooletz
Copy link
Contributor Author

I confirm that with the usage of the Rlp context reading the version behaves as it was with the copied RlpStreamReader, reducing the measured alloc ratio by at least 10%. Merging.

@Scooletz Scooletz merged commit 5488e31 into master Mar 22, 2023
@Scooletz Scooletz deleted the trie-node-optimization branch March 22, 2023 08:14
kamilchodola added a commit that referenced this pull request Apr 3, 2023
kamilchodola added a commit that referenced this pull request Apr 3, 2023
This reverts commit 5488e31.

Co-authored-by: Kamil Chodoła <kamil@nethermind.io>
tanishqjasoria pushed a commit that referenced this pull request Apr 4, 2023
* _storageRoot as an array member

* RlpStreamReader introduced

* fixes and tests

* benchmark refactored a bit

* format

* minor adjustements

* the reader removed and usages replaced with Rlp.ValueDecoderContext
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants