-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
Branchless ternary, min and max methods #4976
Branchless ternary, min and max methods #4976
Conversation
🦋 Changeset detectedLatest commit: dc6abbc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @Lohann and thank you for this PR.
The min/max option is interresting, because branches are indeed notoriously bad. Code readability is not that great though. If we don't have them yet, this would definitelly need to be fuzzed.
I'm not that fan of the rest. The objective of this library has always been security, readability, and auditability. Regardless of what some other project may think, we strongly believe in readability (and documentation through comments) of the code.
Therefore, I'm not leaning toward a full inline assembly version, nor am I willing to remove the documentation comments (that we took so long to make sure are clear)
@Amxx thanks for the feedback, will remove some assembly at the cost of a consuming a bit more gas, but essentially what the sqrt does is: function sqrt(uint256 a) public pure returns (uint256) {
unchecked {
// Approximate to the closest power of 2 of the square root. This is equivalent to set the MSB of
// the square root, this reduces Netwon's Method iterations required to get the final result.
uint256 xn = 2 ** (Math.log2(a) / 2);
// 7 iterations of Netwon's method
xn = (xn + a / xn) >> 1;
xn = (xn + a / xn) >> 1;
xn = (xn + a / xn) >> 1;
xn = (xn + a / xn) >> 1;
xn = (xn + a / xn) >> 1;
xn = (xn + a / xn) >> 1;
xn = (xn + a / xn) >> 1;
// Round down
// Obs: Using `Math.max` is equivalent to round to nearest, not round up.
xn = Math.min(xn, a / xn);
return xn;
}
} Except that:
|
This is almost exactly what we used to have It got changed in a long and painfull (to review) PR. This included documenting a lot of math to prove (and not just feel) that the new version is correct (in addition to being cheaper). If we are to go back on sqrt, I'll do it in a separate issue/PR. I'd rather split the review effort and the frustration. I really see a point to merging min/max decently fast ... and I fear sqrt will take much more discussion. |
@Amxx makes sense, I removed the sqrt changes, I also removed all assembly code maintaining the same gas efficiency to I made it a bit more generic by introducing the |
Anythink that is not a security fix goes into a minor anyway.
We'll discuss that. Thank you for your effort ... though please don't be frustrated if this takes time to merge. Our processes are ususally considered slow by external contributors, but we fell that is how we achieve maximum quality/security. |
I can make the |
I don't think it should be private. We want to be able to use it in different places/libraries without re-declaring it. I think it comes down to documentation that it is not always cheaper than a ternary, and that both branches are always evaluate. |
Catching up with this. I'd like to highlight that @Lohann started this looking for constant cost in math operations. Although it's not the kind of priorities OpenZeppelin Contracts has, it's an honest requirement. I also agree with @Amxx with that we prefer more readable code. I would consider creating an issue for branchless math if this requirement of constant costs ever comes back. I'll be leaving comments while I read the conversation. |
Is this true? I see there's an open issue about it: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also have mixed feelings regarding compiler branchless optimization, but I think it's not implemented and, in general, Solidity developers complain about obvious optimizations not performed, so I see value in giving some control to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit with the changeset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried looking for ternary branchless optimization in Solidity without luck, so I'd expect such optimization to not exist. Regardless, we can't ensure it'll be more efficient in the future, so I focused the changeset on the new ternary
function and its "constant gas" property.
For the controversial part of Math.sol
, the discussion will continue back and forth without strong evidence of the distribution in applications out there, so I wouldn't focus on it if the result is more-less 20 gas units.
LGTM, but let's wait on @Amxx since the name change to ternary
wasn't agreed with him.
Congrats, your important contribution to this open-source project has earned you a GitPOAP! GitPOAP: 2024 OpenZeppelin Contracts Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
Description
Follow a Branchless Min and Max functions, all of them consumes less gas than the current implementation, and have a constant gas cost independent of the input value.
Motivation
I'm working in the Analog Timechain, which is a cross-chain protocol that allow interoperability between different blockchains.
At some point our protocol needs for a given input calculate the exact execution gas cost, but this was hard due a lot of branching, branching that should not even exist, for example the
Math.min
function use more or less gas whena < b
ora >= b
, either compiling in debug and optimized mode:a < b
consumes306 gas
a >= b
consumes316 gas
This was annoying because for every change in the contract I had to dive into the EVM assembly to understand how solidity generate different branches to understand how it affect the gas used. So I decided to take a different approach and make everything constant gas cost, as result I implemented the BranchlessMath.sol library, which have a constant gas cost for any input, and surprisingly I also noticed significant gas savings using those methods compared to the OpenZeppeling's Math library, so I decided to make it available to everyone by contributing here.
In comparison the branchless version reduces the final bytecode size, and also consumes less gas:
Always consumes
291 gas
, independently ifa < b
ora >= b
.PR Checklist
npx changeset add
)