Optimize Sstore operation execution time (#4836)

* Optimize Sstore operation, get current value and original value only once. Signed-off-by: Ameziane H <[email protected]> * Fix javadoc. Signed-off-by: Ameziane H <[email protected]> * refactoring (add a supplier implementation to better support all the forks) Signed-off-by: Ameziane H <[email protected]> * delete unnecessary parameters, add final for some fields and modify CHANGELOG.md Signed-off-by: Ameziane H <[email protected]> * add a missing parameter in Javadoc Signed-off-by: Ameziane H <[email protected]> Signed-off-by: Ameziane H <[email protected]>
hyperledger · Dec 23, 2022 · 3ecb09d · 3ecb09d
1 parent 0c5b36b
commit 3ecb09d
Show file tree

Hide file tree

Showing 11 changed files with 138 additions and 98 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -16,6 +16,7 @@
 - Optimization:  Memoize transaction size and hash at the same time [#4812](https://github.com/hyperledger/besu/pull/4812)
 
 ### Breaking Changes
+- Optimize SSTORE Operation execution time (memoize current and original value) [#4836](https://github.com/hyperledger/besu/pull/4836)
 
 ### Bug Fixes
 

diff --git a/...reum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/BonsaiWorldStateUpdater.java b/...reum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/BonsaiWorldStateUpdater.java
@@ -367,8 +367,8 @@ public Optional<UInt256> getStorageValueBySlotHash(final Address address, final
   public UInt256 getPriorStorageValue(final Address address, final UInt256 storageKey) {
     // TODO maybe log the read into the trie layer?
     final Map<Hash, BonsaiValue<UInt256>> localAccountStorage = storageToUpdate.get(address);
-    final Hash slotHash = Hash.hash(storageKey);
     if (localAccountStorage != null) {
+      final Hash slotHash = Hash.hash(storageKey);
       final BonsaiValue<UInt256> value = localAccountStorage.get(slotHash);
       if (value != null) {
         if (value.isCleared()) {

diff --git a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/mainnet/RefundSstoreGasTest.java b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/mainnet/RefundSstoreGasTest.java
@@ -19,12 +19,12 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
-import org.hyperledger.besu.evm.account.Account;
 import org.hyperledger.besu.evm.gascalculator.ConstantinopleGasCalculator;
 import org.hyperledger.besu.evm.gascalculator.GasCalculator;
 import org.hyperledger.besu.evm.gascalculator.IstanbulGasCalculator;
 import org.hyperledger.besu.evm.gascalculator.PetersburgGasCalculator;
 
+import com.google.common.base.Supplier;
 import org.apache.tuweni.units.bigints.UInt256;
 import org.assertj.core.api.Assertions;
 import org.junit.Before;
@@ -122,24 +122,33 @@ public static Object[][] scenarios() {
   @Parameter(value = 6)
   public long expectedGasRefund;
 
-  private final Account account = mock(Account.class);
+  private final Supplier<UInt256> mockSupplierForOriginalValue = mockSupplier();
+  private final Supplier<UInt256> mockSupplierCurrentValue = mockSupplier();
+
+  @SuppressWarnings("unchecked")
+  private <T> Supplier<T> mockSupplier() {
+    return mock(Supplier.class);
+  }
 
   @Before
   public void setUp() {
-    when(account.getOriginalStorageValue(UInt256.ZERO)).thenReturn(originalValue);
-    when(account.getStorageValue(UInt256.ZERO)).thenReturn(currentValue);
+    when(mockSupplierForOriginalValue.get()).thenReturn(originalValue);
+    when(mockSupplierCurrentValue.get()).thenReturn(currentValue);
   }
 
   @Test
   public void shouldChargeCorrectGas() {
-    Assertions.assertThat(gasCalculator.calculateStorageCost(account, UInt256.ZERO, newValue))
+    Assertions.assertThat(
+            gasCalculator.calculateStorageCost(
+                newValue, mockSupplierCurrentValue, mockSupplierForOriginalValue))
         .isEqualTo(expectedGasCost);
   }
 
   @Test
   public void shouldRefundCorrectGas() {
     Assertions.assertThat(
-            gasCalculator.calculateStorageRefundAmount(account, UInt256.ZERO, newValue))
+            gasCalculator.calculateStorageRefundAmount(
+                newValue, mockSupplierCurrentValue, mockSupplierForOriginalValue))
         .isEqualTo(expectedGasRefund);
   }
 }
diff --git a/evm/src/main/java/org/hyperledger/besu/evm/gascalculator/BerlinGasCalculator.java b/evm/src/main/java/org/hyperledger/besu/evm/gascalculator/BerlinGasCalculator.java
@@ -28,6 +28,7 @@
 
 import java.math.BigInteger;
 
+import com.google.common.base.Supplier;
 import org.apache.tuweni.bytes.Bytes;
 import org.apache.tuweni.units.bigints.UInt256;
 
@@ -163,15 +164,17 @@ public long extCodeCopyOperationGasCost(
   @Override
   // As per https://eips.ethereum.org/EIPS/eip-2200
   public long calculateStorageCost(
-      final Account account, final UInt256 key, final UInt256 newValue) {
+      final UInt256 newValue,
+      final Supplier<UInt256> currentValue,
+      final Supplier<UInt256> originalValue) {
 
-    final UInt256 currentValue = account.getStorageValue(key);
-    if (currentValue.equals(newValue)) {
+    final UInt256 localCurrentValue = currentValue.get();
+    if (localCurrentValue.equals(newValue)) {
       return SLOAD_GAS;
     } else {
-      final UInt256 originalValue = account.getOriginalStorageValue(key);
-      if (originalValue.equals(currentValue)) {
-        return originalValue.isZero() ? SSTORE_SET_GAS : SSTORE_RESET_GAS;
+      final UInt256 localOriginalValue = originalValue.get();
+      if (localOriginalValue.equals(localCurrentValue)) {
+        return localOriginalValue.isZero() ? SSTORE_SET_GAS : SSTORE_RESET_GAS;
       } else {
         return SLOAD_GAS;
       }
@@ -182,15 +185,17 @@ public long calculateStorageCost(
   @Override
   // As per https://eips.ethereum.org/EIPS/eip-2200
   public long calculateStorageRefundAmount(
-      final Account account, final UInt256 key, final UInt256 newValue) {
+      final UInt256 newValue,
+      final Supplier<UInt256> currentValue,
+      final Supplier<UInt256> originalValue) {
 
-    final UInt256 currentValue = account.getStorageValue(key);
-    if (currentValue.equals(newValue)) {
+    final UInt256 localCurrentValue = currentValue.get();
+    if (localCurrentValue.equals(newValue)) {
       return 0L;
     } else {
-      final UInt256 originalValue = account.getOriginalStorageValue(key);
-      if (originalValue.equals(currentValue)) {
-        if (originalValue.isZero()) {
+      final UInt256 localOriginalValue = originalValue.get();
+      if (localOriginalValue.equals(localCurrentValue)) {
+        if (localOriginalValue.isZero()) {
           return 0L;
         } else if (newValue.isZero()) {
           return SSTORE_CLEARS_SCHEDULE;
@@ -199,18 +204,18 @@ public long calculateStorageRefundAmount(
         }
       } else {
         long refund = 0L;
-        if (!originalValue.isZero()) {
-          if (currentValue.isZero()) {
+        if (!localOriginalValue.isZero()) {
+          if (localCurrentValue.isZero()) {
             refund = NEGATIVE_SSTORE_CLEARS_SCHEDULE;
           } else if (newValue.isZero()) {
             refund = SSTORE_CLEARS_SCHEDULE;
           }
         }
 
-        if (originalValue.equals(newValue)) {
+        if (localOriginalValue.equals(newValue)) {
           refund =
               refund
-                  + (originalValue.isZero()
+                  + (localOriginalValue.isZero()
                       ? SSTORE_SET_GAS_LESS_SLOAD_GAS
                       : SSTORE_RESET_GAS_LESS_SLOAD_GAS);
         }

diff --git a/evm/src/main/java/org/hyperledger/besu/evm/gascalculator/ConstantinopleGasCalculator.java b/evm/src/main/java/org/hyperledger/besu/evm/gascalculator/ConstantinopleGasCalculator.java
@@ -18,9 +18,9 @@
 import static org.hyperledger.besu.evm.internal.Words.clampedMultiply;
 import static org.hyperledger.besu.evm.internal.Words.clampedToLong;
 
-import org.hyperledger.besu.evm.account.Account;
 import org.hyperledger.besu.evm.frame.MessageFrame;
 
+import com.google.common.base.Supplier;
 import org.apache.tuweni.bytes.Bytes32;
 import org.apache.tuweni.units.bigints.UInt256;
 
@@ -48,15 +48,17 @@ public long create2OperationGasCost(final MessageFrame frame) {
   @Override
   // As per https://eips.ethereum.org/EIPS/eip-1283
   public long calculateStorageCost(
-      final Account account, final UInt256 key, final UInt256 newValue) {
+      final UInt256 newValue,
+      final Supplier<UInt256> currentValue,
+      final Supplier<UInt256> originalValue) {
 
-    final UInt256 currentValue = account.getStorageValue(key);
-    if (currentValue.equals(newValue)) {
+    final UInt256 localCurrentValue = currentValue.get();
+    if (localCurrentValue.equals(newValue)) {
       return SSTORE_NO_OP_COST;
     } else {
-      final UInt256 originalValue = account.getOriginalStorageValue(key);
-      if (originalValue.equals(currentValue)) {
-        return originalValue.isZero()
+      final UInt256 localOriginalValue = originalValue.get();
+      if (localOriginalValue.equals(localCurrentValue)) {
+        return localOriginalValue.isZero()
             ? SSTORE_FIRST_DIRTY_NEW_STORAGE_COST
             : SSTORE_FIRST_DIRTY_EXISTING_STORAGE_COST;
       } else {
@@ -68,15 +70,17 @@ public long calculateStorageCost(
   @Override
   // As per https://eips.ethereum.org/EIPS/eip-1283
   public long calculateStorageRefundAmount(
-      final Account account, final UInt256 key, final UInt256 newValue) {
+      final UInt256 newValue,
+      final Supplier<UInt256> currentValue,
+      final Supplier<UInt256> originalValue) {
 
-    final UInt256 currentValue = account.getStorageValue(key);
-    if (currentValue.equals(newValue)) {
+    UInt256 localCurrentValue = currentValue.get();
+    if (localCurrentValue.equals(newValue)) {
       return 0L;
     } else {
-      final UInt256 originalValue = account.getOriginalStorageValue(key);
-      if (originalValue.equals(currentValue)) {
-        if (originalValue.isZero()) {
+      UInt256 localOriginalValue = originalValue.get();
+      if (localOriginalValue.equals(localCurrentValue)) {
+        if (localOriginalValue.isZero()) {
           return 0L;
         } else if (newValue.isZero()) {
           return STORAGE_RESET_REFUND_AMOUNT;
@@ -85,18 +89,18 @@ public long calculateStorageRefundAmount(
         }
       } else {
         long refund = 0L;
-        if (!originalValue.isZero()) {
-          if (currentValue.isZero()) {
+        if (!localOriginalValue.isZero()) {
+          if (localCurrentValue.isZero()) {
             refund = NEGATIVE_STORAGE_RESET_REFUND_AMOUNT;
           } else if (newValue.isZero()) {
             refund = STORAGE_RESET_REFUND_AMOUNT;
           }
         }
 
-        if (originalValue.equals(newValue)) {
+        if (localOriginalValue.equals(newValue)) {
           refund =
               refund
-                  + (originalValue.isZero()
+                  + (localOriginalValue.isZero()
                       ? SSTORE_DIRTY_RETURN_TO_UNUSED_REFUND_AMOUNT
                       : SSTORE_DIRTY_RETURN_TO_ORIGINAL_VALUE_REFUND_AMOUNT);
         }

diff --git a/evm/src/main/java/org/hyperledger/besu/evm/gascalculator/FrontierGasCalculator.java b/evm/src/main/java/org/hyperledger/besu/evm/gascalculator/FrontierGasCalculator.java
@@ -25,6 +25,7 @@
 import org.hyperledger.besu.evm.internal.Words;
 import org.hyperledger.besu.evm.operation.ExpOperation;
 
+import com.google.common.base.Supplier;
 import org.apache.tuweni.bytes.Bytes;
 import org.apache.tuweni.units.bigints.UInt256;
 
@@ -417,18 +418,20 @@ public long getSloadOperationGasCost() {
 
   @Override
   public long calculateStorageCost(
-      final Account account, final UInt256 key, final UInt256 newValue) {
-    return !newValue.isZero() && account.getStorageValue(key).isZero()
+      final UInt256 newValue,
+      final Supplier<UInt256> currentValue,
+      final Supplier<UInt256> originalValue) {
+    return !newValue.isZero() && currentValue.get().isZero()
         ? STORAGE_SET_GAS_COST
         : STORAGE_RESET_GAS_COST;
   }
 
   @Override
   public long calculateStorageRefundAmount(
-      final Account account, final UInt256 key, final UInt256 newValue) {
-    return newValue.isZero() && !account.getStorageValue(key).isZero()
-        ? STORAGE_RESET_REFUND_AMOUNT
-        : 0L;
+      final UInt256 newValue,
+      final Supplier<UInt256> currentValue,
+      final Supplier<UInt256> originalValue) {
+    return newValue.isZero() && !currentValue.get().isZero() ? STORAGE_RESET_REFUND_AMOUNT : 0L;
   }
 
   @Override

diff --git a/evm/src/main/java/org/hyperledger/besu/evm/gascalculator/GasCalculator.java b/evm/src/main/java/org/hyperledger/besu/evm/gascalculator/GasCalculator.java
@@ -41,6 +41,7 @@
 
 import java.util.List;
 
+import com.google.common.base.Supplier;
 import org.apache.tuweni.bytes.Bytes;
 import org.apache.tuweni.units.bigints.UInt256;
 
@@ -348,22 +349,24 @@ long callOperationGasCost(
   /**
    * Returns the cost for an SSTORE operation.
    *
-   * @param account the account that storage will be changed in
-   * @param key the key the new value is to be stored under
    * @param newValue the new value to be stored
+   * @param currentValue the supplier of the current value
+   * @param originalValue the supplier of the original value
    * @return the gas cost for the SSTORE operation
    */
-  long calculateStorageCost(Account account, UInt256 key, UInt256 newValue);
+  long calculateStorageCost(
+      UInt256 newValue, Supplier<UInt256> currentValue, Supplier<UInt256> originalValue);
 
   /**
    * Returns the refund amount for an SSTORE operation.
    *
-   * @param account the account that storage will be changed in
-   * @param key the key the new value is to be stored under
    * @param newValue the new value to be stored
+   * @param currentValue the supplier of the current value
+   * @param originalValue the supplier of the original value
    * @return the gas refund for the SSTORE operation
    */
-  long calculateStorageRefundAmount(Account account, UInt256 key, UInt256 newValue);
+  long calculateStorageRefundAmount(
+      UInt256 newValue, Supplier<UInt256> currentValue, Supplier<UInt256> originalValue);
 
   /**
    * Returns the refund amount for deleting an account in a {@link SelfDestructOperation}.

diff --git a/evm/src/main/java/org/hyperledger/besu/evm/gascalculator/IstanbulGasCalculator.java b/evm/src/main/java/org/hyperledger/besu/evm/gascalculator/IstanbulGasCalculator.java
@@ -14,8 +14,7 @@
  */
 package org.hyperledger.besu.evm.gascalculator;
 
-import org.hyperledger.besu.evm.account.Account;
-
+import com.google.common.base.Supplier;
 import org.apache.tuweni.bytes.Bytes;
 import org.apache.tuweni.units.bigints.UInt256;
 
@@ -56,15 +55,17 @@ public long transactionIntrinsicGasCost(final Bytes payload, final boolean isCon
   @Override
   // As per https://eips.ethereum.org/EIPS/eip-2200
   public long calculateStorageCost(
-      final Account account, final UInt256 key, final UInt256 newValue) {
+      final UInt256 newValue,
+      final Supplier<UInt256> currentValue,
+      final Supplier<UInt256> originalValue) {
 
-    final UInt256 currentValue = account.getStorageValue(key);
-    if (currentValue.equals(newValue)) {
+    final UInt256 localCurrentValue = currentValue.get();
+    if (localCurrentValue.equals(newValue)) {
       return SLOAD_GAS;
     } else {
-      final UInt256 originalValue = account.getOriginalStorageValue(key);
-      if (originalValue.equals(currentValue)) {
-        return originalValue.isZero() ? SSTORE_SET_GAS : SSTORE_RESET_GAS;
+      final UInt256 localOriginalValue = originalValue.get();
+      if (localOriginalValue.equals(localCurrentValue)) {
+        return localOriginalValue.isZero() ? SSTORE_SET_GAS : SSTORE_RESET_GAS;
       } else {
         return SLOAD_GAS;
       }
@@ -74,15 +75,17 @@ public long calculateStorageCost(
   @Override
   // As per https://eips.ethereum.org/EIPS/eip-2200
   public long calculateStorageRefundAmount(
-      final Account account, final UInt256 key, final UInt256 newValue) {
+      final UInt256 newValue,
+      final Supplier<UInt256> currentValue,
+      final Supplier<UInt256> originalValue) {
 
-    final UInt256 currentValue = account.getStorageValue(key);
-    if (currentValue.equals(newValue)) {
+    final UInt256 localCurrentValue = currentValue.get();
+    if (localCurrentValue.equals(newValue)) {
       return 0L;
     } else {
-      final UInt256 originalValue = account.getOriginalStorageValue(key);
-      if (originalValue.equals(currentValue)) {
-        if (originalValue.isZero()) {
+      final UInt256 localOriginalValue = originalValue.get();
+      if (localOriginalValue.equals(localCurrentValue)) {
+        if (localOriginalValue.isZero()) {
           return 0L;
         } else if (newValue.isZero()) {
           return SSTORE_CLEARS_SCHEDULE;
@@ -91,18 +94,18 @@ public long calculateStorageRefundAmount(
         }
       } else {
         long refund = 0L;
-        if (!originalValue.isZero()) {
-          if (currentValue.isZero()) {
+        if (!localOriginalValue.isZero()) {
+          if (localCurrentValue.isZero()) {
             refund = NEGATIVE_SSTORE_CLEARS_SCHEDULE;
           } else if (newValue.isZero()) {
             refund = SSTORE_CLEARS_SCHEDULE;
           }
         }
 
-        if (originalValue.equals(newValue)) {
+        if (localOriginalValue.equals(newValue)) {
           refund =
               refund
-                  + (originalValue.isZero()
+                  + (localOriginalValue.isZero()
                       ? SSTORE_SET_GAS_LESS_SLOAD_GAS
                       : SSTORE_RESET_GAS_LESS_SLOAD_GAS);
         }