Array optimizations for ERC1155 #3940
Comments
|
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 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 |
|
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. 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. |
|
Hey, sorry por taking so long but here it is The tests I used is the gas-reporter already set in the repository I implemented your ideas in this commit here: Using the same tests in both versions |
|
@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. |
|
Nice, I`ve updated the other functions to use the _asSingletonArrays and the benchmark can be taken from this commit: RenanSouza2@a3598e4 And I also took the liberty to make this PR: #4196 |
|
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 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: the previous result can be checked on this commit. Following the discussed improvement regarding 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 ): And finally using both optimizations the results were (check it here.): Whit both changes the gas reduction is about 400 for |
|
@clauBv23 There is an existing PR for the array optimization. If you want you can submit a PR to use unsafeAccess. |
|
sure, perfect! #4300 created |
This PR refers to the
next-v5.0branch. In #3876 two potential optimizations were identified._asSingletonArray: Throughout the contract we use: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._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
_unsafeAccessfor memory arrays we should consider applying it in other placesThe text was updated successfully, but these errors were encountered: