V2 optimize #55

Vectorized · 2023-08-07T15:53:14Z

Contracts look very efficient overall.

Some minor optimizations.

0xfoobar · 2023-08-07T16:41:15Z

Gas diff GH action seems broken on external contributors, can you manually run

forge test --match-contract GasBenchmark --gas-report > gasbenchmark10mil and commit that for comparison?

forge test --match-contract HashBenchmark --gas-report > hashbenchmark10mil is also useful

src/libraries/RegistryHashes.sol

Vectorized · 2023-08-07T17:25:10Z

src/libraries/RegistryStorage.sol

        assembly {
            from := and(firstPacked, CLEAN_ADDRESS)
            to := and(secondPacked, CLEAN_ADDRESS)
-            contract_ := or(shr(64, and(firstPacked, CLEAN_PACKED8_BYTES_ADDRESS)), shr(160, secondPacked))
+            contract_ := or(shr(64, xor(firstPacked, from)), shr(160, secondPacked))


Save some deployment bytecode. Same runtime gas performance.

0xfoobar · 2023-08-07T18:23:11Z

We should also use either consistent decimal or consistent hex across all code for pointer location addressing, @mireynolds would love to hear your thoughts here

…registry into v2-optimize

src/DelegateRegistry.sol

0xfoobar · 2023-08-07T23:08:32Z

"Inline assembly that neither involves any operations that access memory nor assigns to any Solidity variables in memory is automatically considered memory-safe and does not need to be annotated." -> Might make sense to leave blocks where this applies unannotated and let the compiler take care of it. Does this apply to any of the new tags @Vectorized ?

Actually on further thought probably good to be as explicit & descriptive as possible everywhere

mireynolds · 2023-08-08T12:18:33Z

We should also use either consistent decimal or consistent hex across all code for pointer location addressing, @mireynolds would love to hear your thoughts here

Think we should be using decimals consistently in assembly blocks as it makes the code more readable and understandable. It's important to note that most people reading the code are going to familiar with decimal arithmetic rather than hex arithmetic - so using hex makes it harder for others to verify what the code does.

mireynolds · 2023-08-08T12:23:04Z

"Inline assembly that neither involves any operations that access memory nor assigns to any Solidity variables in memory is automatically considered memory-safe and does not need to be annotated." -> Might make sense to leave blocks where this applies unannotated and let the compiler take care of it. Does this apply to any of the new tags @Vectorized ?

Actually on further thought probably good to be as explicit & descriptive as possible everywhere

We should only be marking assembly blocks as memory safe when it is appropriate: for an assembly block that uses memory operations according to solidity's safe memory rules. Marking assembly blocks as memory safe when they do not use memory operations makes no sense for explicitness and descriptiveness, and it is not convention to do that. If anything, it demonstrates that we don't understand the use of the notation and automatic treatment of assembly blocks which contain no memory operations.

mireynolds

Really like the ideas behind most of these optimisations. Most the issues I've raised can be resolved with minimal additional gas I think. The general theme here is we can skip compiler checks where appropriate / more efficient, but we can't make assumptions about compiler behaviour when it comes to memory management and presence of dirty bits.

src/DelegateRegistry.sol

src/libraries/RegistryHashes.sol

mireynolds · 2023-08-08T16:40:11Z

src/DelegateRegistry.sol

            valid = _validateDelegation(Hashes.allLocation(from, rights, to), from) || _validateDelegation(Hashes.contractLocation(from, rights, to, contract_), from)
                || _validateDelegation(Hashes.erc721Location(from, rights, to, tokenId, contract_), from);
        }
+        assembly ("memory-safe") {
+            mstore(0x00, valid)


Again, assumption of clean bits.

src/libraries/RegistryOps.sol

mireynolds · 2023-08-08T17:17:35Z

src/libraries/RegistryOps.sol

+    /// @dev `x & y`.
+    function and(bool x, bool y) internal pure returns (bool z) {
+        assembly ("memory-safe") {
+            z := and(x, y)


Assumption about clean bits here on the return value.

mireynolds · 2023-08-08T17:17:51Z

src/libraries/RegistryOps.sol

+    /// @dev `x | y`.
+    function or(bool x, bool y) internal pure returns (bool z) {
+        assembly ("memory-safe") {
+            z := or(x, y)


Similar to above, assumption of clean bits on the return value.

mireynolds · 2023-08-08T17:25:32Z

src/libraries/RegistryStorage.sol

@@ -52,7 +52,7 @@ library RegistryStorage {
        assembly {
            from := and(firstPacked, CLEAN_ADDRESS)
            to := and(secondPacked, CLEAN_ADDRESS)
-            contract_ := or(shr(64, and(firstPacked, CLEAN_PACKED8_BYTES_ADDRESS)), shr(160, secondPacked))
+            contract_ := or(shr(64, xor(firstPacked, from)), shr(160, secondPacked))


I think we need to stick with a version of this that doesn't result in dirty bits in contract_. We can't assume firstPacked is cleaned of the upper 8 bytes and return it uncleaned.

Vectorized · 2023-08-09T14:33:33Z

Thanks for the great feedback!

Please feel free to change anything to fit. :)

I'll continue focusing on other work now, will check back occasionally.

mireynolds

Mostly resolved from what I can tell except for the assumption of clean bits on bools could lead to unexpected behaviour with the iszero operations. Still the question of whether the global lock on memory optimization (as a result of memory usage outside the Solidity memory model in the assembly blocks) in a library that could be used by contracts with high memory usage or lots of local variables that might benefit from memory / stack optimisation.

mireynolds · 2023-08-09T14:35:36Z

src/DelegateRegistry.sol

-        if (rights != "" && !valid) valid = _validateDelegation(Hashes.allLocation(from, rights, to), from);
+        if (!Ops.or(rights == "", valid)) valid = _validateDelegation(Hashes.allLocation(from, rights, to), from);
+        assembly ("memory-safe") {
+            mstore(0x00, iszero(iszero(valid)))


This still assumes valid is free of dirty bits, which the compiler doesn't guarantee. A valid with dirty bits will incorrectly return false under iszero even if the last 8 bits are set to zero. If we're going to use assembly here, we need to clean valid, then store it in memory for return. Compiler will do this under the hood, so if we want to skip a bit of gas on the return we need to ensure it's going to give the same result as if there were no assembly block overriding the return of valid.

mireynolds · 2023-08-09T14:35:53Z

src/DelegateRegistry.sol

            valid = _validateDelegation(Hashes.allLocation(from, rights, to), from) || _validateDelegation(Hashes.contractLocation(from, rights, to, contract_), from);
        }
+        assembly ("memory-safe") {
+            mstore(0x00, iszero(iszero(valid)))


mireynolds · 2023-08-09T14:35:59Z

src/DelegateRegistry.sol

            valid = _validateDelegation(Hashes.allLocation(from, rights, to), from) || _validateDelegation(Hashes.contractLocation(from, rights, to, contract_), from)
                || _validateDelegation(Hashes.erc721Location(from, rights, to, tokenId, contract_), from);
        }
+        assembly ("memory-safe") {
+            mstore(0x00, iszero(iszero(valid)))


mireynolds · 2023-08-09T15:59:24Z

src/libraries/RegistryOps.sol

+    /// @dev `x & y`.
+    function and(bool x, bool y) internal pure returns (bool z) {
+        assembly {
+            z := iszero(iszero(and(x, y)))


Similar here, unexpected behaviour here if x and y contain dirty bits (which the compiler doesn't guarantee).

The truthyness of booleans in Solidity uses all 256 bits.

mireynolds · 2023-08-09T16:00:49Z

src/libraries/RegistryOps.sol

+    /// @dev `x | y`.
+    function or(bool x, bool y) internal pure returns (bool z) {
+        assembly {
+            z := iszero(iszero(or(x, y)))


Similar here, unexpected behaviour with dirty bits.

0xfoobar · 2023-08-09T17:35:07Z

Reminder to run forge fmt so the GH Action will progress to the test suite

0xfoobar · 2023-08-10T01:58:05Z

src/libraries/RegistryOps.sol

+    function and(bool x, bool y) internal pure returns (bool z) {
+        assembly {
+            // Any non-zero 256 bit word is true, else false.
+            z := and(iszero(iszero(x)), iszero(iszero(y)))


This double iszero() is incorrect, any dirty upper bits will cause iszero(iszero(x)) to evaluate to 1, even if the lowest bit is 0. So in best case (no dirty upper bits) it's a no-op, and in worst case (dirty upper bits) it can return the incorrect value. Should be removed & replaced with a proper bitshift or other cleaning method.

0xfoobar · 2023-08-10T01:58:16Z

src/libraries/RegistryOps.sol

+    function or(bool x, bool y) internal pure returns (bool z) {
+        assembly {
+            // Any non-zero 256 bit word is true, else false.
+            z := or(iszero(iszero(x)), iszero(iszero(y)))


Similar incorrect double iszero()

Any 256 bit word with one or more non-zero bit is considered true in regular Solidity. Bools are given special treatment.

Ok, the docs doesn’t explicitly state this rule. But in the compiler, any non-zero 256 bit word is defined as true. There’s no concept of “dirty higher order bits” for bool.

The compiler uses this definition to have a simplification rule that converts a iszero(iszero(b)) to b, if used in the condition for a JUMPI. Here, b can be any bool expression.

https://github.com/ethereum/solidity/blob/3edf91adc8eddd76fe4781698f15c6f1a8a8b355/libevmasm/PeepholeOptimiser.cpp#L294

Looks like the docs explicitly state you can't be sure upper bits will be zero.

"If you access variables of a type that spans less than 256 bits (for example uint64, address, or bytes16), you cannot make any assumptions about bits not part of the encoding of the type. Especially, do not assume them to be zero. To be safe, always clear the data properly before you use it in a context where this is important"

https://docs.soliditylang.org/en/latest/assembly.html#access-to-external-variables-functions-and-libraries

0xfoobar · 2023-08-10T13:51:18Z

Merging into branch on this repo so we can more easily push the latest nits

Vectorized added 2 commits August 7, 2023 14:53

Optimize RegistryHashes library

c9f3072

Add branchless operations

3032921

0xfoobar reviewed Aug 7, 2023

View reviewed changes

src/libraries/RegistryHashes.sol Show resolved Hide resolved

Optimize RegistryStorage

adf0d4b

Vectorized commented Aug 7, 2023

View reviewed changes

Vectorized added 4 commits August 7, 2023 17:27

Gas benchmarks

6a9cb05

Use back branching && in _validateDelegation

8f34062

Gas benchmarks

8f792be

Add memory-safe tags

cafa302

Vectorized and others added 9 commits August 7, 2023 19:03

Optimize hashes a bit more

2160282

Nit comment

c6852b0

Minor optimizations

2120c03

Merge branch 'v2' into v2-optimize

becd414

Minor optimizations

9cf1dbd

Merge branch 'v2-optimize' of https://github.com/Vectorized/delegate-…

21d7dbd

…registry into v2-optimize

Minor optimizations

765da84

Gas benchmarks

ce3b4b0

Merge branch 'v2' into v2-optimize

89f755c

0xfoobar reviewed Aug 7, 2023

View reviewed changes

src/DelegateRegistry.sol Outdated Show resolved Hide resolved

Nit typo

d357baf

mireynolds suggested changes Aug 8, 2023

View reviewed changes

Vectorized added 2 commits August 9, 2023 07:32

Fixes

a9dc73a

Add comments for max function

3562ecd

0xfoobar requested a review from mireynolds August 9, 2023 14:03

mireynolds suggested changes Aug 9, 2023

View reviewed changes

Vectorized added 2 commits August 9, 2023 17:09

Add truthyness demostrations

5ac544d

Add more truthyness demostrations

243f76d

Vectorized added 5 commits August 9, 2023 17:40

Fix

21cc5de

Fix

47c114e

Gas benchmark

67cef7d

fmt

6e25a49

Optimize Ops

1ff5448

0xfoobar reviewed Aug 10, 2023

View reviewed changes

0xfoobar changed the base branch from v2 to gasoptvect August 10, 2023 13:50

0xfoobar merged commit c52831d into delegatexyz:gasoptvect Aug 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V2 optimize #55

V2 optimize #55

Vectorized commented Aug 7, 2023 •

edited

Loading

0xfoobar commented Aug 7, 2023 •

edited

Loading

Vectorized Aug 7, 2023

0xfoobar commented Aug 7, 2023

0xfoobar commented Aug 7, 2023 •

edited

Loading

mireynolds commented Aug 8, 2023

mireynolds commented Aug 8, 2023

mireynolds left a comment

mireynolds Aug 8, 2023

mireynolds Aug 8, 2023

mireynolds Aug 8, 2023

mireynolds Aug 8, 2023

Vectorized commented Aug 9, 2023

mireynolds left a comment

mireynolds Aug 9, 2023

mireynolds Aug 9, 2023

mireynolds Aug 9, 2023

mireynolds Aug 9, 2023

Vectorized Aug 9, 2023

mireynolds Aug 9, 2023

0xfoobar commented Aug 9, 2023

0xfoobar Aug 10, 2023

0xfoobar Aug 10, 2023

Vectorized Aug 10, 2023 •

edited

Loading

Vectorized Aug 10, 2023 •

edited

Loading

0xfoobar Aug 10, 2023

0xfoobar commented Aug 10, 2023

V2 optimize #55

V2 optimize #55

Conversation

Vectorized commented Aug 7, 2023 • edited Loading

0xfoobar commented Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

0xfoobar commented Aug 7, 2023

0xfoobar commented Aug 7, 2023 • edited Loading

mireynolds commented Aug 8, 2023

mireynolds commented Aug 8, 2023

mireynolds 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

Vectorized commented Aug 9, 2023

mireynolds 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

0xfoobar commented Aug 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vectorized Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Vectorized Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xfoobar commented Aug 10, 2023

Vectorized commented Aug 7, 2023 •

edited

Loading

0xfoobar commented Aug 7, 2023 •

edited

Loading

0xfoobar commented Aug 7, 2023 •

edited

Loading

Vectorized Aug 10, 2023 •

edited

Loading

Vectorized Aug 10, 2023 •

edited

Loading