clones-with-immutable-args icon indicating copy to clipboard operation
clones-with-immutable-args copied to clipboard

⛽ Gas Optimizations

Open Saw-mon-and-Natalie opened this issue 2 years ago • 6 comments

This PR applies some gas optimizations to ClonesWithImmutableArgs.sol and Clone.sol.

  • ClonesWithImmutableArgs.sol has been rewritten to optimize gas and also consolidate all the assembly blocks.
  • The edits to Clone.sol include using more assembly for _getArgUint256Array method and also together with ClonesWithImmutableArgs.sol edits, we are assuming the 2 bytes at the end of the clone/proxy contract is the data length plus 2. In this way we would save subtracting 2 in ClonesWithImmutableArgs.sol and adding it back in Clone.sol to calculate the extra data offset in the calldata.

In the 1st commit 30958a4d3a39c31293353dee7c41519df9ca133b, we update the .gas-snapshot since it was not previously updated.

Here is the result of running forge snapshot ... with the new optimization edits:

$ forge snapshot --include-fuzz-tests --desc --diff
[⠒] Compiling...
[⠃] Compiling 8 files with 0.8.13
[⠊] Solc 0.8.13 finished in 1.79s
Compiler run successful

Running 4 tests for src\test\ExampleClone.t.sol:ExampleCloneTest
[PASS] testGas_param1() (gas: 8144)
[PASS] testGas_param2() (gas: 8074)
[PASS] testGas_param3() (gas: 8132)
[PASS] testGas_param4() (gas: 8177)
Test result: ok. 4 passed; 0 failed; finished in 2.97ms

Running 2 tests for src\test\ExampleCloneFactory.t.sol:ExampleCloneFactoryTest
[PASS] testCorrectness_clone(address,uint256,uint64,uint8) (runs: 256, μ: 69927, ~: 69927)
[PASS] testGas_clone(address,uint256,uint64,uint8) (runs: 256, μ: 64003, ~: 64003)
Test result: ok. 2 passed; 0 failed; finished in 170.58ms
testGas_clone(address,uint256,uint64,uint8) (gas: -576 (-0.009%))
testGas_param2() (gas: -105 (-0.013%))
testGas_param4() (gas: -117 (-0.014%))
testGas_param3() (gas: -117 (-0.014%))
testCorrectness_clone(address,uint256,uint64,uint8) (gas: -1038 (-0.015%))
testGas_param1() (gas: -123 (-0.015%))
Overall gas change: -2076 (-0.080%)

Saw-mon-and-Natalie avatar Jul 10 '22 15:07 Saw-mon-and-Natalie

do you need unchecked here? https://github.com/wighawag/clones-with-immutable-args/blob/732be6078792f02caca0df341a75f1988008921f/src/ClonesWithImmutableArgs.sol#L25

z0r0z avatar Jul 12 '22 22:07 z0r0z

do you need unchecked here?

https://github.com/wighawag/clones-with-immutable-args/blob/732be6078792f02caca0df341a75f1988008921f/src/ClonesWithImmutableArgs.sol#L25

@z0r0z , This unchecked block was carried over from the current implementation.

The potential overflows that might be problematic would be:

https://github.com/wighawag/clones-with-immutable-args/blob/732be6078792f02caca0df341a75f1988008921f/src/ClonesWithImmutableArgs.sol#L28-L29

and

https://github.com/wighawag/clones-with-immutable-args/blob/732be6078792f02caca0df341a75f1988008921f/src/ClonesWithImmutableArgs.sol#L151

Basically for the overflow to happen we would need add(ptr, creationSize) or

$$ \tt{ptr} + \tt{BOOTSTRAPLENGTH} + \tt{mload}(\tt{data}) + 2 \geq 2^{256} $$

$$ \tt{ptr} + \tt{mload}(\tt{data}) \geq 2^{256} - 65 $$

From EIP-170, we know that the MAX_CODE_SIZE is a capped at $2^{14} + 2^{13}$. That means

$$ \tt{mload}(\tt{data}) \lt 2^{14} + 2^{13} $$

otherwise, the create instruction should fail:

https://github.com/wighawag/clones-with-immutable-args/blob/732be6078792f02caca0df341a75f1988008921f/src/ClonesWithImmutableArgs.sol#L148

So combing everything so far, we should have in case of an overflow as a rough estimate:

$$ \tt{ptr} \geq 2^{256} - 2^{14} - 2^{13} - 65 \gt 2^{255} $$

This would mean the memory has been expanded quite a lot, plugging the lower band in the memory expansion cost function would roughly give us

$$ C = 3 \cdot 2^{255} + \lceil \frac{2^{510}}{512} \rceil \gt 2^{500} \gg 30,000,000 $$

So the memory expansion gas cost would be astronomically high. Based on these calculations the function call would run out of gas before it would overflow.

Saw-mon-and-Natalie avatar Jul 19 '22 12:07 Saw-mon-and-Natalie

I am under the impression that unchecked doesn't apply w/i an assembly block and so may be safely removed here

wminshew avatar Aug 12 '22 05:08 wminshew

@wminshew , you are right. I updated the PR.

@z0r0z, @wighawag , please have a look. It know includes more optimizations compared to a few months back.

Saw-mon-and-Natalie avatar Nov 25 '22 10:11 Saw-mon-and-Natalie

anything blocking this? I'm wanting to deploy a clone soon and these changes look helpful

BlinkyStitt avatar Aug 20 '23 20:08 BlinkyStitt

@WyseNynja believe solady's clone has these updates + others baked in

wminshew avatar Aug 21 '23 03:08 wminshew