Skip to content
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.

Sign up for GitHub

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

V2 optimize #55

Merged
merged 26 commits into from
Aug 10, 2023
Merged

V2 optimize #55

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
c9f3072
Optimize RegistryHashes library
Vectorized Aug 7, 2023
3032921
Add branchless operations
Vectorized Aug 7, 2023
adf0d4b
Optimize RegistryStorage
Vectorized Aug 7, 2023
6a9cb05
Gas benchmarks
Vectorized Aug 7, 2023
8f34062
Use back branching && in _validateDelegation
Vectorized Aug 7, 2023
8f792be
Gas benchmarks
Vectorized Aug 7, 2023
cafa302
Add memory-safe tags
Vectorized Aug 7, 2023
2160282
Optimize hashes a bit more
Vectorized Aug 7, 2023
c6852b0
Nit comment
Vectorized Aug 7, 2023
2120c03
Minor optimizations
Vectorized Aug 7, 2023
becd414
Merge branch 'v2' into v2-optimize
0xfoobar Aug 7, 2023
9cf1dbd
Minor optimizations
Vectorized Aug 7, 2023
21d7dbd
Merge branch 'v2-optimize' of https://github.com/Vectorized/delegate-…
Vectorized Aug 7, 2023
765da84
Minor optimizations
Vectorized Aug 7, 2023
ce3b4b0
Gas benchmarks
Vectorized Aug 7, 2023
89f755c
Merge branch 'v2' into v2-optimize
0xfoobar Aug 7, 2023
d357baf
Nit typo
Vectorized Aug 7, 2023
a9dc73a
Fixes
Vectorized Aug 9, 2023
3562ecd
Add comments for max function
Vectorized Aug 9, 2023
5ac544d
Add truthyness demostrations
Vectorized Aug 9, 2023
243f76d
Add more truthyness demostrations
Vectorized Aug 9, 2023
21cc5de
Fix
Vectorized Aug 9, 2023
47c114e
Fix
Vectorized Aug 9, 2023
67cef7d
Gas benchmark
Vectorized Aug 9, 2023
6e25a49
fmt
Vectorized Aug 9, 2023
1ff5448
Optimize Ops
Vectorized Aug 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions gasbenchmark10mil
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,24 @@
No files changed, compilation skipped

Running 1 test for test/GasBenchmark.t.sol:GasBenchmark
[PASS] testGas(address,bytes32) (runs: 256, μ: 14540532, ~: 14540813)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 289.94ms
[PASS] testGas(address,bytes32) (runs: 256, μ: 12490859, ~: 12491136)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 394.77ms
| src/DelegateRegistry.sol:DelegateRegistry contract | | | | | |
|----------------------------------------------------|-----------------|--------|--------|--------|---------|
| Deployment Cost | Deployment Size | | | | |
| 2124658 | 10644 | | | | |
| 1783877 | 8942 | | | | |
| Function Name | min | avg | median | max | # calls |
| checkDelegateForAll | 2980 | 3183 | 3183 | 3386 | 2 |
| checkDelegateForContract | 5473 | 5895 | 5895 | 6317 | 2 |
| checkDelegateForERC1155 | 7990 | 8685 | 8685 | 9380 | 2 |
| checkDelegateForERC20 | 7927 | 8610 | 8610 | 9293 | 2 |
| checkDelegateForERC721 | 7991 | 8644 | 8644 | 9297 | 2 |
| delegateAll | 135861 | 135861 | 135861 | 135861 | 2 |
| delegateContract | 114469 | 125419 | 125419 | 136369 | 2 |
| delegateERC1155 | 159393 | 170343 | 170343 | 181293 | 2 |
| delegateERC20 | 136942 | 147892 | 147892 | 158842 | 2 |
| delegateERC721 | 136936 | 147886 | 147886 | 158836 | 2 |
| multicall | 689696 | 689696 | 689696 | 689696 | 1 |
| checkDelegateForAll | 2823 | 2986 | 2986 | 3150 | 2 |
| checkDelegateForContract | 5240 | 5579 | 5579 | 5919 | 2 |
| checkDelegateForERC1155 | 7671 | 8226 | 8226 | 8782 | 2 |
| checkDelegateForERC20 | 7610 | 8157 | 8157 | 8705 | 2 |
| checkDelegateForERC721 | 7688 | 8217 | 8217 | 8746 | 2 |
| delegateAll | 135880 | 135880 | 135880 | 135880 | 2 |
| delegateContract | 114464 | 125414 | 125414 | 136364 | 2 |
| delegateERC1155 | 159363 | 170313 | 170313 | 181263 | 2 |
| delegateERC20 | 136903 | 147853 | 147853 | 158803 | 2 |
| delegateERC721 | 136903 | 147853 | 147853 | 158803 | 2 |
| multicall | 689608 | 689608 | 689608 | 689608 | 1 |



Expand Down
26 changes: 13 additions & 13 deletions hashbenchmark10mil
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,24 @@
No files changed, compilation skipped

Running 1 test for test/HashBenchmark.t.sol:HashBenchmark
[PASS] testHashGas(address,bytes32,address,uint256,address,bytes32) (runs: 256, μ: 20187, ~: 20187)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 26.78ms
[PASS] testHashGas(address,bytes32,address,uint256,address,bytes32) (runs: 256, μ: 19737, ~: 19737)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 54.85ms
| test/HashBenchmark.t.sol:HashHarness contract | | | | | |
|-----------------------------------------------|-----------------|-----|--------|-----|---------|
| Deployment Cost | Deployment Size | | | | |
| 396030 | 2010 | | | | |
| 275918 | 1410 | | | | |
| Function Name | min | avg | median | max | # calls |
| allHash | 679 | 679 | 679 | 679 | 1 |
| allLocation | 724 | 724 | 724 | 724 | 1 |
| contractHash | 813 | 813 | 813 | 813 | 1 |
| contractLocation | 851 | 851 | 851 | 851 | 1 |
| allHash | 666 | 666 | 666 | 666 | 1 |
| allLocation | 694 | 694 | 694 | 694 | 1 |
| contractHash | 759 | 759 | 759 | 759 | 1 |
| contractLocation | 797 | 797 | 797 | 797 | 1 |
| decodeType | 371 | 371 | 371 | 371 | 1 |
| erc1155Hash | 821 | 821 | 821 | 821 | 1 |
| erc1155Location | 920 | 920 | 920 | 920 | 1 |
| erc20Hash | 792 | 792 | 792 | 792 | 1 |
| erc20Location | 831 | 831 | 831 | 831 | 1 |
| erc721Hash | 866 | 866 | 866 | 866 | 1 |
| erc721Location | 888 | 888 | 888 | 888 | 1 |
| erc1155Hash | 776 | 776 | 776 | 776 | 1 |
| erc1155Location | 875 | 875 | 875 | 875 | 1 |
| erc20Hash | 738 | 738 | 738 | 738 | 1 |
| erc20Location | 766 | 766 | 766 | 766 | 1 |
| erc721Hash | 821 | 821 | 821 | 821 | 1 |
| erc721Location | 843 | 843 | 843 | 843 | 1 |
| location | 384 | 384 | 384 | 384 | 1 |


Expand Down
69 changes: 48 additions & 21 deletions src/DelegateRegistry.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pragma solidity ^0.8.21;
import {IDelegateRegistry as IDelegateRegistry} from "./IDelegateRegistry.sol";
import {RegistryHashes as Hashes} from "./libraries/RegistryHashes.sol";
import {RegistryStorage as Storage} from "./libraries/RegistryStorage.sol";
import {RegistryOps as Ops} from "./libraries/RegistryOps.sol";

/**
* @title DelegateRegistry
Expand All @@ -19,7 +20,7 @@ contract DelegateRegistry is IDelegateRegistry {
/// @dev Vault delegation enumeration outbox, for pushing new hashes only
mapping(address from => bytes32[] delegationHashes) internal outgoingDelegationHashes;

/// @dev Delegate enumeration inbox, for pushing new hashes only
/// @dev Delegate delegation enumeration inbox, for pushing new hashes only
Vectorized marked this conversation as resolved.
Show resolved Hide resolved
mapping(address to => bytes32[] delegationHashes) internal incomingDelegationHashes;

/// @dev Standardizes from storage flags to prevent double-writes in the delegation in/outbox if the same delegation is revoked and rewritten
Expand Down Expand Up @@ -133,25 +134,37 @@ contract DelegateRegistry is IDelegateRegistry {
/// @inheritdoc IDelegateRegistry
function checkDelegateForAll(address to, address from, bytes32 rights) external view override returns (bool valid) {
valid = _validateDelegation(Hashes.allLocation(from, "", to), from);
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, valid)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to use the scratch space to return the bool here. But valid needs to be cleaned. Types that that span less than 256 bits cannot be assumed to have a particular encoding and need to be cleaned or checked before using them in an assembly block.

return(0x00, 0x20) // Direct return. Skips Solidity's redundant copying to save gas.
}
}

/// @inheritdoc IDelegateRegistry
function checkDelegateForContract(address to, address from, address contract_, bytes32 rights) external view override returns (bool valid) {
valid = _validateDelegation(Hashes.allLocation(from, "", to), from) || _validateDelegation(Hashes.contractLocation(from, "", to, contract_), from);
if (rights != "" && !valid) {
if (!Ops.or(rights == "", valid)) {
valid = _validateDelegation(Hashes.allLocation(from, rights, to), from) || _validateDelegation(Hashes.contractLocation(from, rights, to, contract_), from);
}
assembly ("memory-safe") {
mstore(0x00, valid)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, can't assume the bits are clean.

return(0x00, 0x20) // Direct return. Skips Solidity's redundant copying to save gas.
}
}

/// @inheritdoc IDelegateRegistry
function checkDelegateForERC721(address to, address from, address contract_, uint256 tokenId, bytes32 rights) external view override returns (bool valid) {
valid = _validateDelegation(Hashes.allLocation(from, "", to), from) || _validateDelegation(Hashes.contractLocation(from, "", to, contract_), from)
|| _validateDelegation(Hashes.erc721Location(from, "", to, tokenId, contract_), from);
if (rights != "" && !valid) {
if (!Ops.or(rights == "", valid)) {
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, assumption of clean bits.

return(0x00, 0x20) // Direct return. Skips Solidity's redundant copying to save gas.
}
}

/// @inheritdoc IDelegateRegistry
Expand All @@ -160,12 +173,16 @@ contract DelegateRegistry is IDelegateRegistry {
amount = (_validateDelegation(Hashes.allLocation(from, "", to), from) || _validateDelegation(Hashes.contractLocation(from, "", to, contract_), from))
? type(uint256).max
: (_validateDelegation(location, from) ? _loadDelegationUint(location, Storage.Positions.amount) : 0);
if (rights != "" && amount != type(uint256).max) {
if (!Ops.or(rights == "", amount == type(uint256).max)) {
location = Hashes.erc20Location(from, rights, to, contract_);
uint256 rightsBalance = (
_validateDelegation(Hashes.allLocation(from, rights, to), from) || _validateDelegation(Hashes.contractLocation(from, rights, to, contract_), from)
) ? type(uint256).max : (_validateDelegation(location, from) ? _loadDelegationUint(location, Storage.Positions.amount) : 0);
amount = rightsBalance > amount ? rightsBalance : amount;
amount = Ops.max(rightsBalance, amount);
}
assembly ("memory-safe") {
mstore(0x00, amount)
return(0x00, 0x20) // Direct return. Skips Solidity's redundant copying to save gas.
}
}

Expand All @@ -175,12 +192,16 @@ contract DelegateRegistry is IDelegateRegistry {
amount = (_validateDelegation(Hashes.allLocation(from, "", to), from) || _validateDelegation(Hashes.contractLocation(from, "", to, contract_), from))
? type(uint256).max
: (_validateDelegation(location, from) ? _loadDelegationUint(location, Storage.Positions.amount) : 0);
if (rights != "" && amount != type(uint256).max) {
if (!Ops.or(rights == "", amount == type(uint256).max)) {
location = Hashes.erc1155Location(from, rights, to, tokenId, contract_);
uint256 rightsBalance = (
_validateDelegation(Hashes.allLocation(from, rights, to), from) || _validateDelegation(Hashes.contractLocation(from, rights, to, contract_), from)
) ? type(uint256).max : (_validateDelegation(location, from) ? _loadDelegationUint(location, Storage.Positions.amount) : 0);
amount = rightsBalance > amount ? rightsBalance : amount;
amount = Ops.max(rightsBalance, amount);
}
assembly ("memory-safe") {
mstore(0x00, amount)
return(0x00, 0x20) // Direct return. Skips Solidity's redundant copying to save gas.
}
}

Expand Down Expand Up @@ -215,7 +236,7 @@ contract DelegateRegistry is IDelegateRegistry {
for (uint256 i = 0; i < hashes.length; ++i) {
bytes32 location = Hashes.location(hashes[i]);
address from = _loadFrom(location, Storage.Positions.firstPacked);
if (from == DELEGATION_EMPTY || from == DELEGATION_REVOKED) {
if (Ops.or(from == DELEGATION_EMPTY, from == DELEGATION_REVOKED)) {
delegations_[i] = Delegation({type_: DelegationType.NONE, to: address(0), from: address(0), rights: "", amount: 0, contract_: address(0), tokenId: 0});
} else {
(, address to, address contract_) = _loadDelegationAddresses(location, Storage.Positions.firstPacked, Storage.Positions.secondPacked);
Expand All @@ -238,15 +259,15 @@ contract DelegateRegistry is IDelegateRegistry {
*/

function readSlot(bytes32 location) external view returns (bytes32 contents) {
assembly {
assembly ("memory-safe") {
contents := sload(location)
}
}

function readSlots(bytes32[] calldata locations) external view returns (bytes32[] memory contents) {
uint256 length = locations.length;
contents = new bytes32[](length);
assembly {
assembly ("memory-safe") {
for { let i := 0 } lt(i, length) { i := add(i, 1) } { mstore(add(contents, mul(add(i, 1), 32)), sload(calldataload(add(68, mul(i, 32))))) }
}
}
Expand All @@ -259,7 +280,7 @@ contract DelegateRegistry is IDelegateRegistry {
/// @param interfaceId The interface identifier
/// @return valid Whether the queried interface is supported
function supportsInterface(bytes4 interfaceId) external pure returns (bool) {
return interfaceId == type(IDelegateRegistry).interfaceId || interfaceId == 0x01ffc9a7;
return Ops.or(interfaceId == type(IDelegateRegistry).interfaceId, interfaceId == 0x01ffc9a7);
}

/**
Expand All @@ -274,14 +295,14 @@ contract DelegateRegistry is IDelegateRegistry {

/// @dev Helper function that writes bytes32 data to delegation data location at array position
function _writeDelegation(bytes32 location, Storage.Positions position, bytes32 data) internal {
assembly {
assembly ("memory-safe") {
sstore(add(location, position), data)
}
}

/// @dev Helper function that writes uint256 data to delegation data location at array position
function _writeDelegation(bytes32 location, Storage.Positions position, uint256 data) internal {
assembly {
assembly ("memory-safe") {
sstore(add(location, position), data)
}
}
Expand All @@ -291,7 +312,7 @@ contract DelegateRegistry is IDelegateRegistry {
internal
{
(bytes32 firstSlot, bytes32 secondSlot) = Storage.packAddresses(from, to, contract_);
assembly {
assembly ("memory-safe") {
sstore(add(location, firstPacked), firstSlot)
sstore(add(location, secondPacked), secondSlot)
}
Expand Down Expand Up @@ -347,22 +368,22 @@ contract DelegateRegistry is IDelegateRegistry {

/// @dev Helper function that loads delegation data from a particular array position and returns as bytes32
function _loadDelegationBytes32(bytes32 location, Storage.Positions position) internal view returns (bytes32 data) {
assembly {
assembly ("memory-safe") {
data := sload(add(location, position))
}
}

/// @dev Helper function that loads delegation data from a particular array position and returns as uint256
function _loadDelegationUint(bytes32 location, Storage.Positions position) internal view returns (uint256 data) {
assembly {
assembly ("memory-safe") {
data := sload(add(location, position))
}
}

// @dev Helper function that loads the from address from storage according to the packing rule for delegation storage
function _loadFrom(bytes32 location, Storage.Positions firstPacked) internal view returns (address) {
bytes32 data;
assembly {
assembly ("memory-safe") {
data := sload(add(location, firstPacked))
}
return Storage.unpackAddress(data);
Expand All @@ -376,15 +397,21 @@ contract DelegateRegistry is IDelegateRegistry {
{
bytes32 firstSlot;
bytes32 secondSlot;
assembly {
assembly ("memory-safe") {
firstSlot := sload(add(location, firstPacked))
secondSlot := sload(add(location, secondPacked))
}
(from, to, contract_) = Storage.unpackAddresses(firstSlot, secondSlot);
}

/// @dev Helper function to establish whether a delegation is enabled
function _validateDelegation(bytes32 location, address from) internal view returns (bool) {
return (_loadFrom(location, Storage.Positions.firstPacked) == from && from > DELEGATION_REVOKED);
function _validateDelegation(bytes32 location, address from) internal view returns (bool result) {
address loaded = _loadFrom(location, Storage.Positions.firstPacked);
address revoked = DELEGATION_REVOKED;
assembly ("memory-safe") {
// We don't need to clean the upper 96 bits of `from`. It is directly passed in from the
// parameter of an external function, and Solidity has included checks to ensure that it is clean.
result := and(eq(from, loaded), gt(from, revoked))
}
mireynolds marked this conversation as resolved.
Show resolved Hide resolved
}
}
Loading