perf: Speed up addition time for drastically different exponents - #7825
perf: Speed up addition time for drastically different exponents#7825ximinez wants to merge 16 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.
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! |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a focused, well-reasoned performance fix for Number::operator+= that short-circuits the digit-dropping loop once the shrinking operand's mantissa is exhausted and its Guard is full (no more information can be recovered), jumping directly to the target exponent instead of looping once per exponent difference. The fix is bounded correctly: the shortcut only fires when both mantissa == 0 and unrecoverable() (guard full) hold, which is guaranteed to happen within a small, fixed number of iterations (bounded by the mantissa's bit width and the Guard's fixed capacity) regardless of how large the exponent gap is — matching the reported ~1000x speedup with unchanged rounding semantics. The redundant targetExponent > exponent check inside the if (duplicating the debug-only XRPL_ASSERT) is defensive but harmless. Test additions (STNumber_test.cpp refactor to a shared throw-checking lambda, and the new extreme-exponent addition tests across all rounding modes in Number.cpp) look correct and directly validate the fixed behavior, including the Upward-rounding carry case. No correctness, security, or resource issues found in the changed lines.
There was a problem hiding this comment.
The change adds a short-circuit (doDropDigitWithTarget) to Number::Guard that jumps the shrinking operand's exponent directly to the target once its mantissa is fully zeroed and the guard holds no recoverable digits, avoiding a long O(exponent-delta) loop for additions with widely separated exponents. I traced through the logic: the short-circuit only fires when mantissa==0 and digits_==0, in which case further loop iterations would only push zero digits (no information loss, xbit_ unaffected), so jumping the exponent is behaviorally equivalent to looping. The first loop (trailing-zero shrink) is correctly left using doDropDigit since it can't reach mantissa==0 by construction. The accompanying tests (extreme opposite-exponent addition across all rounding modes, and the STNumber/Number test refactors) appear consistent with this reasoning and the described before/after timing. No correctness, security, or resource issues found in the diff.
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.