Skip to content

core/vm: avoid unnecessary stack memory copies during gas calc#15561

Closed
karalabe wants to merge 1 commit into
ethereum:masterfrom
karalabe:remove-unnecessary-stack-copies
Closed

core/vm: avoid unnecessary stack memory copies during gas calc#15561
karalabe wants to merge 1 commit into
ethereum:masterfrom
karalabe:remove-unnecessary-stack-copies

Conversation

@karalabe
Copy link
Copy Markdown
Member

This PR removes two batches of unnecessary memory allocations and copies with regard to EVM stack and gas calculation:

  • Whenever a CALL* opcode is executed, the gas allowance on the EVM stack is replaced according to the 63/64 fork rule. I don't see a reason to allocate a new big.Int for the new stack item. If we're overwriting anyway, we can overwrite the existing content.
  • Due to modifying the stack for CALL* opcodes, the EVM tracer currently copied the entire stack for every single opcode. This is insanely wasteful since we're only changing 1 value very rarely, and the tracer copied all values all the time to handle it. The PR contains a very very selective logic for saving the original stack item only if it will actually change. This saves us about 27% of tracing time.

@karalabe karalabe requested a review from holiman as a code owner November 27, 2017 12:51
@karalabe karalabe added this to the 1.8.0 milestone Nov 27, 2017
@fjl
Copy link
Copy Markdown
Contributor

fjl commented Nov 27, 2017

The copy was introduced for trace compatibility in #15035. I already complained there that this is a super annoying hack. This PR makes the hack more complicated.

The real solution is to add a struct field, e.g. callGas, that will hold the gas value calculated according to the 63/64 rule. This would eliminate the need to track this value on the stack. The copy could then be removed entirely.

@holiman
Copy link
Copy Markdown
Contributor

holiman commented Nov 27, 2017

The real solution is to add a struct field, e.g. callGas that will hold the gas value calculate according to the 63/64 rule.

I agree, but I also thinks that this change -- which only touches code within in.cfg.Debug- scopes -- is a lot less critical...

@fjl
Copy link
Copy Markdown
Contributor

fjl commented Nov 27, 2017

#15563 implements my suggestion.

@karalabe
Copy link
Copy Markdown
Member Author

Closing in favor of #15563.

@karalabe karalabe closed this Nov 28, 2017
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.

3 participants