statemanager: cache and other refactors #3569

gabrocheleau · 2024-08-07T18:47:22Z

This reverts some of the changes from #3554 and rebuilds upon them.

This reverts commit d7d1dab.

codecov · 2024-08-09T10:44:50Z

Codecov Report

Attention: Patch coverage is 66.93227% with 83 lines in your changes missing coverage. Please review.

Project coverage is 28.94%. Comparing base (f9788a1) to head (ba00837).
Report is 1 commits behind head on master.

Additional details and impacted files

Flag	Coverage Δ
block	`73.45% <ø> (?)`
blockchain	`85.02% <ø> (?)`
client	`0.00% <0.00%> (ø)`
common	`89.70% <ø> (?)`
evm	`63.39% <ø> (?)`
genesis	`0.00% <ø> (?)`
statemanager	`65.63% <73.75%> (-0.20%)`	⬇️
trie	`52.05% <ø> (?)`
tx	`77.77% <ø> (ø)`
util	`69.89% <ø> (?)`
vm	`58.47% <100.00%> (?)`
wallet	`0.00% <ø> (?)`

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

holgerd77

Looks great so far! 🤩

The one major thing which I would still do structurally differently is that I think we should pass in the cache options directly to the Caches class and - for the state managers - take an initialized Caches object as the option input (and no cache options at all).

The we can remove the default initialization this._caches = new Caches(....) and go with no-cache-as-default.

Default initializations are major tree shaking killers. If we remove, a huge block of code goes away (if not used), so all caching code + the large LRU and js-sdsl dependencies.

It is a bit more to do for users on the UX side (so if caches should be used one needs to pass over: { caches: new Caches() } (or with some options, but I would think that this is very much worth it.

… the sm

gabrocheleau · 2024-08-09T18:56:32Z

Looks great so far! 🤩

The one major thing which I would still do structurally differently is that I think we should pass in the cache options directly to the Caches class and - for the state managers - take an initialized Caches object as the option input (and no cache options at all).

The we can remove the default initialization this._caches = new Caches(....) and go with no-cache-as-default.

Default initializations are major tree shaking killers. If we remove, a huge block of code goes away (if not used), so all caching code + the large LRU and js-sdsl dependencies.

It is a bit more to do for users on the UX side (so if caches should be used one needs to pass over: { caches: new Caches() } (or with some options, but I would think that this is very much worth it.

Just did that!

It did involve some other changes, notably in tests and in the default VM.create() state manager instantiation. I have decided to default to using Caches() (so, same behavior as before) for the default VM as a lot of vm packages tests seem to rely on that.

gabrocheleau · 2024-08-09T19:59:50Z

Made some more progess with making cache optional and fixing tests where necessary.

With regards to your comment:
"I think you can likely simplify many of the above if-causes in a same manner and simply remove." (link)

I'm actually having a broader reflection about the handling of the undefined caches. I'm not a fan of the complex if clauses at the moment, I think the dual checking of undefined and deactivate is confusing.

I don't have a clear full picture right now. We currently instantiate each cache (account, storage, code) with defaults, unless deactivate has been set to true. Now since _caches itself is optional, and all caches are optional on their own, I don't think it's necessary to check against deactivate in usage as we are doing here. Perhaps we still need to keep deactivate as a CacheStateManagerOption, but I think we could/should refrain from using it to check whether or not a particular cache is active.

acolytec3 · 2024-08-09T23:49:37Z

Made some more progess with making cache optional and fixing tests where necessary.

With regards to your comment: "I think you can likely simplify many of the above if-causes in a same manner and simply remove." (link)

I'm actually having a broader reflection about the handling of the undefined caches. I'm not a fan of the complex if clauses at the moment, I think the dual checking of undefined and deactivate is confusing.

I don't have a clear full picture right now. We currently instantiate each cache (account, storage, code) with defaults, unless deactivate has been set to true. Now since _caches itself is optional, and all caches are optional on their own, I don't think it's necessary to check against deactivate in usage as we are doing here. Perhaps we still need to keep deactivate as a CacheStateManagerOption, but I think we could/should refrain from using it to check whether or not a particular cache is active.

If the caches are optional, can't you just do

this.account?.del(key)
this.storage?.del(key)
this.storage?.get(key)

gabrocheleau · 2024-08-10T01:48:13Z

Made some more progess with making cache optional and fixing tests where necessary.
With regards to your comment: "I think you can likely simplify many of the above if-causes in a same manner and simply remove." (link)
I'm actually having a broader reflection about the handling of the undefined caches. I'm not a fan of the complex if clauses at the moment, I think the dual checking of undefined and deactivate is confusing.
I don't have a clear full picture right now. We currently instantiate each cache (account, storage, code) with defaults, unless deactivate has been set to true. Now since _caches itself is optional, and all caches are optional on their own, I don't think it's necessary to check against deactivate in usage as we are doing here. Perhaps we still need to keep deactivate as a CacheStateManagerOption, but I think we could/should refrain from using it to check whether or not a particular cache is active.

If the caches are optional, can't you just do
this.account?.del(key)
this.storage?.del(key)
this.storage?.get(key)

Pretty much what I have ended up doing, yes!

holgerd77 · 2024-08-10T10:00:32Z

Yes, agree

holgerd77 · 2024-08-10T10:02:02Z

packages/client/src/execution/vmexecution.ts

-        size: this.config.codeCache,
-      },
+      caches: new Caches({
+        accountCacheOpts: {


I think we can fully drop the CacheOpts from this key, makes it nicer to digest/read and remains fully expressive.

(same for others of course)

holgerd77 · 2024-08-10T10:04:09Z

packages/statemanager/src/cache/types.ts

+  size: number
+}
+
+export interface CacheStateManagerOpts {


Maybe drop the StateManager here from the name

If we remove StateManager, we'll have a naming conflict with CacheOpts declared above. I am not 100% satisfied with the general naming, and there is some overlap between these interfaces/types (which makes me want to unify them somehow), but there does not seem to be sufficient overlap to really unify them. Open to suggestions!

Maybe give this a second thought, but I would kill off the deactivate part and take the size === 0 as the one and only way to deactivate and then remove the CacheOpts and only use the CachesStateManagerOpts (then renamed to CacheOpts 😋) as the one thing for all.

Interesting, just seeing this comment I'll give that a try, would really like to simplify further

holgerd77 · 2024-08-10T10:05:35Z

packages/statemanager/src/rpcStateManager.ts

-  _storageCache: StorageCache
-  _contractCache: Map<string, Uint8Array>
-
+  protected _accountCache: AccountCache


The caches in RPCStateManager will still be transfered to the new interface, right?

The contractCache seems different in the rpcStateManager than in the others (i.e. not the ContractCache class). It would indeed be nice to refactor in a similar fashion here. @acolytec3, why do we use a custom Map<string, Uint8Array> here rather than the ContractCache class?

I am pretty sure that just some of the inconsistencies we have in state manager and that was rather to have some simple version in and so you can just fully and 1:1 with the original version replace.

I'm 99% sure that is just because the original implementation from the RPCstatemanager just didn't have caches at all. No compelling reason though and the cache will do the same thing.

Just updated to use the same _caches property and type as the other 2

…lities-refactor

…om/ethereumjs/ethereumjs-monorepo into statemanager/capabilities-refactor

acolytec3

LGTM. One TODO to be cleaned up separately

holgerd77 · 2024-08-12T19:26:12Z

🎉

gabrocheleau added 7 commits August 7, 2024 14:46

Revert "statemanager: refactor logic into capabilities (#3554)"

fa3c2ec

This reverts commit d7d1dab.

statemanager: refactor modifyAccountFields

bd1f237

statemanager: adjust return statements

d5aaf30

statemanager: refactor separate caches into caches class

533fa8c

chore: merge with master

31c94af

statemanager: fix shallow copy logic

bba928b

client: fix vmexecution statemanager cache opts

a04158d

gabrocheleau added 2 commits August 9, 2024 07:16

statemanager: refactor some capabilities to caches methods

2431594

statemanager: fix tests

814b9f7

gabrocheleau added PR state: WIP type: refactor breaking package: statemanager PR state: needs review and removed PR state: WIP labels Aug 9, 2024

gabrocheleau marked this pull request as ready for review August 9, 2024 11:19

holgerd77 reviewed Aug 9, 2024

View reviewed changes

gabrocheleau added 4 commits August 9, 2024 14:33

statemanager: refactor caches into optional opt passed in directly to…

2439219

… the sm

statemanager: adjust tests with refactored caches

4c6240f

client: adjust vm execution with update cache

5e1666d

vm: fix vm tests

c754eac

gabrocheleau changed the title ~~statemanager: rework capabilities refactoring~~ statemanager: cache and other refactors Aug 9, 2024

vm: adjust test runners with non-default caches

5d4895c

gabrocheleau added 3 commits August 9, 2024 16:58

statemanager: simplify handling of deactivate

888b20e

statemanager: remove redundant checks

905a52e

statemanager: remove non null asesertion

f30288a

holgerd77 reviewed Aug 10, 2024

View reviewed changes

gabrocheleau and others added 10 commits August 10, 2024 09:45

statemanager: remove cache opt from key naming

ba00837

statemanager: refactor rpc state manager to use caches

c037ae2

statemanager: fix rpc state manager tests

fdcdb36

client: vmexecution cache stats refactor

3d93587

Merge remote-tracking branch 'origin/master' into statemanager/capabi…

48baee3

…lities-refactor

Merge branch 'master' into statemanager/capabilities-refactor

f0b66bf

statemanageR: updategetproof json-rpc call format

5f4c253

statemanager: remove deactivate from caches

7fe21fd

client: remove deactivate from vm execution instantiation

4864383

Merge branch 'statemanager/capabilities-refactor' of https://github.c…

b6ee97b

…om/ethereumjs/ethereumjs-monorepo into statemanager/capabilities-refactor

acolytec3 reviewed Aug 12, 2024

View reviewed changes

acolytec3 approved these changes Aug 12, 2024

View reviewed changes

acolytec3 merged commit 45e0a6d into master Aug 12, 2024
34 checks passed

acolytec3 deleted the statemanager/capabilities-refactor branch August 12, 2024 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

statemanager: cache and other refactors #3569

statemanager: cache and other refactors #3569

gabrocheleau commented Aug 7, 2024

codecov bot commented Aug 9, 2024 •

edited

Loading

holgerd77 left a comment •

edited

Loading

gabrocheleau commented Aug 9, 2024

gabrocheleau commented Aug 9, 2024

acolytec3 commented Aug 9, 2024

gabrocheleau commented Aug 10, 2024

holgerd77 commented Aug 10, 2024

holgerd77 Aug 10, 2024

holgerd77 Aug 10, 2024

gabrocheleau Aug 10, 2024

holgerd77 Aug 10, 2024

gabrocheleau Aug 10, 2024

holgerd77 Aug 10, 2024 •

edited

Loading

gabrocheleau Aug 11, 2024

holgerd77 Aug 10, 2024

gabrocheleau Aug 10, 2024

holgerd77 Aug 10, 2024

acolytec3 Aug 11, 2024

gabrocheleau Aug 11, 2024 •

edited

Loading

acolytec3 left a comment

holgerd77 commented Aug 12, 2024

statemanager: cache and other refactors #3569

statemanager: cache and other refactors #3569

Conversation

gabrocheleau commented Aug 7, 2024

codecov bot commented Aug 9, 2024 • edited Loading

Codecov Report

holgerd77 left a comment • edited Loading

Choose a reason for hiding this comment

gabrocheleau commented Aug 9, 2024

gabrocheleau commented Aug 9, 2024

acolytec3 commented Aug 9, 2024

gabrocheleau commented Aug 10, 2024

holgerd77 commented Aug 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 Aug 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrocheleau Aug 11, 2024 • edited Loading

Choose a reason for hiding this comment

acolytec3 left a comment

Choose a reason for hiding this comment

holgerd77 commented Aug 12, 2024

codecov bot commented Aug 9, 2024 •

edited

Loading

holgerd77 left a comment •

edited

Loading

holgerd77 Aug 10, 2024 •

edited

Loading

gabrocheleau Aug 11, 2024 •

edited

Loading