search

estarriolvetch / ERC721Psi

MIT License
117 stars 29 forks source link

using Solady LibBitmap speeds burn by several hunded (~500 avg) gas #17

Open nidhhoggr opened 1 year ago

nidhhoggr commented 1 year ago

https://github.com/nidhhoggr/ERC721Psi/commit/9b7a15fd2a0cf4c4392a26642370d5929ceadfa5

estarriolvetch commented 1 year ago

Solady's bitmap uses different ordering (LSB -> MSB) than solidity-bits (MSB -> LSB).

This will be a breaking change (especially to upgradeable contracts). I will be happy to include that in the next major version changes.

estarriolvetch commented 1 year ago

btw I like how you generate the gas report. lol

Do you generate them with commands? I'm thinking of adding them to the CI and storing the reports as artifacts.

estarriolvetch commented 1 year ago

I created a new branch for breaking changes.

https://github.com/estarriolvetch/ERC721Psi/tree/v0.8

nidhhoggr commented 1 year ago

Hello @estarriolvetch ,

That's interesting because after I swapped SolidityBits.scanForward with SoladyLibBitmap.findLastSet all the test are still passing.

Here is the code for reference:
https://github.com/Vectorized/solady/blob/main/src/utils/LibBitmap.sol#L171

I'm not sure what the difference is between the ERC721Psi and ERC721PsiUpgradeable, all of the _batchHead BitMap operations seems to be the same.

For the reports I added the following bashscript:

npx hardhat run scripts/benchmark_mint.js > reports/.benchmark_mint
npx hardhat run scripts/benchmark_safemint.js > reports/.benchmark_safemint
npx hardhat run scripts/benchmark_burn.js > reports/.benchmark_burn
npx hardhat run scripts/benchmark_ownerOf.js > reports/.benchmark_ownerOf
npx hardhat run scripts/benchmark_transfer.js > reports/.benchmark_transfer
npx hardhat run scripts/benchmark_addressData.js > reports/.benchmark_addressData
REPORT_GAS=true REPORT_GAS_FILE=./reports/.hardhat_gas_report npx hardhat test

Note: I did install hardhat-gas-reporter reported for the last command with the folliwing in hardhat config

  gasReporter: {
    enabled: (process.env.REPORT_GAS) ? true : false,
    outputFile: process.env.REPORT_GAS_FILE
  },
estarriolvetch commented 1 year ago

Yes, the test will pass. The problem will occur if the contract is deployed with solidity-bits version and later upgraded to the solady version.

Freshly deployed contracts are fine.

nidhhoggr commented 1 year ago

Okay, that makes sense.

estarriolvetch commented 1 year ago

@nidhhoggr Do you still plan to add this to v0.8?

nidhhoggr commented 1 year ago

Yes, hopefully I can get to that by the end of the weekend. Should be a pretty simple fix. In the meantime, did you look further into any way to start artifacting gas reports so we have commit-based benchmarks for optimizations? It's helpful to see exactly how a specific feature increases or decreases performance.

nidhhoggr commented 1 year ago

As mentioned in the PR #32 , these is the associated Benchmark comparison: https://github.com/nidhhoggr/ERC721PsiReports/commit/522518ad7d9b6838fab5ee9d8089a8e38ace7e29