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 toString #3573

Merged
merged 18 commits into from
Aug 31, 2022
Merged

Optimize toString #3573

merged 18 commits into from
Aug 31, 2022

Conversation

CodeSandwich
Copy link
Contributor

@CodeSandwich CodeSandwich commented Jul 23, 2022

Fixes #????

This PR reduces gas usage of Strings.toString for decimal numbers. Here are some results:

Input                                                                          | Current | New  | Gas saved
-------------------------------------------------------------------------------|---------|------|-----------------
0                                                                              | 131     | 458  | -327  (-249,62%)
1                                                                              | 833     | 458  | 375   (45,02%)
12                                                                             | 1383    | 534  | 849   (61,39%)
123                                                                            | 1933    | 611  | 1322  (68,39%)
1234                                                                           | 2483    | 687  | 1796  (72,33%)
12345                                                                          | 3033    | 735  | 2298  (75,77%)
1234567                                                                        | 4133    | 888  | 3245  (78,51%)
1234567890                                                                     | 5783    | 1059 | 4724  (81,69%)
123456789012345                                                                | 8533    | 1413 | 7120  (83,44%)
12345678901234567890                                                           | 11283   | 1708 | 9575  (84,86%)
123456789012345678901234567890                                                 | 16783   | 2357 | 14426 (85,96%)
1234567890123456789012345678901234567890                                       | 22289   | 2983 | 19306 (86,62%)
12345678901234567890123456789012345678901234567890                             | 27789   | 3574 | 24215 (87,14%)
123456789012345678901234567890123456789012345678901234567890                   | 33289   | 4252 | 29037 (87,23%)
1234567890123456789012345678901234567890123456789012345678901234567890         | 38795   | 4820 | 33975 (87,58%)
115792089237316195423570985008687907853269984665640564039457584007913129639935 | 43195   | 5345 | 37850 (87,63%)

(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

  • Tests
  • Documentation
  • Changelog entry

@Amxx
Copy link
Collaborator

Amxx commented Jul 25, 2022

Thanks @CodeSandwich for this PR

Note: We could use the same length-search pattern for the hex version.

@CodeSandwich
Copy link
Contributor Author

Yes, if this PR goes through, we can add these optimizations there too. One thing at a time 😃

Comment on lines 18 to 20
if (value < 10) {
return string(abi.encodePacked(uint8(value + 48)));
}
Copy link
Collaborator

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

Copy link
Contributor Author

@CodeSandwich CodeSandwich Jul 25, 2022

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.

Copy link
Collaborator

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

Copy link
Contributor Author

@CodeSandwich CodeSandwich Jul 25, 2022

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 0 it creates a string with length 1 too big. Plus now all single-digit numbers have cost increased 3-fold to >500, not only 1 to 9. Your commit also introduces bad powers of 10. I need to look closer into this 🤔

Copy link
Contributor Author

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?

Copy link
Collaborator

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.

Copy link
Contributor Author

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 Show resolved Hide resolved
@Amxx Amxx modified the milestones: 5.0, 4.8 Jul 25, 2022

// compute log256(value), and add it to length
uint256 valueCopy = value;
if (valueCopy >> 128 > 0) {
Copy link
Contributor Author

@CodeSandwich CodeSandwich Jul 25, 2022

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.

Copy link
Collaborator

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 ?

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

@CodeSandwich CodeSandwich Aug 25, 2022

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.

Copy link
Contributor

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.

@CodeSandwich
Copy link
Contributor Author

I've pushed an optimization of the memory writing loop. I don't exactly understand why, streamlining the loop and putting the iszero command as the last thing of the loop just clicks with the optimizer. The new gas usage results:

0: 458
1: 458
12: 534
123: 611
1234: 687
12345: 735
1234567: 888
1234567890: 1059
123456789012345: 1413
12345678901234567890: 1708
123456789012345678901234567890: 2357
1234567890123456789012345678901234567890: 2983
12345678901234567890123456789012345678901234567890: 3574
123456789012345678901234567890123456789012345678901234567890: 4252
1234567890123456789012345678901234567890123456789012345678901234567890: 4820
115792089237316195423570985008687907853269984665640564039457584007913129639935: 5345 (8 fold decrease from master 😆)

The usage of the symbols table has zero impact on gas, but IMO it's much clearer than the addition of a magic number.

contracts/utils/Strings.sol Outdated Show resolved Hide resolved
@frangio
Copy link
Contributor

frangio commented Aug 23, 2022

Sorry I'm late to this PR.

My thoughts are:

  • For loops in Yul have very unfamiliar syntax, at the moment we don't have any in the entire codebase, and I would prefer if that remained the case. The main gains in this PR are due to an optimized log10 implementation, and probably some more from removing bounds checks on array accesses. Some assembly is necessary for the second part but I would strongly prefer if the for loop was outside of that.
  • I would really appreciate if the log10 implementation was added as Math.log10 so it could be used on its own. That also implies some more testing though, so I would be ok with doing that in a later PR.
  • As I commented above, >= 1 << 128 vs >> 127 > 0 seems to not make a difference at all so I'd like us to revert the unrelated changes in Math.sol.

@Amxx
Copy link
Collaborator

Amxx commented Aug 23, 2022

I would really appreciate if the log10 implementation was added as Math.log10 so it could be used on its own. That also implies some more testing though, so I would be ok with doing that in a later PR.

Making a PR that includes Math.log10 and Math.log2 would be great.

  • Math.log2 could be used in Math.sqrt(uint256) and Strings.toHexString(uint256)
  • Math.log10 would be used in Strings.toString(uint256)

@CodeSandwich, would you be able to do that ?

Comment on lines 114 to 117
for (uint256 i = 2 * length + 1; i > 1; --i) {
buffer[i] = _HEX_SYMBOLS[value & 0xf];
buffer[i] = _SYMBOLS[value & 0xf];
value >>= 4;
}
Copy link
Contributor

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.

frangio
frangio previously approved these changes Aug 31, 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.

LGTM

@frangio frangio enabled auto-merge (squash) August 31, 2022 22:59
@frangio frangio merged commit 160bf1a into OpenZeppelin:master Aug 31, 2022
@Amxx Amxx mentioned this pull request Sep 1, 2022
3 tasks
ronhuafeng added a commit to ronhuafeng/openzeppelin-contracts that referenced this pull request Sep 9, 2022
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
JulissaDantes pushed a commit to JulissaDantes/openzeppelin-contracts that referenced this pull request Nov 3, 2022
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: Francisco Giordano <frangio.1@gmail.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