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

Evm optimisations #5877

Merged
merged 4 commits into from
Jul 6, 2023
Merged

Evm optimisations #5877

merged 4 commits into from
Jul 6, 2023

Conversation

benaadams
Copy link
Member

@benaadams benaadams commented Jul 1, 2023

Changes

  • Unconditionally reduce gas and then check if out of gas at end of loop to reduce method size
  • Skip unnecessary conversions to UInt256 and operations on that type when can be done on smaller native type (e.g. ulong)
  • Faster Vector256 clears rather than Span.Slice.Clear()

image

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

@benaadams benaadams added the minor label Jul 3, 2023
@benaadams benaadams changed the title Reduce in-loop code gen from gas calcs Evm optimisations Jul 3, 2023
src/Nethermind/Nethermind.Evm/EvmPooledMemory.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Evm/EvmPooledMemory.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Evm/EvmStack.cs Outdated Show resolved Hide resolved
@@ -2066,6 +2060,10 @@ private CallResult ExecuteCall<TTracingInstructions>(EvmState vmState, byte[]? p
}
}

if (gasAvailable < 0)
Copy link
Member

Choose a reason for hiding this comment

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

What about underflow?

Copy link
Member Author

@benaadams benaadams Jul 3, 2023

Choose a reason for hiding this comment

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

Don't think any operation can charge 9.2 * 10^18 gas?

Copy link
Member Author

Choose a reason for hiding this comment

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

Max would be SHA3 at 1.3 × 10^10 (2^31*6+30) for a 2 GiB SHA3 op

Other methods like create; sstore etc have their own checks

Copy link
Member

Choose a reason for hiding this comment

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

Also, it could potentially change the workflow and traces? You are manipulating the stack now.

Copy link
Member Author

@benaadams benaadams Jul 6, 2023

Choose a reason for hiding this comment

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

Out of gas doesn't report stack; only changes I can see is it might report InvalidJumpDestination or AccessViolation as the error instead of OutOfGas for a couple instructions as it now checks those conditions first; though would fail at the same instruction

Copy link
Member Author

Choose a reason for hiding this comment

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

That could be seen as more helpful as if you increased the gas, its still going to fail 🤷‍♂️

@LukaszRozmej
Copy link
Member

Hive tests needed too.

@MarekM25 MarekM25 requested a review from Demuirgos July 3, 2023 10:40
@benaadams
Copy link
Member Author

Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

Overall looks good, not sure about if it should be like that when tracing.

@Demuirgos @deffrian ?

@benaadams
Copy link
Member Author

Overall looks good, not sure about if it should be like that when tracing.

Should be the same, does the outofgas test prior to EndInstructionTrace

@benaadams benaadams merged commit e65406a into master Jul 6, 2023
126 of 130 checks passed
@benaadams benaadams deleted the gas-calcs branch July 6, 2023 15:28
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.

None yet

2 participants