fix: add cache to snapshot and re-cache irregular states #4819

Wodann · 2024-02-03T00:20:33Z

No description provided.

changeset-bot · 2024-02-03T00:20:37Z

⚠️ No Changeset found

Latest commit: b21b1ec

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel · 2024-02-03T00:20:39Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
hardhat	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 5, 2024 8:21pm

Wodann · 2024-02-03T00:22:23Z

crates/edr_provider/src/data.rs

@@ -948,6 +944,7 @@ impl<LoggerErrorT: Debug> ProviderData<LoggerErrorT> {

        let snapshot = Snapshot {
            block_number: self.blockchain.last_block_number(),
+            block_state_cache: self.block_state_cache.clone(),


Add the cache to the snapshot to ensure that people can revert to previous snapshots (excluding newer cache entries).

Wodann · 2024-02-03T00:23:02Z

crates/edr_provider/src/data.rs

-            // We cannot use `LruCache::try_get_or_insert`, because it needs &mut self, but
-            // we would need &self in the callback to construct the context.
-            if !self.block_context_cache.contains(&block_number) {
+        let block = if let Some(block_spec) = block_spec {


I had to collapse the two functions into one to make the borrow checker happy with the Cow<'state, _>'s 'state lifetime

Wodann · 2024-02-03T00:30:52Z

crates/edr_provider/src/data.rs

+            let block_number = block_header.number;
+
+            // Avoid caching of states that may contain state overrides
+            let can_cache = self.irregular_state.state_overrides().is_empty();


The original caching strategy wasn't taking into consideration the possibility of having irregular state changes to a block. If you made such changes, it would still return previously cached versions of the block.

My approach is too conservative and results in performance degradation for certain tests. I didn't have time before end-of-day to make this criteria more flexible though.

This function shows how irregular state overrides affect the construction of a block.

There are 3 approaches to consider and experiment with:

Update the cached entry when an irregular state change happens

Instead of just caching per block number, we can also incorporate the irregular state overrides that would impact the requested block number (everything up until this block number). I'm not sure how expensive it would be to validate this; e.g. using a hash; but you could experiment with this.

Even if we never cache irregular state, we can make this condition less strict by looking whether there are any state overrides up until the requested block number.

Thanks for the detailed explanation! I think something else might be going on here as well regarding the performance degradation. Running the Rocketpool scenario on my machine is almost 10x slower with this change than in the base branch, and Rocketpool doesn't use any hardhat_* methods which as I understand is the only way to set irregular state.

I created a flame graph using the Rockepool scenario on which two things stand out:

Most of the time is spent in compute_state_at_block

Dropping a snapshot takes significant time

I cherry picked this commit in the edr/save-scenarios branch so you can run the Rocketpool scenario yourself (after downloading it from the shared Google Drive) from the crates/tools directory:

cargo run --release scenario ../../benchmark-scenarios/rocketpool-6a9dbfd8/06549782-8dc5-4700-bdde-9457cdd7b388

And to generate a flame graph:

cargo install flamegraph

CARGO_PROFILE_RELEASE_DEBUG=true cargo flamegraph --release -- scenario ../../benchmark-scenarios/rocketpool-6a9dbfd8/06549782-8dc5-4700-bdde-9457cdd7b388

Make sure to ignore the part of the flame graph that loads the scenario files. This is easily identifiable as it uses rayon, not tokio.

I think something else might be going on here as well regarding the performance degradation. Running the Rocketpool scenario on my machine is almost 10x slower with this change than in the base branch, and Rocketpool doesn't use any hardhat_* methods which as I understand is the only way to set irregular state.

I'd need to look at the specific scenarios, but when forking a remote network with a configuration that has "genesis accounts", you're always creating an irregular state override for those, which is applied to that first block.

Additionally, we're always at least calculating the block state's once, even when using latest. This can be avoided by caching when we change the state.

I'm having a look at the scenario you highlighted to see whether either of these is the case and make the caching more aggressive.

I made two optimisations:

I ensure that we always cache changes to a block, such that we can always use the latest version of the requested block number. This is the same behaviour as generating a state using `state_at_block_number.

I use a custom global allocator that retains memory pages such that we don't always have to go through the system allocator.

agostbiro

Thanks for jumping on this @Wodann I left some comments, but looks like the right direction.

agostbiro · 2024-02-05T11:23:07Z

crates/edr_provider/src/data.rs

+            let contextual_state = if can_cache && self.block_state_cache.contains(&block_number) {
+                // We cannot use `LruCache::try_get_or_insert`, because it needs &mut self, but
+                // we would need &self in the callback to construct the context.
+                Cow::Borrowed(self.block_state_cache.get(&block_number).expect(


I think the expect invariant doesn't hold anymore as the push is after the get in this change

There is a contains check (previously it was the inverse) which guarantees that the get will succeed.

agostbiro · 2024-02-05T11:37:25Z

crates/edr_provider/src/data.rs

+            let block_number = block_header.number;
+
+            // Avoid caching of states that may contain state overrides
+            let can_cache = self.irregular_state.state_overrides().is_empty();


Thanks for the detailed explanation! I think something else might be going on here as well regarding the performance degradation. Running the Rocketpool scenario on my machine is almost 10x slower with this change than in the base branch, and Rocketpool doesn't use any hardhat_* methods which as I understand is the only way to set irregular state.

I created a flame graph using the Rockepool scenario on which two things stand out:

Most of the time is spent in compute_state_at_block

Dropping a snapshot takes significant time

I cherry picked this commit in the edr/save-scenarios branch so you can run the Rocketpool scenario yourself (after downloading it from the shared Google Drive) from the crates/tools directory:

cargo run --release scenario ../../benchmark-scenarios/rocketpool-6a9dbfd8/06549782-8dc5-4700-bdde-9457cdd7b388

And to generate a flame graph:

cargo install flamegraph

CARGO_PROFILE_RELEASE_DEBUG=true cargo flamegraph --release -- scenario ../../benchmark-scenarios/rocketpool-6a9dbfd8/06549782-8dc5-4700-bdde-9457cdd7b388

Make sure to ignore the part of the flame graph that loads the scenario files. This is easily identifiable as it uses rayon, not tokio.

Wodann · 2024-02-05T20:57:36Z

crates/edr_provider/src/data.rs

        self.irregular_state
            .state_override_at_block_number(block_number)
            .or_insert_with(|| StateOverride::with_state_root(state_root))
            .diff
            .apply_account_change(address, account_info.clone());

-        self.mem_pool.update(&self.state)?;
+        self.block_state_cache.push(block_number, state.clone());
+        self.state = state;


This ensures that the state is not modified when there are previous failures in fn state_root or fn update calls.

This occurs at the cost of an extra clone. Once we adopt an immutable Merkle-Patricia trie, this cost should become negligible.

agostbiro · 2024-02-07T15:52:51Z

Thanks @Wodann , it all looks good, but there is a segfault in the MacOS build: https://github.com/NomicFoundation/hardhat/actions/runs/7790363699/job/21244112219

It's probably due to the custom allocator. I'll cherry pick your commits to the base branch and I'll try to remove the cache from the snapshot as that seems to be the biggest performance hit now.

agostbiro

Thanks for removing the custom allocator!

fix: add cache to snapshot and don't cache irregular states

3bfe021

Wodann added the area:edr label Feb 3, 2024

Wodann requested a review from agostbiro February 3, 2024 00:20

Wodann self-assigned this Feb 3, 2024

github-actions bot added the status:triaging label Feb 3, 2024

vercel bot deployed to Preview February 3, 2024 00:21 View deployment

Wodann removed the status:triaging label Feb 3, 2024

Wodann commented Feb 3, 2024

View reviewed changes

Wodann mentioned this pull request Feb 3, 2024

fix: cache block contexts #4815

Merged

agostbiro requested changes Feb 5, 2024

View reviewed changes

feat: more aggressive caching by always storing latest modified state

b21b1ec

vercel bot deployed to Preview February 5, 2024 17:18 View deployment

Wodann changed the title ~~fix: add cache to snapshot and don't cache irregular states~~ fix: add cache to snapshot and re-cache irregular states Feb 5, 2024

vercel bot deployed to Preview February 5, 2024 20:21 View deployment

Wodann requested a review from agostbiro February 5, 2024 20:53

Wodann commented Feb 5, 2024

View reviewed changes

Wodann force-pushed the edr/fix/cache-block-contexts2 branch from 5eb5b62 to b21b1ec Compare February 7, 2024 17:18

agostbiro approved these changes Feb 8, 2024

View reviewed changes

agostbiro merged commit 28064d8 into edr/fix/cache-block-contexts Feb 8, 2024
95 of 96 checks passed

agostbiro deleted the edr/fix/cache-block-contexts2 branch February 8, 2024 06:17

github-actions bot locked as resolved and limited conversation to collaborators May 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add cache to snapshot and re-cache irregular states #4819

fix: add cache to snapshot and re-cache irregular states #4819

Wodann commented Feb 3, 2024

changeset-bot bot commented Feb 3, 2024 •

edited

Loading

vercel bot commented Feb 3, 2024 •

edited

Loading

Wodann Feb 3, 2024 •

edited

Loading

Wodann Feb 3, 2024 •

edited

Loading

Wodann Feb 3, 2024 •

edited

Loading

agostbiro Feb 5, 2024

Wodann Feb 5, 2024

Wodann Feb 5, 2024

agostbiro left a comment

agostbiro Feb 5, 2024

Wodann Feb 5, 2024

agostbiro Feb 5, 2024

Wodann Feb 5, 2024

agostbiro commented Feb 7, 2024

agostbiro left a comment

fix: add cache to snapshot and re-cache irregular states #4819

fix: add cache to snapshot and re-cache irregular states #4819

Conversation

Wodann commented Feb 3, 2024

changeset-bot bot commented Feb 3, 2024 • edited Loading

⚠️ No Changeset found

vercel bot commented Feb 3, 2024 • edited Loading

Wodann Feb 3, 2024 • edited Loading

Choose a reason for hiding this comment

Wodann Feb 3, 2024 • edited Loading

Choose a reason for hiding this comment

Wodann Feb 3, 2024 • edited Loading

Choose a reason for hiding this comment

agostbiro Feb 5, 2024

Choose a reason for hiding this comment

Wodann Feb 5, 2024

Choose a reason for hiding this comment

Wodann Feb 5, 2024

Choose a reason for hiding this comment

agostbiro left a comment

Choose a reason for hiding this comment

agostbiro Feb 5, 2024

Choose a reason for hiding this comment

Wodann Feb 5, 2024

Choose a reason for hiding this comment

agostbiro Feb 5, 2024

Choose a reason for hiding this comment

Wodann Feb 5, 2024

Choose a reason for hiding this comment

agostbiro commented Feb 7, 2024

agostbiro left a comment

Choose a reason for hiding this comment

changeset-bot bot commented Feb 3, 2024 •

edited

Loading

vercel bot commented Feb 3, 2024 •

edited

Loading

Wodann Feb 3, 2024 •

edited

Loading

Wodann Feb 3, 2024 •

edited

Loading

Wodann Feb 3, 2024 •

edited

Loading