perf: Speed up addition time for drastically different exponents - #7825
Open
ximinez wants to merge 14 commits into
Open
perf: Speed up addition time for drastically different exponents#7825ximinez wants to merge 14 commits into
ximinez wants to merge 14 commits into
Conversation
- When dropping digits, short circuit the loop if the smaller value loses all significant digits in both the mantissa, and the Guard. - Adds unit tests demonstrating some extreme examples.
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a performance corner case in xrpl::Number addition when operands have exponents at opposite extremes of the valid range, avoiding long digit-dropping loops when the smaller operand has no remaining significant/recoverable digits. It also adds tests to demonstrate both the performance-sensitive addition scenario and expected overflow behavior when parsing extremely large exponents from JSON.
Changes:
- Add a
Number::Guardfast-path (doDropDigitWithTarget) to jump the exponent to the target once the mantissa is zero and guard digits are no longer recoverable. - Add unit tests covering addition with
kMaxExponentvskMinExponentacross rounding modes. - Add unit tests asserting JSON parsing/normalization overflows for very large exponent strings (e.g.
1e2000000000).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libxrpl/basics/Number.cpp | Adds a guard-based short-circuit during exponent alignment in addition to avoid pathological loop counts. |
| src/tests/libxrpl/basics/Number.cpp | Adds regression tests for extreme-exponent addition behavior (including rounding-mode-specific expectations) and a normalization overflow case. |
| src/test/protocol/STNumber_test.cpp | Refactors repeated exception assertions into a helper and adds overflow tests for huge exponent strings passed via JSON. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
High Level Overview of Change
Context of Change
We received a report that a Json string with an out of range exponent (e.g.
1e2000000000) would successfully bypassNumberinvariants, and that adding anything to that value would tie up the node for 1-2 seconds as the exponents were brought into sync. The report was completely wrong about the Json parsing. (I added tests toSTNumber_test.cppdemonstrating such.) Those values overflow as designed and expected.However, when I checked two numbers with exponents at the valid opposite extremes (e.g. approximately
1e32767and1e-32767), there was in fact a measurable delay.Before tests, the entire
Numbertest suite took < 1ms (the granularity at which the test harness measured).I added 16 addition operation tests of such
Numbers (one for each combination ofMantissaScaleandRoundingMode). Together, they took about 80ms. So an average of about 5ms each. Still a pretty insignificant amount of time. Additionally, because theSTAmounts, which everySTNumberfield actually represents, have an exponent range of -96 to 80, any of these extreme values will not get very far in the transaction engine itself. Thus I conclude that the current behavior is not exploitable in any meaningful way. A 5ms addition operation is likely to be dwarfed by the I/O of loading the necessary ledger object for any transaction in which it is relevant.The fix is relatively straightforward: When shrinking the mantissa and increasing the exponent of one of the operands in an addition operation, if that operand reaches a state where the only possible change would be to increase the exponent, simply set that exponent to the exponent of the other (larger) operand, and exit the loop. The fact that digits were dropped is still preserved, so when rounding up, the result still rounds up to the next representable value.
With this fix, the entire
Numbertest suite is back to running in under 1ms.