Optimize Math operations using branchless bool to uint translation. #4878
Conversation
🦋 Changeset detectedLatest commit: b0ad358 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 |
CodeSandwich
force-pushed
the
optimize_log2
branch
from
40c3a38
to
7f5b1b3
Compare
|
Hello @CodeSandwich for this proposal. @ernestognw & @frangio, how do you feel about this kind of assembly. We used to go for readability first, but it feels like we are doing assembly more and more (for gas costs). I have mixed feelings. On the one hand, I can see how effective it can be in math libraries. On the other, I don't want us to start doing assembly everywhere. |
|
Note: if we decide to do this change, then I think we should also do it for log10 and log256. |
|
This is interesting, I don't really see this PR as significantly increasing the use of assembly. The only reason that assembly is used here is for an integer-valued "greater than" operation returning 0/1. Solidity has no branchless way of casting a boolean to an integer in this way. This operation could be written as a separate Solidity function to keep the function body free of assembly. The question is if this PR increases the complexity of the function significantly, and I don't think it does. The gas improvement is not negligible either. |
|
The amount of assembly seems reasonable. Also, assembly {
isGt := gt(value, x)
}Alternatively, the function _gt(uint256 a, uint256 b) private pure returns(uint256 isGt) {
assembly {
isGt := gt(a, b)
}
}
function log2(uint256 value) internal pure returns (uint256 result) {
unchecked {
uint256 isGt = _gt(value,0xffffffffffffffffffffffffffffffff);
value >>= isGt * 128;
result += isGt * 128;
isGt = _gt(value, 0xffffffffffffffff);
value >>= isGt * 64;
result += isGt * 64;
isGt = _gt(value, 0xffffffff);
value >>= isGt * 32;
result += isGt * 32;
...
} |
|
What about a function _boolToUint(bool b) private pure returns (uint256 u) {
assembly { u := b }
}
function log2(uint256 value) internal pure returns (uint256 result) {
unchecked {
uint256 isGt = _boolToUint(value > 0xffffffffffffffffffffffffffffffff);
value >>= isGt * 128;
result += isGt * 128;
isGt = _boolToUint(value > 0xffffffffffffffff);
value >>= isGt * 64;
result += isGt * 64;
isGt = _boolToUint(value > 0xffffffff);
value >>= isGt * 32;
result += isGt * 32;
...
}? Note: maybe the return type should be |
That also seems fine. I still see no issue with the assembly version, so I'd reconsider using it if there's a significant difference in gas (shouldn't be though). |
|
I'm trying to run a snapshot myself with the same parameters as @CodeSandwich: But I'm getting a nasty error: We should benchmark both the |
This is a known, common compiler error when using Solidity with via-ir on larger projects. I haven't been compiling the entire OZ to do the benchmarks, I created a separate contract with a copy of the original log2 and the modified version, and only run regular Here's the code I used for benchmarking:// SPDX-License-Identifier: GPL-3.0-only
pragma solidity ^0.8.23;
import {Test, console} from "forge-std/Test.sol";
contract Log2Test is Test {
function log2Old(uint256 value) internal pure returns (uint256) {
uint256 result = 0;
unchecked {
if (value >> 128 > 0) {
value >>= 128;
result += 128;
}
if (value >> 64 > 0) {
value >>= 64;
result += 64;
}
if (value >> 32 > 0) {
value >>= 32;
result += 32;
}
if (value >> 16 > 0) {
value >>= 16;
result += 16;
}
if (value >> 8 > 0) {
value >>= 8;
result += 8;
}
if (value >> 4 > 0) {
value >>= 4;
result += 4;
}
if (value >> 2 > 0) {
value >>= 2;
result += 2;
}
if (value >> 1 > 0) {
result += 1;
}
}
return result;
}
function log2(uint256 value) internal pure returns (uint256 result) {
unchecked {
uint256 isGt;
assembly {
isGt := gt(value, 0xffffffffffffffffffffffffffffffff)
}
value >>= isGt * 128;
result += isGt * 128;
assembly {
isGt := gt(value, 0xffffffffffffffff)
}
value >>= isGt * 64;
result += isGt * 64;
assembly {
isGt := gt(value, 0xffffffff)
}
value >>= isGt * 32;
result += isGt * 32;
assembly {
isGt := gt(value, 0xffff)
}
value >>= isGt * 16;
result += isGt * 16;
assembly {
isGt := gt(value, 0xff)
}
value >>= isGt * 8;
result += isGt * 8;
assembly {
isGt := gt(value, 0xf)
}
value >>= isGt * 4;
result += isGt * 4;
assembly {
isGt := gt(value, 0x3)
}
value >>= isGt * 2;
result += isGt * 2;
assembly {
isGt := gt(value, 0x1)
}
result += isGt;
}
}
uint256 zero = 0;
function bench(uint256 x) public {
unchecked {
x += zero;
}
uint256 gasOld = gasleft();
uint256 oldY = log2Old(x);
gasOld -= gasleft();
uint256 gasNew = gasleft();
uint256 newY = log2(x);
gasNew -= gasleft();
console.log("Gas old : gas new : input", gasOld, gasNew, x);
if (zero > 0) console.log(oldY, newY);
assertEq(oldY, newY, "Invalid result");
}
function testBench() public {
for (uint256 i = 0; i < 33; i++) {
bench(i);
}
for (uint256 i = 1; i < 39; i++) {
bench(100 ** i);
bench(100 ** i * 2);
bench(100 ** i * 5);
}
bench(10 ** 77);
bench(type(uint256).max);
}
}The bool-to-uint helper is a brilliant idea, it produced 0 overhead and is very elegant, I'll push a commit using it: function boolToUint(bool b) internal pure returns(uint256 u) {
assembly {
u := b
}
}It's important to return |
|
I've added |
|
I see that you added the changes yourself. I did that too, with some clean-ups for readability: I think that there's a very important caveat I highlighted in the docs for There could be 2 versions, a private (the OZ API seems pitfall-averse) Actually the safe function boolToUint(bool b) internal pure returns (uint256 u) {
assembly {
u := iszero(iszero(b))
}
} |
CodeSandwich
force-pushed
the
optimize_log2
branch
from
a1ae543
to
32b3d97
Compare
Amxx
changed the title
There was a problem hiding this comment.
This all looks pretty good to me.
Need a second review from @ernestognw
contracts/utils/math/Math.sol
Outdated
| function boolToUint(bool b) internal pure returns (uint256 u) { | ||
| /// @solidity memory-safe-assembly | ||
| assembly { | ||
| u := b | ||
| } | ||
| } |
There was a problem hiding this comment.
This looks to me more of a fit into the SafeCast library with the function toUint(bool b) signature as it already requires a boolean as an argument.
There was a problem hiding this comment.
I'm totally fine with this, but does it fit the other functions from SafeCast? IIUC the main goal of this library is to make dangerous operations and throw in case of an overflow. In toUint256(bool) there's no overflow possible, there's nothing "safe" about this SafeCast function.
There was a problem hiding this comment.
Yeah that's right, I don't have the last word but I feel confident that SafeCast is a better fit than Math.
We might consider having these unconventional casts in a different library but that's a broader discussion.
|
I pushed a commit moving the I still feel it's a better fit in If this takes too long. I'm down for keeping the function private for now. The intention of this PR was to Optimize |
This PR makes the
Math.log2function branchless. I benchmarked it using Solidity 0.8.24, via-ir and 1B optimizer runs.Here are the results.
PR Checklist
npx changeset add)