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

feat: add hashcache to pbss difflayer #29991

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

will-2012
Copy link

@will-2012 will-2012 commented Jun 13, 2024

Description

In the native transfer test scenario, we found that the bottleneck of pbss trienode reading is the recursive query of the 128-layer difflayer. The current PR is mainly to optimize the pbss trienode reading latency.

Rationale

In difflayer, a new cache map with node hash as key is added. The Node interface gives priority to accessing the cache. If a hit is found, it is returned directly. If it is not found, the disklayer is queried. Compared with the recursive query of the 128-layer difflayer, this cache query is O(1). The original query path is optimized, and most of them are fastpath.
The cache is added when the difflayer is added to the layertree and is released when the difflayer is merged into the disklayer.

Performance

image

Test scenario: 1 million accounts were randomly selected from 25 million accounts, and native transfer transactions were performed at 600qps. The difflayer cache optimization effect was significant, and the validation phase was reduced from 450ms to 100ms.

Changes

Notable changes:

  • Pbss difflayer: add hash cache.

@karalabe
Copy link
Member

karalabe commented Jun 13, 2024

Hmmm, interesting idea with using the hash as the cache key since it can't change, so you don't need to track liveness across layers. Out of curiosity, wouldn't using an existing cache type work instead of defining your own? We use (I think) victoriametrics.

Also, do you have any benchmarks or numbers that you could point us to? Edit: was added later.

@will-2012
Copy link
Author

Hmmm, interesting idea with using the hash as the cache key since it can't change, so you don't need to track liveness across layers. Out of curiosity, wouldn't using an existing cache type work instead of defining your own? We use (I think) victoriametrics.

Also, do you have any benchmarks or numbers that you could point us to? Edit: was added later.

The performance of using fastcache is basically the same, but it takes up extra memory~

@karalabe
Copy link
Member

karalabe commented Jun 13, 2024

Seems your PR has a consensus fault in it, PTAL: https://ci.appveyor.com/project/ethereum/go-ethereum/builds/50010174

@rjl493456442
Copy link
Member

@karalabe this pull request is not compatible with verkle. I guess it might be the reason for the failing test.

@will-2012
Copy link
Author

Seems your PR has a consensus fault in it, PTAL: https://ci.appveyor.com/project/ethereum/go-ethereum/builds/50010174

Fixed~

@rjl493456442
Copy link
Member

Dump some preliminary performance data after benchmarking this branch against master for a few days (~85 hours).

  • This branch is 7hours ahead of the master, roughly 8% performance speedup.
截屏2024-06-18 10 33 33
  • The speedup mostly comes from the accountUpdate which involves trie node retrieval.
截屏2024-06-18 10 36 29
  • EVM execution is slightly slower (5ms per block on average), which might be relevant with some weird side effects

  • Triedb commit is slightly slower (2ms per block) due to the overhead of cache maintenance

I think the idea is valuable and i am very curious how much performance gain we can have by applying it to state snapshot.

@will-2012
Copy link
Author

how much performance gain we can have by applying it to state snapshot.
-->
This is a very good idea, and it may produce good results. It is worth trying. I will try it in the near future, and I will update the results if there are any.

cache: make(map[common.Hash]*RefTrieNode),
}
case *diffLayer:
dl.origin = l.originDiskLayer()
Copy link

Choose a reason for hiding this comment

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

Here you can get the parent origin directly without the read lock, because it's already gotten when it's called.

Copy link
Author

Choose a reason for hiding this comment

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

Semantically, the origin disklayer is mutable, and perhaps the read and write operations of the origin can also be serialized, without the need for a lock guard. But adding a lock guard may be clearer :)

@will-2012
Copy link
Author

how much performance gain we can have by applying it to state snapshot. --> This is a very good idea, and it may produce good results. It is worth trying. I will try it in the near future, and I will update the results if there are any.

At present, adding multi-version cache to snapshot difflayer does not improve performance. The main reasons are as follows:

  1. Snapshot is a flat kv, without the mpt amplification effect; in the test data set, the access volume is about ~20% of the mpt trie;
  2. Snapshot difflayer has a bloomfilter to avoid unnecessary access;
  3. mpt has a large number of internal shared node accesses, and the probability of hitting difflayer is high, about ~60%; snapshot is a flat account kv access, and the probability of hitting difflayer is low, about ~30%.

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.

5 participants