-
Notifications
You must be signed in to change notification settings - Fork 810
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
Reduce UInt256 in EVMOperations #5331
Reduce UInt256 in EVMOperations #5331
Conversation
Reduce the use of UInt256 in operations, replacing them with Bytes as appropriate. Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
|
…6-in-operations Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Benchmark results - https://docs.google.com/spreadsheets/d/1qbvlAveoLCiWbhbxouHgcpFeDVjws7UD0dgZghJZ6oU/edit?usp=sharing Macbook Pro M1 MAX Top line result - 10-15% "all in" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few comments on parts I don't understand
overall I think it's fine
performance improvement sounds fab
think a second opinion would be good :)
.thenReturn(UInt256.fromBytes(Bytes32.fromHexStringLenient(shift))) | ||
.thenReturn(UInt256.fromHexString(number)); | ||
.thenReturn(Bytes32.fromHexStringLenient(shift)) | ||
.thenReturn(Bytes.fromHexString(number)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not Bytes32 everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's something worth benchmarking, but the number of times we need to take a 256 bit number and consider it as a 8 or 32 bit number are very high (memory access, shifts, etc). So my first pass was trimmed bytes, where we can assess size much quicker.
I'll look into Bytes32 or possibly ByteBuffer integrations in a future pass. It did cross my mind that consistently sized words may improve garbage collection implications and (by extension) speed.
private static Bytes getByte(final Bytes seq, final Bytes offset) { | ||
Bytes trimmedOffset = offset.trimLeadingZeros(); | ||
if (trimmedOffset.size() > 1) { | ||
return Bytes.EMPTY; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the cases where return UInt256.ZERO has changed to Bytes.EMPTY - I'm not an EVM expert - there is already test coverage for this? no side effects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Semantically they are the same on the stack, it's quicker to get an int or byte zero out of a Bytes.EMPTY than a UInt256.ZERO (UInt256 would have to check 7 ints as zero before returning the last one, and possibly bit checking for size).
The tests that detected the Tuweni 3.2 constant values bug (where Bytes32.ZERO couldn't be trimmed to 20 bytes in length) would trip this if it was a problem.
evm/src/main/java/org/hyperledger/besu/evm/operation/SDivOperation.java
Outdated
Show resolved
Hide resolved
@shemnon should this get a changelog entry? |
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
…6-in-operations Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the slicing is a bit scary tbh, but happy to see evm perf improvements. This PR would be a good candidate for fuzzing on a private testnet. What kind of testing did you do already?
@@ -184,7 +183,7 @@ public OperationResult execute(final MessageFrame frame, final EVM evm) { | |||
frame.expandMemory(outputDataOffset(frame), outputDataLength(frame)); | |||
frame.incrementRemainingGas(gasAvailableForChildCall(frame) + cost); | |||
frame.popStackItems(getStackItemsConsumed()); | |||
frame.pushStackItem(UInt256.ZERO); | |||
frame.pushStackItem(Bytes.EMPTY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FAILURE_STACK_ITEM here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -41,10 +43,10 @@ public BlockHashOperation(final GasCalculator gasCalculator) { | |||
@Override | |||
public Operation.OperationResult executeFixedCostOperation( | |||
final MessageFrame frame, final EVM evm) { | |||
final UInt256 blockArg = UInt256.fromBytes(frame.popStackItem()); | |||
final Bytes blockArg = frame.popStackItem().trimLeadingZeros(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might this lead to problems if we are not aligned on a word boundary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't. The call to blockArg.toLong()
on line 54 handles any byte length of 8 or less. That's the only use of this variable.
@@ -21,6 +21,9 @@ | |||
/** The Jump dest operation. */ | |||
public class JumpDestOperation extends AbstractFixedCostOperation { | |||
|
|||
/** constant for a successful jumpdest * */ | |||
public static final OperationResult jumpdestSuccess = new OperationResult(1L, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest all caps on public static final
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
final UInt256 result = value0.subtract(value1); | ||
final BigInteger result = value0.subtract(value1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
surprising that BigInteger is more performant than UInt256
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect there are some JVM optimizations targeting that stretch of code. It doesn't look like it's directly an @intrinsic function but it ought to be. Also, the zero check in UInt256 is kind of expensive.
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Before I press Merge I was going to give it a day or so on Marius's fuzzer. It found the other corner cases fairly quickly. |
How did it go? @shemnon |
Use a primitive to bytes function that is more direct than the Bytes methods. Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
Fuzz testing was useful. |
b8ab092
to
1b343e1
Compare
Performance improvements - Reduce the use of UInt256 in operations, replacing them with Bytes as appropriate. - Use Java17 expression switch Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Performance improvements - Reduce the use of UInt256 in operations, replacing them with Bytes as appropriate. - Use Java17 expression switch Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
PR description
Improve the performance of the EVM by reducing the usage of UInt256 in almost all operations and moving the main switch to a Java 17 expression switch
Fixed Issue(s)