-
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
Optimize toString #3573
Optimize toString #3573
Conversation
Thanks @CodeSandwich for this PR Note: We could use the same length-search pattern for the hex version. |
Yes, if this PR goes through, we can add these optimizations there too. One thing at a time 😃 |
contracts/utils/Strings.sol
Outdated
if (value < 10) { | ||
return string(abi.encodePacked(uint8(value + 48))); | ||
} |
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.
We should try to remove that special case
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.
We can't remove it entirely, we still need to handle the special case of 0
. A change from if(value < 10)
to if(value == 0) return "0"
decreases gas for the case of 0
from 170 to 131, but for other single-digit numbers increases from 170 to 525.
Why do you want to remove it, is it to clean up the logic or for a different reason? If it's only to clean up, IMO the gas savings justify the tiny additional complexity, especially since single-digit conversions seem to be a common use case, but that's just that: an opinion.
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.
we can, see my commit
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.
This is the commit: CodeSandwich@8597e3d
The problem is that it creates bad output, in all cases except Plus now all single-digit numbers have cost increased 3-fold to >500, not only 0
it creates a string with length 1 too big.1
to 9
. Your commit also introduces bad powers of 10. I need to look closer into this 🤔
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.
Ok, I see that it'll work just fine, the change in powers of 10 counter the initial length of 1, very clever! But the gas usage for single-digit numbers is still much higher. What are the advantages?
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.
IMO its easier to understand/review, which is always positive.
I'm love to see the gas number.
Also, I'm wondering what are the usecase for this function to be executed as part of a transaction and not in an offchain call. I think our initial approach was to optimize for deployment cost, assuming it would (almost) never be paid for anyway.
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.
Does removal of the special case reduce a lot of bytecode? Lower gas usage is always a good thing, because we don't know how this code will be used. Otherwise this entire PR is pointless, it complicates the logic in exchange for lower gas. IMO we can keep all your changes including length = 1
, but still restore the single-digit shortcut.
contracts/utils/Strings.sol
Outdated
|
||
// compute log256(value), and add it to length | ||
uint256 valueCopy = value; | ||
if (valueCopy >> 128 > 0) { |
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.
if (valueCopy >= 1 << 128) {
will be equivalent, but should be cheaper, the shifting will be evaluated at compilation time.
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'm not sure the shift is done at compile time. Can you confirm ?
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.
As of 0.8.13, yes, I've changed some constants to 1 << X
and the gas usage hasn't changed at all.
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.
but should be cheaper
Why? I'm seeing the compiler emit code sequences that cost exactly the same for the two expressions.
PUSH1 0x1 PUSH1 0x80 SHL DUP2 LT
vs
PUSH1 0x80 DUP2 SWAP1 SHR ISZERO
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'm not sure what opcodes exactly it generates, but it's marginally cheaper, 30 gas in total or 6 gas per if
, probably because it doesn't do any bit-shifting at all. I'm surprised that you got any SHL
at all, so the compiler must've not precalculated the constants, did you enable the optimizer?
The gas difference is ridiculously low anyway, I don't have a strong opinion about which version is better. The current one (valueCopy >= 1 << N
) is a little more consistent with the decimal toString
, but that's just a matter of preference. There's no "old version" to revert to, it's a new piece of code, the old implementation was completely different.
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.
did you enable the optimizer?
Yes. For constants Solidity will often prioritize code size before runtime cost.
I've pushed an optimization of the memory writing loop. I don't exactly understand why, streamlining the loop and putting the
The usage of the symbols table has zero impact on gas, but IMO it's much clearer than the addition of a magic number. |
Sorry I'm late to this PR. My thoughts are:
|
Making a PR that includes Math.log10 and Math.log2 would be great.
@CodeSandwich, would you be able to do that ? |
for (uint256 i = 2 * length + 1; i > 1; --i) { | ||
buffer[i] = _HEX_SYMBOLS[value & 0xf]; | ||
buffer[i] = _SYMBOLS[value & 0xf]; | ||
value >>= 4; | ||
} |
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.
This could be written in the same or similar way that the decimal toString
.
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.
LGTM
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com> Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com> Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
Fixes #????
This PR reduces gas usage of
Strings.toString
for decimal numbers. Here are some results:(Edit: results updated for the newest commits) Each measurement was done in a separate dapptools test to remove memory usage bias. Solc was in version 0.8.13 with optimizer set to 1 million runs.
A little more gas can be pinched, e.g. manual string allocation with Yul saves ~119 gas, but that both complicates the codebase and makes the optimizer behavior harder to predict.
Tests are cleaned up and extended.
PR Checklist