rippled
rippled copied to clipboard
XRPLF
Number merge
High Level Overview of Change
Use Number for IOUAmount and STAmount arithmetic
* Guarded by amendment fixUniversalNumber
* Produces slightly better accuracy in some computations.
Context of Change
There are two many decimal floating point classes in rippled, and AMM needed a third. This merges common code. A side effect is that some transaction arithmetic becomes more accurate in the final digit or two. Therefore this change is guarded by an amendment.
The first many commits introduce Number. Only the final commit merges the common code with IOUAmount and STAmount, and introduces the amendment.
Type of Change
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [x] Refactor (non-breaking change that only restructures code)
- [ ] Tests (You added tests for code that already exists, or your new feature included in this PR)
- [ ] Documentation Updates
- [ ] Release
Test Plan
Some tests are updated with the slightly more accurate results.
@zhangdongwei123, this repository is for the XRP Ledger and has nothing to do with radar.
@zhangdongwei123, you are asking at the wrong site. This site has no connection with radar. We don't know whether radar is working or not. Please find somewhere else to ask.
@HowardHinnant I'm seeing two unit tests fail (the first is this one: #2 failed: Taker_test.cpp(238)) I haven't dug into what that test is doing, but I wanted to make a note before I dug into the code.
This new commit fixes the failures you observed @seelabs. For reasons that are not clear to me, the Taker test ran with the amendment on in non-unity build, and with the amendment off in unity build. The patch forces the amendment on in the Taker test for both non-unity and unity builds.
For posterity, some notes re: 6c8d3abfb1d27dab7a03556ea943c5084e892311
I've tested on two versions of macOS: 10.14 and 13.2. The patch replaces the use of
__udivti3within theNumbermultiplication operator. I tested for correctness on over 55 quintillion values ofuint128_t, which is a tiny fraction of the total values that this type can hold. I concentrated the tests at and near 0, around the transition between the upper 64 bits going from zero to non-zero, and at and near the maximum value ofuint128_t. These tests were not included with the patch because they would be far too time consuming for--unittest. I also tested for performance on both macOS 10.14 and 13.2. On 10.14 the patch is a little over 30x faster than__udivti3. But on 13.2 it is the exact same speed. Not because the patch is slower, but because__udivti3is that much faster on 13.2 (for the divide by a constant 10 case). I do not know if the linux gcc tools have implemented such an optimization or not. So this may or may not improve the performance ofNumber::operator*=on linux. Although it will definitely eliminate the call to__udivti3.
Internal testing:
[We are] very pleased with the results. For one of the consensus phase I picked, it used to take 3051ms to apply 458 transactions before the fix but now it only took 1063ms to apply 475 transactions to the ledger.
We observed both have a ~ 3.6 better performance than previous build. ...
Submitresponse time also reduced significantly as well.
Just needs conflicts to be resolved @HowardHinnant . There is no need to rebase (although that is fine if that's what you prefer) - merge commits are OK since they will be squashed anyway.
Squashed and rebased to Develop (1.10.0-rc1). Builds and passes --unittest.