Array optimizations for ERC1155

[frangio]

This PR refers to the next-v5.0 branch. In https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3876 two potential optimizations were identified.

1. _asSingletonArray: Throughout the contract we use:

        uint256[] memory ids = _asSingletonArray(id);
        uint256[] memory amounts = _asSingletonArray(amount);

We could combine this into a single function _asSigletonArrays(id, amount) and using assembly we would be able to do a single allocation and possibly avoid a couple of bounds checks.

2. _unsafeAccess: In places where we loop over arrays of ids and amounts, we don't need to do bounds checking inside the for loop, so we could define a function to skip it.

Given that these are arrays in memory, it's unclear how much of an improvement these optimizations will make, so we have to benchmark to know for sure.

Based on the results of using _unsafeAccess for memory arrays we should consider applying it in other places

[RenanSouza2]

Hey, I evaluated this and I don't think the array optimization is worth it

These are the gas evaluations before and after the change

safeTransferFrom(address,address,uint256,uint256,bytes) · 52739 · 60161 · 57220 safeTransferFrom(address,address,uint256,uint256,bytes) · 52562 · 59984 · 57043 · 18 · -

and at leaast the code I wrote was a bit hacky

function _asSingletonArrays(
    uint256 element1,
    uint element2
) private pure returns (uint256[] memory, uint256[] memory) {
    uint256[] memory array1 = new uint256[](3);
    uint256[] memory array2;
    assembly {
        mstore(array1, 1)
        mstore(add(array1, 0x20), element1)

        array2 := add(array1, 0x40)
        mstore(array2, 1)
        mstore(add(array2, 0x20), element2)
    }

    return (array1, array2);
}

edit: sorry for the bunch of edits I was trying to get the markdown right

[ernestognw]

Hey @RenanSouza2, just edited it to help you a bit with the code example, thanks!

Can you share your benchmark scripts? I can't interpret what the numbers you shared mean, is that a Foundry output?

Regarding the suggested implementation, I think it's more expensive because of the way variables are declared, we should be able to do the same in pure assembly by just returning a pointer without allocating memory (eg. uint256[] memory array1 = new uint256[](3) is already allocating it and performing other operations).

We haven't looked into this yet since we're going through an audit for the next 4.9 release. We're considering this for 5.0 later this year.

[RenanSouza2]

Hey, sorry por taking so long but here it is

The tests I used is the gas-reporter already set in the repository in this commit I selected a few tests: https://github.com/RenanSouza2/openzeppelin-contracts/commit/d43ff2d932294b8e20fdd2a4d07aa7705cb77f22

I implemented your ideas in this commit here: https://github.com/RenanSouza2/openzeppelin-contracts/commit/1b3190bbd3f43fb7acdb39af012d15cc65475ac3

Using the same tests in both versions (I'm trying to fing the best way to share a table with you)

·-------------------------------------------|---------------------------|-------------|
|           Solc version: 0.8.13            ·  Optimizer enabled: true  ·  Runs: 200  ·
············································|···························|·············|
|  Methods                                                                             
························|···················|········|········|·········|·············|
|  Contract             ·  Method           ·  Min   ·  Max   ·   Avg   ·     # calls ·
························|···················|········|········|·········|·············|
|  $ERC1155 (original)  ·  safeTransferFrom · 52739  · 60161  ·  56964  ·         20  ·
························|···················|········|········|·········|·············|
|  $ERC1155 (assembly)  ·  safeTransferFrom · 52478  · 59900  ·  56703  ·         20  ·
························|···················|········|········|·········|·············|

[frangio]

@RenanSouza2 Thanks for running these benchmarks! The code you shared is what I had in mind.

So it looks like this change results in about 250 gas reduction. It's not huge, in relative terms it's less than 1% of execution cost, but in absolute terms it's not negligible. My opinion is we should do this optimization.

[RenanSouza2]

Nice,

I`ve updated the other functions to use the _asSingletonArrays and the benchmark can be taken from this commit: https://github.com/RenanSouza2/openzeppelin-contracts/commit/a3598e4af0b866ad1631dcfa3f8b4a7dc12cc8dd

And I also took the liberty to make this PR: https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4196

[clauBv23]

Hi there, I was taking a look at the potential optimizations and I'm not sure if the improvements are worth it, but here go the results I found.

Regarding the usage of _unsafeAccess, using the next function for unsafe access:

    function unsafeMemoryAccess(uint256[] memory arr, uint256 pos) internal pure returns (uint256 res) {
        /// @solidity memory-safe-assembly
        assembly {
            res := mload(add(add(arr, 0x20), mul(pos, 0x20)))
        }
    }

the results were:

|               Solc version: 0.8.19               ·  Optimizer enabled: true  ·  Runs: 200  ·
···················································|···························|·············|
|  Methods                                         ·               27 gwei/gas               ·
··························|························|·············|·············|·············|
|  Contract               ·  Method                ·      Min    ·      Max    ·      Avg    ·
··························|························|·············|·············|·············|
|  $ERC1155               ·  safeBatchTransferFrom ·      79580  ·     108008  ·      90701  ·
··························|························|·············|·············|·············|
|  $ERC1155 (optimized)   ·  safeBatchTransferFrom ·      79404  ·     107744  ·      90509  ·
··························|························|·············|·············|·············|

|  $ERC1155               ·  safeTransferFrom      ·      52744  ·      60144  ·      56678  ·
··························|······················ ·|·············|·············|·············|
|  $ERC1155 (optimized)   ·  safeTransferFrom      ·      52574  ·      59974  ·      56508  ·
··························|······················· |·············|·············|·············|

the previous result can be checked on this commit.

Following the discussed improvement regarding _asSigletonArrays and using this function:

function _asSingletonArrays(
        uint256 element1,
        uint256 element2
    ) private pure returns (uint256[] memory array1, uint256[] memory array2) {
        /// @solidity memory-safe-assembly
        assembly {
            array1 := mload(0x40)
            mstore(array1, 1)
            mstore(add(array1, 0x20), element1)

            array2 := add(array1, 0x40)
            mstore(array2, 1)
            mstore(add(array2, 0x20), element2)

            mstore(0x40, add(array2, 0x40))
        }
    }

the results were (check it on this commit ):

·--------------------------------------------------|---------------------------|-------------|
|               Solc version: 0.8.19               ·  Optimizer enabled: true  ·  Runs: 200  ·
···················································|···························|·············|
|  Methods                                         ·               27 gwei/gas               ·
··························|························|·············|·············|·············|
|  Contract               ·  Method                ·      Min    ·      Max    ·      Avg    ·
··························|························|·············|·············|·············|
|  $ERC1155               ·  safeTransferFrom      ·      52744  ·      60144  ·      56678  ·
··························|······················ ·|·············|·············|·············|
|  $ERC1155 (optimized)   ·  safeTransferFrom      ·      52420  ·      59820  ·      56354  ·
··························|······················· |·············|·············|·············|

And finally using both optimizations the results were (check it here.):

·--------------------------------------------------|---------------------------|-------------|
|               Solc version: 0.8.19               ·  Optimizer enabled: true  ·  Runs: 200  ·
···················································|···························|·············|
|  Methods                                         ·               27 gwei/gas               ·
··························|························|·············|·············|·············|
|  Contract               ·  Method                ·      Min    ·      Max    ·      Avg    ·
··························|························|·············|·············|·············|
|  $ERC1155               ·  safeBatchTransferFrom ·      79580  ·     108008  ·      90701  ·
··························|························|·············|·············|·············|
|  $ERC1155 (optimized)   ·  safeBatchTransferFrom ·      79404  ·     107744  ·      90509  ·
··························|························|·············|·············|·············|

|  $ERC1155               ·  safeTransferFrom      ·      52744  ·      60144  ·      56678  ·
··························|······················ ·|·············|·············|·············|
|  $ERC1155 (optimized)   ·  safeTransferFrom      ·      52250  ·      59650  ·      56184  ·
··························|······················· |·············|·············|·············|

Whit both changes the gas reduction is about 400 for safeTransferFrom and more than 150 for safeBatchTransferFrom so, IMHO it could be, at least, considered. Didn't create a PR to keep OZ repo clean, but I could if this is a green light.

[frangio]

@clauBv23 There is an existing PR for the array optimization. If you want you can submit a PR to use unsafeAccess.

[clauBv23]

sure, perfect! #4300 created