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.

Sign up for GitHub

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

Jump to bottom

Optimize toString #3573

Merged
merged 18 commits into from
Aug 31, 2022
Merged

Optimize toString #3573

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
e0c9042
Optimize toString
CodeSandwich Jul 23, 2022
2292bf4
Set PR number
CodeSandwich Jul 23, 2022
10a7a90
Express long numbers as powers
CodeSandwich Jul 25, 2022
029c82e
lint
CodeSandwich Jul 25, 2022
365e07b
refactor toString()
Amxx Jul 25, 2022
8597e3d
Merge branch into fast_toString
Amxx Jul 25, 2022
57af5c4
use the library name/selector in mocks & test
Amxx Jul 25, 2022
e8e61fc
improve length estimation
Amxx Jul 25, 2022
7e81cc4
minimize PR change
Amxx Jul 25, 2022
eb7f013
minimize PR change
Amxx Jul 25, 2022
2eeeb5c
gas optimization in toString() and sqrt()
Amxx Jul 25, 2022
8f84142
fix > → >=
Amxx Jul 25, 2022
4eace99
Optimize writing loop
CodeSandwich Jul 26, 2022
b9a4ae1
Less assembly
CodeSandwich Aug 25, 2022
34ba82a
Merge branch 'master' into fast_toString
Amxx Aug 25, 2022
8e955af
prettier
CodeSandwich Aug 26, 2022
d3d66ac
revert unrelated changes
frangio Aug 31, 2022
b026166
Merge branch 'master' into fast_toString
frangio Aug 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@
* `ReentrancyGuard`: Reduce code size impact of the modifier by using internal functions. ([#3515](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3515))
* `SafeCast`: optimize downcasting of signed integers. ([#3565](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3565))
* `ERC20FlashMint`: add an internal `_flashFee` function for overriding. ([#3551](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3551))
* `Strings`: optimize `toString`. ([#3573](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3573))

### Compatibility Note

Expand Down
8 changes: 4 additions & 4 deletions contracts/mocks/StringsMock.sol
View file
Open in desktop
Expand Up @@ -5,19 +5,19 @@ pragma solidity ^0.8.0;
import "../utils/Strings.sol";

contract StringsMock {
function fromUint256(uint256 value) public pure returns (string memory) {
function toStringDecimal(uint256 value) public pure returns (string memory) {
Amxx marked this conversation as resolved.
Show resolved Hide resolved
return Strings.toString(value);
}

function fromUint256Hex(uint256 value) public pure returns (string memory) {
function toHexString(uint256 value) public pure returns (string memory) {
return Strings.toHexString(value);
}

function fromUint256HexFixed(uint256 value, uint256 length) public pure returns (string memory) {
function toHexString(uint256 value, uint256 length) public pure returns (string memory) {
return Strings.toHexString(value, length);
}

function fromAddressHexFixed(address addr) public pure returns (string memory) {
function toHexStringAddress(address addr) public pure returns (string memory) {
return Strings.toHexString(addr);
}
}
65 changes: 47 additions & 18 deletions contracts/utils/Strings.sol
View file
Open in desktop
Expand Up @@ -13,26 +13,55 @@ library Strings {
/**
* @dev Converts a `uint256` to its ASCII `string` decimal representation.
*/
function toString(uint256 value) internal pure returns (string memory) {
// Inspired by OraclizeAPI's implementation - MIT licence
// https://github.com/oraclize/ethereum-api/blob/b42146b063c7d6ee1358846c198246239e9360e8/oraclizeAPI_0.4.25.sol
function toString(uint256 value) internal pure returns (string memory result) {
unchecked {
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

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

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.

uint256 length = 0;
uint256 valueCopy = value;
if (valueCopy >= 1000000000000000000000000000000000000000000000000000000000000000) {
valueCopy /= 10000000000000000000000000000000000000000000000000000000000000000;
length += 64;
}
if (valueCopy >= 10000000000000000000000000000000) {
valueCopy /= 100000000000000000000000000000000;
length += 32;
}
if (valueCopy >= 1000000000000000) {
valueCopy /= 10000000000000000;
length += 16;
}
if (valueCopy >= 10000000) {
valueCopy /= 100000000;
length += 8;
}
if (valueCopy >= 1000) {
valueCopy /= 10000;
length += 4;
}
if (valueCopy >= 10) {
valueCopy /= 100;
length += 2;
}
if (valueCopy >= 1) {
length += 1;
}
CodeSandwich marked this conversation as resolved.
Show resolved Hide resolved
result = new string(length);
/// @solidity memory-safe-assembly
assembly {
let ptr := add(result, add(length, 32))
for {

if (value == 0) {
return "0";
}
uint256 temp = value;
uint256 digits;
while (temp != 0) {
digits++;
temp /= 10;
}
bytes memory buffer = new bytes(digits);
while (value != 0) {
digits -= 1;
buffer[digits] = bytes1(uint8(48 + uint256(value % 10)));
value /= 10;
} gt(value, 0) {

} {
ptr := sub(ptr, 1)
mstore8(ptr, add(48, mod(value, 10)))
value := div(value, 10)
}
}
}
return string(buffer);
}

/**
Expand Down
94 changes: 76 additions & 18 deletions test/utils/Strings.test.js
View file
Open in desktop
Expand Up @@ -5,67 +5,125 @@ const { expect } = require('chai');
const StringsMock = artifacts.require('StringsMock');

contract('Strings', function (accounts) {
beforeEach(async function () {
this.strings = await StringsMock.new();
let strings;

before(async function () {
strings = await StringsMock.new();
});

describe('from uint256 - decimal format', function () {
describe('toString', function () {
async function testToString (input) {
expect(await strings.toStringDecimal(input)).to.equal(input);
}

it('converts 0', async function () {
expect(await this.strings.fromUint256(0)).to.equal('0');
await testToString('0');
});

it('converts a positive number', async function () {
expect(await this.strings.fromUint256(4132)).to.equal('4132');
it('converts 7', async function () {
await testToString('7');
});

it('converts 42', async function () {
await testToString('42');
});

it('converts 123', async function () {
await testToString('123');
});

it('converts 4132', async function () {
await testToString('4132');
});

it('converts 12345', async function () {
await testToString('12345');
});

it('converts 1234567', async function () {
await testToString('1234567');
});

it('converts 1234567890', async function () {
await testToString('1234567890');
});

it('converts 123456789012345', async function () {
await testToString('123456789012345');
});

it('converts 12345678901234567890', async function () {
await testToString('12345678901234567890');
});

it('converts 123456789012345678901234567890', async function () {
await testToString('123456789012345678901234567890');
});

it('converts 1234567890123456789012345678901234567890', async function () {
await testToString('1234567890123456789012345678901234567890');
});

it('converts 12345678901234567890123456789012345678901234567890', async function () {
await testToString('12345678901234567890123456789012345678901234567890');
});

it('converts 123456789012345678901234567890123456789012345678901234567890', async function () {
await testToString('123456789012345678901234567890123456789012345678901234567890');
});

it('converts 1234567890123456789012345678901234567890123456789012345678901234567890', async function () {
await testToString('1234567890123456789012345678901234567890123456789012345678901234567890');
});

it('converts MAX_UINT256', async function () {
expect(await this.strings.fromUint256(constants.MAX_UINT256)).to.equal(constants.MAX_UINT256.toString());
await testToString(constants.MAX_UINT256.toString());
});
});

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.toHexString(0)).to.equal('0x00');
});

it('converts a positive number', async function () {
expect(await this.strings.fromUint256Hex(0x4132)).to.equal('0x4132');
expect(await strings.toHexString(0x4132)).to.equal('0x4132');
});

it('converts MAX_UINT256', async function () {
expect(await this.strings.fromUint256Hex(constants.MAX_UINT256))
expect(await strings.toHexString(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.toHexString(0x4132, 32))
.to.equal('0x0000000000000000000000000000000000000000000000000000000000004132');
});

it('converts a positive number (short)', async function () {
await expectRevert(
this.strings.fromUint256HexFixed(0x4132, 1),
strings.toHexString(0x4132, 1),
'Strings: hex length insufficient',
);
});

it('converts MAX_UINT256', async function () {
expect(await this.strings.fromUint256HexFixed(constants.MAX_UINT256, 32))
expect(await strings.toHexString(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.toHexStringAddress(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.toHexStringAddress(addr)).to.equal(addr);
});
});
});