Dag avoid float conversions and fix rare state transition bug#2385
Dag avoid float conversions and fix rare state transition bug#2385Menkib64 wants to merge 6 commits intoLeelaChessZero:masterfrom
Conversation
Q and D are stored as doubles in the tree. The computation converts them to floats. Conversions have a potential for accuracy lose when calculating delta updates.
Divisions are slow instructions. It makes sense to convert repeated divisions with the same number to multiplications.
Transposition status can change while picking is happening. This breaks multiple calls in PickNodesToExtendTask. PR LeelaChessZero#2318 already fixes the issue but it hasn't made it into master yet. This is a quick temporary fix until LeelaChessZero#2318 is merged.
There was a problem hiding this comment.
Pull request overview
This PR improves backup propagation performance and fixes a rare race condition in the DAG search implementation. The changes convert internal evaluation values from float to double to avoid unnecessary type conversions during backup propagation, and optimize the score update calculations by pre-computing division operations. Additionally, the PR addresses a potential n_in_flight underflow issue by adjusting the transposition comparison logic.
Changes:
- Convert WL and D values from float to double throughout Node, LowNode, and related functions to reduce conversions
- Refactor FinalizeScoreUpdate and AdjustForTerminal to optimize division operations by pre-computing divisors
- Fix race condition in transposition detection by changing
n->GetN() < nl->GetN()ton->GetN() + 1 < nl->GetN()
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/search/dag_classic/search.h | Updated function signatures to use double for v/d parameters and return types |
| src/search/dag_classic/search.cc | Added WDLRescale template, fixed race condition in transposition check, removed redundant IsTransposition check in ShouldStopPickingHere |
| src/search/dag_classic/node.h | Changed Eval struct and Node/LowNode methods to use double for wl/d values, updated function signatures |
| src/search/dag_classic/node.cc | Refactored FinalizeScoreUpdate to return divisor, moved n_ increment before calculation, optimized division operations, updated wld_tolerance type |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| static constexpr float wld_tolerance = 0.000001f; | ||
| static constexpr double wld_tolerance = 0.000001f; |
There was a problem hiding this comment.
The variable wld_tolerance is declared as double but initialized with a float literal suffix f. For consistency with the type declaration, use a double literal without the f suffix: 0.000001 instead of 0.000001f.
| static constexpr double wld_tolerance = 0.000001f; | |
| static constexpr double wld_tolerance = 0.000001; |
Small backup propagation improvements which might improve search slightly based on local testing. The difference is within error margin because I haven't run enough test games. Changes mainly aim to make backup propagation a little faster because single threaded code has relatively high impact to CPU bottleneck.
Rare picking node race condition was already fixed in PR #2318. It is still pending. This pull request includes a simplified workaround to avoid rare cases where n_in_flight underflows.