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

perf(evm): only store changed storage entries #3187

Closed
wants to merge 2 commits into from

Conversation

mattsse
Copy link
Member

@mattsse mattsse commented Sep 12, 2022

Blocked by #3183

Motivation

only clone contract storage for step tracer
the storage entry in the geth StructLog is only the state of the contract, Ref https://docs.dcfsscan.io/docs/dapp/tracing

Therefor it's not necessary to clone the entire state. instead clone only the storage of the contract.

@shekhirin I think this is a significant factor for the recent mem issues

Solution

@mattsse mattsse marked this pull request as draft September 12, 2022 20:03
@mattsse mattsse added the T-perf Type: performance label Sep 12, 2022
@mattsse mattsse force-pushed the matt/fix-state-storage branch from 83b0942 to 8a1721b Compare September 12, 2022 20:40
@mattsse mattsse changed the title Matt/fix state storage perf(evm): only store changed storage entries Sep 12, 2022
@shekhirin
Copy link
Contributor

shekhirin commented Sep 12, 2022

yes, I think you're right. This change will basically act as a diff on each step for storage which I was talking about, and that's also the way it's implemented in geth. Thank you!

I have two things to consider but I may be incorrect:

  1. Since we're making a state snapshot in the step(), we've not yet executed the SLOAD/SSTORE opcodes, therefore there's no storage change made. I see geth being a bit different in that behaviour: they capture the change instantly https://github.com/ethereum/go-ethereum/blob/3a4cef5402d8b58831f95a909f8a1bdf3acffe3c/eth/tracers/logger/logger.go#L190-L205.
  2. We can also do the same thing with checking opcodes for SLOAD/SSTORE as geth does, so we wouldn't loop through the storage without a need.

@mattsse
Copy link
Member Author

mattsse commented Sep 12, 2022

I see, hmm how do we get the value during step?

not sure I can follow. most of the work is in #3183, I merely changed the state field's type and added the filter in this PR.

Would you like to add a fix for your 2nd point and I can just close this?

@shekhirin
Copy link
Contributor

hmm how do we get the value during step?

I think we need to fill the storage in step_end(), so we could capture changed slots.

I merely changed the state field's type and added the filter in this PR.

Sorry, I'm talking about further optimization which is checking for SLOAD/SSTORE opcodes in step_end() and only in these cases iterating through the storage. I think what you've done is fine and will work!

@shekhirin
Copy link
Contributor

something like that 5281088

@mattsse
Copy link
Member Author

mattsse commented Sep 13, 2022

something like that 5281088

that looks great, let's merge this instead!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-perf Type: performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants