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.

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

packedOwnership as lengthless array #394

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sillytuna
Copy link

_packedOwnership access was performing a sha3 op for every new index but as these are consecutive they can be stored as a regular array offset from a base storage location irrespective of the start token id. Only one sha3 is required instead of one per index used. Solidity effectively uses this technique for its arrays but also stores array length, which we don't need to do.

Optimised _packedOwnershipOf accordingly.

Pros
Reduces gas usage slightly in common functions but more significantly in bulk transfers (figures below).

Cons
22,772 additional deployment cost
Code readability

New

·····································|·························|·············|·············|·············|···············|··············
|  ERC721AGasReporterMock            ·  transferFrom           ·      44232  ·      85751  ·      54612  ·            4  ·          -  │
·····································|·························|·············|·············|·············|···············|··············
|  ERC721AGasReporterMock            ·  transferTenAsc         ·          -  ·          -  ·     295158  ·            1  ·          -  │
·····································|·························|·············|·············|·············|···············|··············
|  ERC721AGasReporterMock            ·  transferTenAvg         ·          -  ·          -  ·     295651  ·            1  ·          -  │
·····································|·························|·············|·············|·············|···············|··············
|  ERC721AGasReporterMock            ·  transferTenDesc        ·          -  ·          -  ·     299608  ·            1  ·          -  │
·····································|·························|·············|·············|·············|···············|··············

Previous

·····································|·························|·············|·············|·············|···············|··············
|  ERC721AGasReporterMock            ·  transferFrom           ·      44338  ·      86037  ·      54763  ·            4  ·          -  │
·····································|·························|·············|·············|·············|···············|··············
|  ERC721AGasReporterMock            ·  transferTenAsc         ·          -  ·          -  ·     297319  ·            1  ·          -  │
·····································|·························|·············|·············|·············|···············|··············
|  ERC721AGasReporterMock            ·  transferTenAvg         ·          -  ·          -  ·     298166  ·            1  ·          -  │
·····································|·························|·············|·············|·············|···············|··············
|  ERC721AGasReporterMock            ·  transferTenDesc        ·          -  ·          -  ·     303983  ·            1  ·          -  │
·····································|·························|·············|·············|·············|···············|··············

Reduces gas usage by avoiding repeated sha3 ops.
@Vectorized
Copy link
Collaborator

Vectorized commented Jul 28, 2022

O.O

This may require a major version bump, cuz the upgradeable repo requires the storage slots to stay the same.

@sillytuna
Copy link
Author

Understand about the storage slot situ.

I've got another optimisation I need to experiment with which may make this less important (tho it'll still save gas). Will check tomorrow. It would also affect storage slots though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants