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

Implement a more performant code cache #3022

Merged
merged 40 commits into from
Sep 29, 2023
Merged

Implement a more performant code cache #3022

merged 40 commits into from
Sep 29, 2023

Conversation

scorbajio
Copy link
Contributor

@scorbajio scorbajio commented Sep 11, 2023

This PR looks into implementing a more performant code cache for the statemanager. Also see #3021.

Some followup work that is left to do after this change is merged:

  • Refactor the statemanager caches to reduce code redundancies
  • Add an option to cli for enabling/disabling the code cache, similar to the other caches
  • Integrate the code cache with EthersStateManager

@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Merging #3022 (4f03988) into master (4d5ce61) will increase coverage by 0.07%.
The diff coverage is 90.93%.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 88.78% <ø> (ø)
blockchain 92.61% <ø> (ø)
client 87.70% <100.00%> (+0.02%) ⬆️
common 98.19% <ø> (ø)
ethash ∅ <ø> (∅)
evm 71.87% <ø> (ø)
rlp ∅ <ø> (∅)
statemanager 90.23% <90.27%> (+0.05%) ⬆️
trie 90.59% <ø> (+0.54%) ⬆️
tx 96.36% <ø> (ø)
util 86.97% <ø> (ø)
vm 76.21% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@jochem-brouwer
Copy link
Member

This commit: 105403a should fix (some?) of the failing tests.

I have tried to merge master (this had merge conflicts) - not sure if I solved those correctly (if not, rollback to 105403a)

@jochem-brouwer
Copy link
Member

Reverted back to old state due to too many tests failing (note: CI run is thus not up to date with current state since it will not re-run due to merge conflicts)

@holgerd77
Copy link
Member

Cool, but this looks great in general! 🤩

@holgerd77
Copy link
Member

Ok, doing a few client runs here:

  1. npm run client:start -- --sync=none --transports --executeBlocks=1600000-1600100
    First: works at all, that's great (mean this totally unironically, since for me a lot of things still broke along client runs even I had the tests going (for the other-caches-implementation)! 🤩
    Time:(for such a short range period there should be no difference I would assume?)
    Seconds for every repeated run: 32 25 24 (often a first run is slower, wonder if this is because Node is then caching some code or whatever? This is a very general observation I have repeatedly)

Oh.

But the code cache is also not building up any size. 🤔 If I inject console.log(this._codeCache?.size()) in StateManager getContractCode(), this is always 0.

I guess there is still something missing on initialization?

Will submit here and have a look.

@holgerd77
Copy link
Member

Have updated the branch via UI

@holgerd77
Copy link
Member

Update: ah, I think this is due to the exact same thing which I stumbled upon and am solving here #3063 , so that the --executeBlocks option in client is not properly initialized with the caches.

Will wait until this is merged and then update the branch here and then try again (testing this without the --executeBlocks option is basically not possible, since one then cannot get repeated runs on the same (higher-number) block ranges).

@holgerd77
Copy link
Member

Ah, and then we'll also still need to see if the cache "holds" for live-higher-range-client blocks.

This is always an exciting moment. 🙂 😋
(I would guess 50-50 chance from my own experiences...)

@holgerd77
Copy link
Member

Ok, I now updated this branch with the changes from the PR (remember to pull your local branch), will now test again. 🙂

@holgerd77
Copy link
Member

Just so that I do not forget:

There now needs to be a downlevelCaches check in the following code part in StateManager.shallowCopy() (as being done for the other caches):

if (!this._codeCacheSettings.deactivate) {
      codeCacheOpts = { ...codeCacheOpts, type: CacheType.ORDERED_MAP }
    }

Generally code cache is not active yet for the --executeBlocks flag still (also with the above change), will have a closer look.

@holgerd77
Copy link
Member

Haha, it was because the size() method is not complete regarding ORDERED_MAP 😋:

size(): number {
    if (this._lruCache) {
      return this._lruCache.size
    }
    return 0
  }

Please update, maybe you can have another general look if all methods work for both cache types.

@holgerd77
Copy link
Member

Ok, great, then for the above example cache grows to 246.

And I have got 588 reads from cache, 4027 from DB.

Interesting, that's something, already for such a relatively small block range.

Needs to stop, unfortunately can't do any more performance comparisons, but will continue to experiment tomorrow a bit.

@holgerd77
Copy link
Member

Updated this via UI

@holgerd77
Copy link
Member

Some more performance results, if several values this is for consecutive repetitions of runs:

npm run client:start -- --sync=none --executeBlocks=1600000-1600100 (100 blocks)
Seconds (total): 24 24 24

npm run client:start -- --sync=none --executeBlocks=1600000-1600100 --codeCache=0
25 24 25

npm run client:start -- --sync=none --executeBlocks=1600000-1601000 (1000 blocks)
Minutes (total): 2:07 1:55 1:54

npm run client:start -- --sync=none --executeBlocks=1600000-1601000 --codeCache=0
Minutes (total): 1:55 1:54. 1:57

Ok, this for the most part matches. One outlier with this 2:07, but this can very well be the first run.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants