-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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
Optimize toString #3573
Changes from 8 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
e0c9042
Optimize toString
CodeSandwich 2292bf4
Set PR number
CodeSandwich 10a7a90
Express long numbers as powers
CodeSandwich 029c82e
lint
CodeSandwich 365e07b
refactor toString()
Amxx 8597e3d
Merge branch into fast_toString
Amxx 57af5c4
use the library name/selector in mocks & test
Amxx e8e61fc
improve length estimation
Amxx 7e81cc4
minimize PR change
Amxx eb7f013
minimize PR change
Amxx 2eeeb5c
gas optimization in toString() and sqrt()
Amxx 8f84142
fix > → >=
Amxx 4eace99
Optimize writing loop
CodeSandwich b9a4ae1
Less assembly
CodeSandwich 34ba82a
Merge branch 'master' into fast_toString
Amxx 8e955af
prettier
CodeSandwich d3d66ac
revert unrelated changes
frangio b026166
Merge branch 'master' into fast_toString
frangio File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,71 +1,88 @@ | ||
const { constants, expectRevert } = require('@openzeppelin/test-helpers'); | ||
const { BN, constants, expectRevert } = require('@openzeppelin/test-helpers'); | ||
|
||
const { expect } = require('chai'); | ||
|
||
const StringsMock = artifacts.require('StringsMock'); | ||
|
||
contract('Strings', function (accounts) { | ||
beforeEach(async function () { | ||
this.strings = await StringsMock.new(); | ||
}); | ||
let strings; | ||
|
||
describe('from uint256 - decimal format', function () { | ||
it('converts 0', async function () { | ||
expect(await this.strings.fromUint256(0)).to.equal('0'); | ||
}); | ||
|
||
it('converts a positive number', async function () { | ||
expect(await this.strings.fromUint256(4132)).to.equal('4132'); | ||
}); | ||
before(async function () { | ||
strings = await StringsMock.new(); | ||
}); | ||
|
||
it('converts MAX_UINT256', async function () { | ||
expect(await this.strings.fromUint256(constants.MAX_UINT256)).to.equal(constants.MAX_UINT256.toString()); | ||
}); | ||
describe('toString', function () { | ||
for (const [ key, value ] of Object.entries([ | ||
'0', | ||
'7', | ||
'10', | ||
'99', | ||
'100', | ||
'101', | ||
'123', | ||
'4132', | ||
'12345', | ||
'1234567', | ||
'1234567890', | ||
'123456789012345', | ||
'12345678901234567890', | ||
'123456789012345678901234567890', | ||
'1234567890123456789012345678901234567890', | ||
'12345678901234567890123456789012345678901234567890', | ||
'123456789012345678901234567890123456789012345678901234567890', | ||
'1234567890123456789012345678901234567890123456789012345678901234567890', | ||
].reduce((acc, value) => Object.assign(acc, { [value]: new BN(value) }), { | ||
MAX_UINT256: constants.MAX_UINT256.toString(), | ||
}))) { | ||
it(`converts ${key}`, async function () { | ||
expect(await strings.methods['toString(uint256)'](value)).to.equal(value.toString(10)); | ||
}); | ||
} | ||
}); | ||
|
||
describe('from uint256 - hex format', function () { | ||
describe('toHexString', function () { | ||
it('converts 0', async function () { | ||
expect(await this.strings.fromUint256Hex(0)).to.equal('0x00'); | ||
expect(await strings.methods['toHexString(uint256)'](0)).to.equal('0x00'); | ||
}); | ||
|
||
it('converts a positive number', async function () { | ||
expect(await this.strings.fromUint256Hex(0x4132)).to.equal('0x4132'); | ||
expect(await strings.methods['toHexString(uint256)'](0x4132)).to.equal('0x4132'); | ||
}); | ||
|
||
it('converts MAX_UINT256', async function () { | ||
expect(await this.strings.fromUint256Hex(constants.MAX_UINT256)) | ||
expect(await strings.methods['toHexString(uint256)'](constants.MAX_UINT256)) | ||
.to.equal(web3.utils.toHex(constants.MAX_UINT256)); | ||
}); | ||
}); | ||
|
||
describe('from uint256 - fixed hex format', function () { | ||
describe('toHexString fixed', function () { | ||
it('converts a positive number (long)', async function () { | ||
expect(await this.strings.fromUint256HexFixed(0x4132, 32)) | ||
expect(await strings.methods['toHexString(uint256,uint256)'](0x4132, 32)) | ||
.to.equal('0x0000000000000000000000000000000000000000000000000000000000004132'); | ||
}); | ||
|
||
it('converts a positive number (short)', async function () { | ||
await expectRevert( | ||
this.strings.fromUint256HexFixed(0x4132, 1), | ||
strings.methods['toHexString(uint256,uint256)'](0x4132, 1), | ||
'Strings: hex length insufficient', | ||
); | ||
}); | ||
|
||
it('converts MAX_UINT256', async function () { | ||
expect(await this.strings.fromUint256HexFixed(constants.MAX_UINT256, 32)) | ||
expect(await strings.methods['toHexString(uint256,uint256)'](constants.MAX_UINT256, 32)) | ||
.to.equal(web3.utils.toHex(constants.MAX_UINT256)); | ||
}); | ||
}); | ||
|
||
describe('from address - fixed hex format', function () { | ||
describe('toHexString address', function () { | ||
it('converts a random address', async function () { | ||
const addr = '0xa9036907dccae6a1e0033479b12e837e5cf5a02f'; | ||
expect(await this.strings.fromAddressHexFixed(addr)).to.equal(addr); | ||
expect(await strings.methods['toHexString(address)'](addr)).to.equal(addr); | ||
}); | ||
|
||
it('converts an address with leading zeros', async function () { | ||
const addr = '0x0000e0ca771e21bd00057f54a68c30d400000000'; | ||
expect(await this.strings.fromAddressHexFixed(addr)).to.equal(addr); | ||
expect(await strings.methods['toHexString(address)'](addr)).to.equal(addr); | ||
}); | ||
}); | ||
}); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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 anySHL
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 decimaltoString
, 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.
Yes. For constants Solidity will often prioritize code size before runtime cost.