EVM Speed Improvements for MSTORE #2917

shemnon · 2021-10-18T00:19:08Z

PR Description

More EVM Speed Improvements aimed at improving MSTORE times.

Move Memory from raw bytes to byte array
Add calls for 32 byte MSB aligned writes
Remove incrementProgramCounter and move data into OperationResult

Signed-off-by: Danno Ferrin danno.ferrin@gmail.com

Changelog

I thought about the changelog and included a changelog update if required.

More EVM Speed Improvements aimed at improving MLOAD times. * Move Memory from raw bytes to byte array * Add calls for 32 byte MSB aligned copies * Remove `incrementProgramCounter` and move data into OperationResult Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

shemnon · 2021-10-18T00:20:59Z

Performance spreadsheet

48% improvement over all test cases, 266% improvement in mstore. Slowest operation is now SHA3/KECCAK

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

Casting an int out of Bytes can be either ArithmeticException or IllegalArgumentException, so treat all Runtime exceptions as invalid destinations. Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

Trim leading zeros on jump. Since it is unsigned we don't need to worry about sign-extended negative numbers. Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

garyschulte · 2021-10-19T23:50:02Z

evm/src/main/java/org/hyperledger/besu/evm/EVM.java

@@ -184,7 +164,7 @@ public boolean isValidJumpDestination(final UInt256 destination, final Code code
        }
      }
    }
-    long targetLong = validJumpDestinations[jumpDestination >> 6];
+    long targetLong = validJumpDestinations[jumpDestination >>> 6];


I stared at this for a bit before I saw the jumpDestination < 0 check on 154.

garyschulte

This LGTM, the only question I have is the potential performance tradeoff between byte[] for faster MSTORE and MutableBytes for read performance since @matkt made that optimization.

garyschulte · 2021-10-20T00:04:46Z

evm/src/main/java/org/hyperledger/besu/evm/frame/Memory.java

@@ -36,17 +37,17 @@
   * overflow this. A byte array implementation limits us to 2 GiB. But that would cost over 51
   * trillion gas. So this is likely a reasonable limitation, at least at first.
   */
-  private MutableBytes data;
+  private byte[] memBytes;


does the benefit of going back to byte[] for MSTORE outweigh the read optimization @matkt did in #2673 ? Are there additional considerations that make immutable byte[] a better choice here?

It's a mix of the two. This goes back to byte[] but does not use the array copying code that was in place before. It directly slices the array into Byte. The code segments that needed to copy bytes are still doing a copy on the resulting byte object. Those were introduced by MutableBytes and not removed, and are actually orthogonal to the backing data.

The bigger issue is that doing right-aligned copying in MutableBytes efficiently requires APIs that don't exist on it.

garyschulte · 2021-10-20T00:53:18Z

evm/src/main/java/org/hyperledger/besu/evm/operation/AddOperation.java

+    final BigInteger value0 = new BigInteger(1, frame.popStackItem().toArrayUnsafe());
+    final BigInteger value1 = new BigInteger(1, frame.popStackItem().toArrayUnsafe());
+
+    final BigInteger result = value0.add(value1);


Are we seeing better performance using BigInteger rather than UInt256?

Yes. The use of UInt256 incurred a few more byte[]->int[]->byte[] loops. It is a more pronounced improvement in div, mul, and mod as there are less cycles spent moving bytes into new objects.

jflo

The javadoc on Memory.set32BytesAligned could stand some clarification. It is confusing as written. I've included suggestions inline.

jflo · 2021-10-20T01:40:00Z

evm/src/main/java/org/hyperledger/besu/evm/frame/Memory.java

@@ -202,7 +202,7 @@ public Bytes getBytes(final long location, final long numBytes) {
    final int start = asByteIndex(location);

    ensureCapacityForBytes(start, length);
-    return data.slice(start, length);
+    return Bytes.wrap(Arrays.copyOfRange(memBytes, start, start + length));


Nice catch. Tuweni .slice sounds mutable based on their docs, but i may be misreading, sounds ambiguous.

It can be mutable if the underlying bytes are mutable. We got burned on that with #2908.

jflo · 2021-10-20T01:51:40Z

evm/src/main/java/org/hyperledger/besu/evm/frame/Memory.java

+  }
+
+  /**
+   * Copy the bytes from the provided number of bytes from the provided value to memory from the


This could be worded better. Suggest "Copy numBytes bytes from taintedValue to memory at location" or some variant with fewer froms.

I just re-wrote it to my liking. Initially it was copy-paste from the left aligned variant.

jflo · 2021-10-20T01:54:44Z

evm/src/main/java/org/hyperledger/besu/evm/frame/Memory.java

+   * <p>Note that this method will extend memory to accommodate the location assigned and bytes
+   * copied and so never fails.
+   *
+   * @param location the location in memory at which to start copying the bytes of {@code value}.


adding "to" at the end of this description helps

jflo · 2021-10-20T01:56:00Z

evm/src/main/java/org/hyperledger/besu/evm/frame/Memory.java

+   *     numBytes} (in other words, {@link #getActiveWords()} will return a value consistent with
+   *     having set {@code numBytes} bytes, even if less than that have been concretely set due to
+   *     {@code value} being smaller).
+   * @param taintedValue the bytes to copy to memory from {@code location}.


suggest s/from/at

jflo · 2021-10-20T02:16:12Z

evm/src/main/java/org/hyperledger/besu/evm/frame/MessageFrame.java

+      if (length > srcSize) {
+        final MutableBytes paddedAnswer = MutableBytes.create((int) length);
+        if ((long) 0 < srcSize) {
+          value.slice(0, (int) ((long) srcSize)).copyTo(paddedAnswer, (int) (length - srcSize));


what is casting srcSize to long and then back to int doing when defining the length of the slice being copied?

Not sure how neither IntelliJ nor Errorprone tagged it. Initially it was because memory was going to be long indexed, until I was satisfied that 33rd bit memory access was unlikely to ever be viable even in perverse circumstances.

Removed. Sonar flagged it.

jflo · 2021-10-20T02:38:19Z

evm/src/main/java/org/hyperledger/besu/evm/operation/PushOperation.java

@@ -28,6 +30,8 @@

  private final int length;

+  private final OperationResult pushResponse;


Curious, why pull this down from successResponse in parent? readability?

Each push response has a different success, counting the PC to increment (2 for PUSH1, 33 for PUSH32). The parent successResponse increments the PC by 1.

jflo · 2021-10-20T03:00:29Z

Performance spreadsheet

48% improvement over all test cases, 266% improvement in mstore. Slowest operation is now SHA3/KECCAK

sick. how do we get that spreadsheet generated as part of CI? I'd love to be alerted to performance regressions.

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

Bytes.wrap() presumes an immutable array, and only does a shallow copy on a perfect "slice." MutableBytes assumes the bytes could change and always does a deep copy when a copy is requested. Also, updated API to reflect mutable bytes type usage. Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

sonarqubecloud · 2021-10-20T19:27:12Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

93.0% Coverage
3.4% Duplication

garyschulte

🚢 👍

More EVM Speed Improvements aimed at improving MSTORE times. * Move Memory from raw bytes to byte array * Add calls for right packed partial value copy * Remove `incrementProgramCounter` and move data into OperationResult Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

shemnon added 2 commits October 17, 2021 18:28

remove updatesProgramCounter

af258cd

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

changelog

4f31b8d

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

shemnon changed the title ~~EVM Speed Improvements for MLOAD~~ EVM Speed Improvements for MSTORE Oct 18, 2021

spotless

3d3197a

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

shemnon marked this pull request as draft October 18, 2021 06:03

shemnon added 2 commits October 18, 2021 09:27

fix tracing support

8519dea

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

Merge branch 'main' into evmSpeedupMLoad

127a757

shemnon marked this pull request as ready for review October 18, 2021 16:29

shemnon added 4 commits October 18, 2021 12:07

address jump operation regression

0f27c0b

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

merge, update target version

0b07552

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

Casting an int out of Bytes exception

49189c0

Casting an int out of Bytes can be either ArithmeticException or IllegalArgumentException, so treat all Runtime exceptions as invalid destinations. Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

Trim leading zeros

54575b0

Trim leading zeros on jump. Since it is unsigned we don't need to worry about sign-extended negative numbers. Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

shemnon marked this pull request as draft October 18, 2021 19:40

Bounds check and sign free shift

27e28d1

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

shemnon marked this pull request as ready for review October 18, 2021 20:36

Merge branch 'main' into evmSpeedupMLoad

3f440ab

garyschulte reviewed Oct 19, 2021

View reviewed changes

garyschulte reviewed Oct 20, 2021

View reviewed changes

jflo reviewed Oct 20, 2021

View reviewed changes

shemnon and others added 6 commits October 19, 2021 23:05

Merge branch 'main' of github.com:hyperledger/besu into evmSpeedupMLoad

6922c87

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

Javadoc, Naming Fixes, sonar

eaff0ff

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

spotless

5c460ec

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

Merge branch 'main' into evmSpeedupMLoad

844c332

Merge branch 'main' into evmSpeedupMLoad

e5f26c3

garyschulte approved these changes Oct 20, 2021

View reviewed changes

shemnon merged commit ec5623f into hyperledger:main Oct 20, 2021

shemnon deleted the evmSpeedupMLoad branch February 26, 2022 18:43

shemnon added mining mainnet and removed mining labels May 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EVM Speed Improvements for MSTORE #2917

EVM Speed Improvements for MSTORE #2917

shemnon commented Oct 18, 2021 •

edited

Loading

shemnon commented Oct 18, 2021 •

edited

Loading

garyschulte Oct 19, 2021

garyschulte left a comment

garyschulte Oct 20, 2021

shemnon Oct 20, 2021 •

edited

Loading

shemnon Oct 20, 2021

garyschulte Oct 20, 2021

shemnon Oct 20, 2021

jflo left a comment

jflo Oct 20, 2021

shemnon Oct 20, 2021

jflo Oct 20, 2021

shemnon Oct 20, 2021

jflo Oct 20, 2021

jflo Oct 20, 2021

jflo Oct 20, 2021

shemnon Oct 20, 2021

shemnon Oct 20, 2021

jflo Oct 20, 2021

shemnon Oct 20, 2021

jflo commented Oct 20, 2021

sonarqubecloud bot commented Oct 20, 2021

garyschulte left a comment

		@@ -28,6 +30,8 @@

		private final int length;

		private final OperationResult pushResponse;

EVM Speed Improvements for MSTORE #2917

EVM Speed Improvements for MSTORE #2917

Conversation

shemnon commented Oct 18, 2021 • edited Loading

PR Description

Changelog

shemnon commented Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

garyschulte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shemnon Oct 20, 2021 • 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

jflo left a 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

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

Choose a reason for hiding this comment

jflo commented Oct 20, 2021

sonarqubecloud bot commented Oct 20, 2021

garyschulte left a comment

Choose a reason for hiding this comment

shemnon commented Oct 18, 2021 •

edited

Loading

shemnon commented Oct 18, 2021 •

edited

Loading

shemnon Oct 20, 2021 •

edited

Loading