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

Add log2, log10 and log256 functions #3670

Merged
merged 9 commits into from
Sep 7, 2022

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Sep 1, 2022

Logs are already used in different places:

  • in Math.sqrt, we do a log in base 4 (log2 divided by 2)
  • in String.toString(uint256), we do a log base 10
  • in String.toHexString(uint256), we do a log base 256

In #3573, it was mentioned that these operation should all be in math, rather then scattered all over the code base. This rationalizes the code, and allow reuse by devs.

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

*/
function log10(uint256 value) internal pure returns (uint256) {
uint256 result = 0;
if (value >= 10**64) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Amxx you can replace >= by > and save more gas.

Example:

value >= 10**64 == value > 10**64-1

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Amxx what about this one?

Copy link
Collaborator Author

@Amxx Amxx Sep 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested both, and this change makes it more expensive :/

it looks like the compiler does the -1 operation at runtime

if (x >> 2 > 0) {
result <<= 1;
}
uint256 result = 1 << (log2(a) / 2);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can replace 1 << (log2(a) / 2) by 1 << (log2(a) >> 2).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is interresting, but the correct formula would be 1 << (log2(a) >> 1)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, sorry. It >>1.

Copy link
Contributor

@frangio frangio Sep 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before changing that I would check if Solidity does not make that optimization on its own.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested both. using >> 1 saves 54 gas compared to / 2
(0.8.16 with optimizations)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Amxx Because It happen due to solidity checking underflow that. But When we are use shift operator it bypass that check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overflow check make sens for +, - and * ... not for /
The only reasonable check is that we don't divide by 0 ... but there is no reason to check that here.

@frangio
Copy link
Contributor

frangio commented Sep 3, 2022

Shouldn't log256 be log16? 🤔

Edit: Just realized that toHexString indeed uses log256.

@Amxx
Copy link
Collaborator Author

Amxx commented Sep 4, 2022

Shouldn't log256 be log16? thinking

Edit: Just realized that toHexString indeed uses log256.

Yes, hex chars go by pairs.

@Amxx Amxx marked this pull request as ready for review September 6, 2022 19:33
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Looking forward to fuzzing too.

@Amxx Amxx enabled auto-merge (squash) September 7, 2022 08:29
@Amxx Amxx merged commit c1d6e39 into OpenZeppelin:master Sep 7, 2022
@Amxx Amxx deleted the feature/math/logX branch September 7, 2022 08:29
ronhuafeng added a commit to ronhuafeng/openzeppelin-contracts that referenced this pull request Sep 9, 2022
JulissaDantes pushed a commit to JulissaDantes/openzeppelin-contracts that referenced this pull request Nov 3, 2022
JulissaDantes pushed a commit to JulissaDantes/openzeppelin-contracts that referenced this pull request Nov 4, 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