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

Reduce gas in log256 #3745

Merged
merged 2 commits into from
Oct 5, 2022
Merged

Conversation

TomAFrench
Copy link
Contributor

@TomAFrench TomAFrench commented Sep 30, 2022

This PR replaces a multiplication by 8 inside log256(uint256, Rounding) with shifting the bits by 3. There's other usages of multiplication/division through bitshifts in the same file so I don't think this is an example of being overly "optimizooor".

PR Checklist

The change is covered by the existing tests and this changes doesn't warrant docs.

  • Tests
  • Documentation
  • Changelog entry

@frangio
Copy link
Contributor

frangio commented Sep 30, 2022

Do you know how much gas this saves? Waiting to see what our gas reports say.

Regardless, it looks good to me. I would put this on 4.8 (currently in release candidate, feature frozen). @Amxx would you agree?

@TomAFrench
Copy link
Contributor Author

A MUL is 5 gas whereas SHL is 3 so we should get a saving of 2 gas. If I run forge snapshot I get exactly that saving in practice as well.

@Amxx
Copy link
Collaborator

Amxx commented Sep 30, 2022

I'm usually against adding non essential thing in the middle of a rc ... and saving 2 gas really doesn't feel essential.

However, we already have a rc update planned, so if you feel like taking this opportunity and adding it, I wouldn't oppose it.

@frangio frangio changed the title Mild gas golfing in log256 Reduce gas in log256 Oct 5, 2022
@frangio frangio merged commit 34e5863 into OpenZeppelin:master Oct 5, 2022
@gitpoap-bot
Copy link

gitpoap-bot bot commented Oct 5, 2022

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2022 OpenZeppelin Contracts Contributor:

GitPOAP: 2022 OpenZeppelin Contracts Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

@TomAFrench TomAFrench deleted the gas-golf-log256 branch October 7, 2022 02:45
JulissaDantes pushed a commit to JulissaDantes/openzeppelin-contracts that referenced this pull request Nov 3, 2022
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.

3 participants