perf: optimize division to uint32 to make re-target-pow calculation much faster#7352
perf: optimize division to uint32 to make re-target-pow calculation much faster#7352knst wants to merge 2 commits into
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis PR adds scalar division operator overloads to the Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/arith_uint256.cpp (1)
76-77: ⚡ Quick winParameter name
b32is misleading foruint64_ttype.The parameter is named
b32but the function acceptsuint64_t. This inconsistency appears in both the declaration (line 161 of arith_uint256.h) and implementation. Consider renaming tob64orbfor clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/arith_uint256.cpp` around lines 76 - 77, Rename the misleading parameter b32 in the division operator to a name that matches its type and intent (e.g., b64 or simply b) in both the declaration and implementation of template<unsigned int BITS> base_uint<BITS>::operator/=(uint64_t b32); update the parameter name in the function signature and all uses inside the implementation so they remain consistent (search for base_uint<BITS>::operator/= and the declaration in the header to change both places).src/arith_uint256.h (1)
161-161: ⚡ Quick winParameter name
b32is misleading foruint64_ttype.The parameter is named
b32but acceptsuint64_t. While the implementation optimizes for values ≤uint32_tmax, the parameter type is 64-bit and the name could cause confusion. Consider renaming tob64or simplyb(as used in line 211) for clarity.✏️ Suggested naming improvement
- base_uint& operator/=(uint64_t b32); + base_uint& operator/=(uint64_t b);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/arith_uint256.h` at line 161, Rename the misleading parameter `b32` in the declaration of operator/= to a clearer name (e.g., `b` or `b64`) and update the corresponding definition/implementation to use the same name; specifically modify the signature `base_uint& operator/=(uint64_t b32);` to `base_uint& operator/=(uint64_t b);` (or `b64`) and change the implementation function parameter and any internal references to match, and then update any callers/forward-declarations that reference the old name to keep declarations and definitions consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/arith_uint256.cpp`:
- Around line 76-77: Rename the misleading parameter b32 in the division
operator to a name that matches its type and intent (e.g., b64 or simply b) in
both the declaration and implementation of template<unsigned int BITS>
base_uint<BITS>::operator/=(uint64_t b32); update the parameter name in the
function signature and all uses inside the implementation so they remain
consistent (search for base_uint<BITS>::operator/= and the declaration in the
header to change both places).
In `@src/arith_uint256.h`:
- Line 161: Rename the misleading parameter `b32` in the declaration of
operator/= to a clearer name (e.g., `b` or `b64`) and update the corresponding
definition/implementation to use the same name; specifically modify the
signature `base_uint& operator/=(uint64_t b32);` to `base_uint&
operator/=(uint64_t b);` (or `b64`) and change the implementation function
parameter and any internal references to match, and then update any
callers/forward-declarations that reference the old name to keep declarations
and definitions consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bf974529-fc8b-450b-857a-fe88d01cd909
📒 Files selected for processing (3)
src/arith_uint256.cppsrc/arith_uint256.hsrc/test/arith_uint256_tests.cpp
|
✅ Review complete (commit e8435d3) |
| } | ||
|
|
||
| template <unsigned int BITS> | ||
| base_uint<BITS>& base_uint<BITS>::operator/=(uint64_t b32) |
There was a problem hiding this comment.
nit: is it a typo?
| base_uint<BITS>& base_uint<BITS>::operator/=(uint64_t b32) | |
| base_uint<BITS>& base_uint<BITS>::operator/=(uint64_t b64) |
same in other places
There was a problem hiding this comment.
not really a typo, because it's supposed optimized for b32 only. If you think that's confusing I'd rename it.
It is supposed to be uint32_t b32 but compiler won't say a word if that's called with uint64_t, so, I keep it mixed.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, well-scoped performance PR: adds a uint64_t divisor overload to base_uint with a fast word-wise long-division path when the divisor fits in uint32_t, falling back to the existing base_uint divisor otherwise. The schoolbook radix-2^32 loop is correct (the running remainder is always < b32 ≤ 2^32, so (rem<<32)|pn[i] fits in uint64_t), zero is rejected, and the regression test asserts bit-identity against the widened arith_uint256 divisor across both code paths plus the zero-throw case. Both agents and CodeRabbit found no in-scope issues.
Issue being fixed or feature implemented
Dash retargets difficulty every block via DarkGravityWave (24-block loop), so every header processed during sync runs several 256-bit divisions. Bitcoin only retargets once per 2016 blocks.
Every division in DGW is by a small integer (nCountBlocks+1) * nTargetTimespan (3600).
But the code routes through the only available overload, operator/=(const base_uint&), which does bit-by-bit long division: ~224 iterations, each doing a full-width
div >>= 1Perf shows that during header sync up to 35% of CPU is wasted on this division on release build (accordingly
perf):What was done?
Implemented optimization for division to unsigned values that could fit inside uint32_t).
How Has This Been Tested?
Got GUIX build from changes in this PR and develop:
Perf stats are updated as expected:
So total header sync just a half of the time. Please note, that header sync will be slightly slower due to #7320 so overall win is smaller.
Breaking Changes
N/A
Checklist: