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 all 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, μ: 12592434, ~: 12592620)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 405.76ms
| src/DelegateRegistry.sol:DelegateRegistry contract | | | | | |
|----------------------------------------------------|-----------------|--------|--------|--------|---------|
| Deployment Cost | Deployment Size | | | | |
| 2124658 | 10644 | | | | |
| 1800698 | 9026 | | | | |
| 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 | 2847 | 3016 | 3016 | 3186 | 2 |
| checkDelegateForContract | 5276 | 5627 | 5627 | 5979 | 2 |
| checkDelegateForERC1155 | 7707 | 8280 | 8280 | 8854 | 2 |
| checkDelegateForERC20 | 7646 | 8211 | 8211 | 8777 | 2 |
| checkDelegateForERC721 | 7736 | 8283 | 8283 | 8830 | 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 44.46ms
| 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
48 changes: 37 additions & 11 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 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, iszero(iszero(valid)))
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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, iszero(iszero(valid)))
Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

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, iszero(iszero(valid)))
Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

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 Down Expand Up @@ -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 Down Expand Up @@ -384,7 +405,12 @@ contract DelegateRegistry is IDelegateRegistry {
}

/// @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) {
uint256 loaded = uint256(uint160(_loadFrom(location, Storage.Positions.firstPacked)));
uint256 revoked = uint256(uint160(DELEGATION_REVOKED));
uint256 fromCasted = uint256(uint160(from));
assembly {
result := and(eq(fromCasted, loaded), gt(fromCasted, revoked))
}
mireynolds marked this conversation as resolved.
Show resolved Hide resolved
}
}
Loading