Repository navigation
Gas: format and parse without the per-digit loops - #337
Merged
Merged
Conversation
Format writes digits directly in assembly, dropping the OpenZeppelin Strings import; parse's trailing-zero scan is an assembly loop. Verbatim copies of both libraries at ce55522 back an equivalence fuzz and a gas bench asserting identical bytes and lower-or-equal gas. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 26 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Parse keeps main's int256 exponent-wrap guard and past-int256 integer path beside the assembly fraction scan; CMASK_ZERO stays imported for main's parseIntegerPastInt256. The parse equivalence copy is refreshed to main's parse at ff6d70c (the format copy already equals main's), and the equivalence suite gains the int256 exponent-bound literals. The scan log is the jq union of both sides. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thedavidmeister
changed the base branch from
2026-10-06-issue-310-core-gas
to
main
October 10, 2026 12:34
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…idity Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The last items of #310: #11 (format and parse) is kept, #6 (add/sub maximizing one operand) is rejected.
Kept: #11, format and parse
toDecimalStringwrites its digits straight into one buffer in assembly. There are no morestring.concatloops or OpenZeppelinStringscalls, and the import is gone. Trailing decimal zeros come out with a 64/32/16/8/4/2/1 divide cascade, not one digit at a time.parseDecimalFloat's trailing-zero scan over the fraction is an assembly loop. It stops at the decimal point at the latest, so it needs no bounds check.test/lib/LibFormatDecimalFloatPr321.solandtest/lib/LibParseDecimalFloatPr321.solare verbatim copies of both libraries on main: format as of ce55522 (#321's head), whose code main has not changed since, and parse at ff6d70c, main's parse with the #355/#356 exponent-wrap guard and the past-int256 integer path. The equivalence fuzz checks the new code against them byte for byte: output strings, parse error selectors and packed floats. The gas bench measures only the library call between twogasleft()reads. Every row asserts identical bytes and no more gas than the copy (forge test --mc LibDecimalFloatFormatParseGasTest -vv). Re-measured at 3e3e59a (after merging main 1047972, which includes #398; main changed only a format doc comment, which this branch carries):Format costs 1.2k to 198k less per call and parse 0 to 1.55k less, with identical output on every row.
Assembly against the same algorithm in plain Solidity
test/lib/LibFormatDecimalFloatSolidity.solis the same digit-buffer algorithm with every assembly block written as plain Solidity: digits backwards into a buffer, copied into an output of exact length, and the same 64/32/16/8/4/2/1 trailing-zero cascade.testFormatSolidityCopyEquivalencefuzzes it against the library for identical bytes and identical revert data. TheformatSolVsAsm/formatSciSolVsAsmrows of the same bench assert identical bytes and that the assembly costs no more:The assembly is cheaper on every row, by 477 to 139405, so
srcstays assembly. Plain Solidity pays a bounds check and a masked single-byte write per digit where the assembly copies 32 bytes per word. The parse scan has no plain-Solidity counterpart:parseDecimalFloatInlineworks on memory cursors, which Solidity cannot read without assembly, so its rows above compare against theisMaskloop only.The main suites,
forge test --mc 'LibDecimalFloat(Ops)?GasTest|LibParseDecimalFloatGasTest' -vv, run on main 51a3451 and on 68cb426 (format and parse src unchanged since): every op row is identical except the parse rows, which only drop.LibParseDecimalFloatGasTest01100-421.5-123.4560.0000011e181.5e-71000000000000000000123456789012345678901234567890.123456789LibDecimalFloatGasTest.testGasParse11.51.234567890123456789-1.5e-101e18Rejected: #6, add/sub maximizing one operand
Every variant costs more on the packed path wherever B's order of magnitude wins (B is swapped in), and that path is the common one. Measured against #321's
addwith a bench whose rows all assert identical results. Gas is theaddcall alone:boolswapv2 and v3 save about 300 to 600 when A wins, but add 119 to 237 whenever B wins. v4 adds about 50 on almost every row. None of them is free on the packed path, so
addis unchanged.QA
Discriminating tests:
LibFormatDecimalFloatMemoryTest(testFormatMemoryScientific,testFormatMemoryNonScientificFraction,testFormatMemoryNonScientificInteger) is new. It checks that format leaves the allocation before it untouched, does not let a second call write over the first string, and leaves the string's padding zero. Each test fails under the mutants below that it kills, and passes on the code. Byte identity with Gas: maximize, mul and packLossy without touching the packed path #321 is checked byLibDecimalFloatFormatParsePr321EquivalenceTestand by every row ofLibDecimalFloatFormatParseGasTest.Mutations applied: 89 mutants, run with
mutation-probeover every forge contract that reaches format or parse. 82 were killed by the suite at 1c2258b. Examples: the scientific 76/75 digit threshold, the exponent sign and digits, eachstripTrailingZerosstep, each copy loop's bound and cursor advance, theZEROS/ZERO_POINTconstants, and all five parse-scan mutants (byte, offset, step, char, disabled). They were killed bytestFormatPr321EquivalenceDigitCounts,testFormatDecimalExamples,testFormatGas,testParseLiteralDecimalFloatDecimalsand others. Four more are killed only by the new memory test:testFormatMemoryScientific.testFormatMemoryNonScientific*.Three are equivalent. N02 and N03 move the positive exponent cap 68 → 69 or 79. int224.max / 10^e is 0 for every e in [68, 78], including the wrapped 10^78, so the call still reverts with the same error. N34 rounds the free pointer with 0x1f in place of 0x3f, which still covers the padding. The record is in
audit/mutation-test-scans.json.Oracle: byte identity with verbatim copies of main's format (unchanged since ce55522) and main's parse at ff6d70c, as Gas: core arithmetic, packing and comparison hot spots #310 requires, plus the existing hand-written expected strings in the format and parse tests. The memory test's expected strings are written out by hand: -1/3 as
-3.33…e-1and-0.33…, and int224.min.testParsePr321EquivalenceExponentBoundsadds literals at the int256 exponent bounds (from a fraction, from trailing zeros past int256, and exponent digits past int256), so the wrap guard'sParseDecimalPrecisionLoss/ExponentOverflowsplit is checked byte for byte through the new scan.Category check: Gas: maximize, mul and packLossy without touching the packed path #321 says it leaves only test add #6 and 2024 07 10 mul #11 of Gas: core arithmetic, packing and comparison hot spots #310 open ("items test add #6 and 2024 07 10 mul #11 stay open there"). This PR does 2024 07 10 mul #11 (format/parse) and rejects test add #6 with the numbers above, so it closes Gas: core arithmetic, packing and comparison hot spots #310.
Runs at 3e3e59a:
forge build --deny warnings --forceandforge lintclean,forge fmt --checkclean,forge test: 867 passed, 0 failed.cargo test --releaseincrates/tests: 159 passed, 0 failed.Merge with main: this PR's src lines are unchanged by it. The only src edit is keeping
CMASK_ZEROimported for main'sparseIntegerPastInt256, so the mutation record above still covers them.Merge of main 1047972 (e1d4a3e): one conflict, the
_toScientificNatSpec, resolved by keeping this branch's function and adding main'se0omission sentence (this formatter already omitse0; main's test: check formatting against an independent rendering #398 oracle tests pass unchanged).audit/mutation-test-scans.jsonmerged as the union of both sides (40 entries).Closes #310
🤖 Generated with Claude Code