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

Optimize Math.max and SignedMath.max #3679

Merged
merged 2 commits into from
Sep 5, 2022
Merged

Conversation

gzliudan
Copy link
Contributor

@gzliudan gzliudan commented Sep 5, 2022

In Solidity, there is no single op-code for <= or >= expressions. Solidity compiler executes the LT/GT (less than/greater than) op-code and afterwards it executes an ISZERO op-code to check if the result of the previous comparison (LT/ GT) is zero and validate it or not. So the use of < and > are cheaper than <= and >=. Below is current version of max function:

    function max(uint256 a, uint256 b) internal pure returns (uint256) {
        return a >= b ? a : b;
    }

The above code works as below steps:

    function max(uint256 a, uint256 b) internal pure returns (uint256) {
        if (a > b) {
            return a;
        } else if (a == b) {
            return a;
        } else {
            return b;
        }
    }

If we use the new version:

    function max(uint256 a, uint256 b) internal pure returns (uint256) {
        return a > b ? a : b;
    }

Then the function will works as below steps:

    function max(uint256 a, uint256 b) internal pure returns (uint256) {
        if (a > b) {
            return a;
        } else {
            return b;
        }
    }

So the step of check a == b is omitted.

@Amxx
Copy link
Collaborator

Amxx commented Sep 5, 2022

Nice find @gzliudan

I was hopping the compiler would translate a >= b as !(a<b) and you "just" pay for the negation part (with can be done using isZero).

Still, that is a nice optimization!

Can you add a small changelog entry ?

@gzliudan
Copy link
Contributor Author

gzliudan commented Sep 5, 2022

Ok, I added a new entry to changelog.

@frangio frangio changed the title optimize function max of library Math and SignedMath Optimize Math.max and SignedMath.max Sep 5, 2022
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.

It's not even necessary to do the negation. The compiler should be able to convert a >= b ? a : b into a < b ? b : a... Indeed, compiling with viaIR: true this optimization is done automatically.

Reluctantly approving this PR because these peephole optimizations are the job of the compiler.

@frangio frangio merged commit 005a35b into OpenZeppelin:master Sep 5, 2022
@gitpoap-bot
Copy link

gitpoap-bot bot commented Sep 5, 2022

Woohoo, 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 on over to GitPOAP.io and connect your GitHub account to mint!

@gzliudan gzliudan deleted the max branch September 5, 2022 14:41
ronhuafeng added a commit to ronhuafeng/openzeppelin-contracts that referenced this pull request Sep 9, 2022
Co-authored-by: Daniel Liu <liudaniel@qq.com>
JulissaDantes pushed a commit to JulissaDantes/openzeppelin-contracts that referenced this pull request Nov 3, 2022
Co-authored-by: Daniel Liu <liudaniel@qq.com>
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