Fix small performance regression versus v2.9#214
Closed
tameware wants to merge 157 commits into
Closed
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to regain a small (~2%) performance regression vs v2.9 by removing redundant/narrowing casts and simplifying a hot-path heuristic branch, reducing unnecessary conversions and work in frequently executed code paths.
Changes:
- Removes redundant
static_cast<unsigned char>(...)/ double-cast patterns when indexing rank/relative-rank tables. - Simplifies part of
weight_alloc_trump_void1()for the “void in trump when trump is led” discard case. - Removes unnecessary casts when copying winner/second-best ranks and hands from
RelRanksTypeintoPos.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| library/src/quick_tricks.cpp | Simplifies rank-to-index conversion when setting win_ranks from AbsRankType::rank. |
| library/src/heuristic_sorting/heuristic_sorting.cpp | Removes redundant casts in rel_rank indexing and simplifies a discard-weighting branch in a hot heuristic function. |
| library/src/ab_search.cpp | Removes unnecessary casts when updating cached winner/second-best info from thrp->rel[...]. |
zzcgumn
reviewed
Jun 29, 2026
| int suitAdd; | ||
|
|
||
| if (suit == trump) | ||
| if (lead_suit == trump) // We pitch |
Collaborator
There was a problem hiding this comment.
This confuses me, is this not the same fix as the previous pull request?
Collaborator
Author
There was a problem hiding this comment.
Would you like me to investigate and address, or can this be dealt with in merge?
Collaborator
There was a problem hiding this comment.
Can you try to rebase or merge this branch with latest develop?
Collaborator
Author
|
Entirely possible! I sometimes get my branches confused.
…On Mon, Jun 29, 2026 at 2:19 PM Martin Nygren ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In library/src/heuristic_sorting/heuristic_sorting.cpp
<#214 (comment)>:
> @@ -676,49 +676,17 @@ void weight_alloc_trump_void1(HeuristicContext& ctx)
unsigned short suitCount = tpos.length[curr_hand][suit];
int suitAdd;
- if (suit == trump)
+ if (lead_suit == trump) // We pitch
This confuses me, is this not the same fix as the previous pull request?
—
Reply to this email directly, view it on GitHub
<#214?email_source=notifications&email_token=ABC4PYHJOQ2O32PTZTFJNFD5CK6MFA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJZGQ3TGMJVGEYKM4TFMFZW63VGMFZXG2LHN2SWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-4594731510>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABC4PYBWNK44RE4QNRS4XGT5CK6MFAVCNFSNUABEKJSXA33TNF2G64TZHMZDMOJYHAZDANR3JFZXG5LFHM2DONRSGQ3DINRTGWQXMAQ>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Pass the resolved dds_mvp.js path via DDS_MVP_JS for Bazel runfiles, and add focus() to mock DOM elements so clearTestData and pageLoad work when merged with web changes. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Add upload-artifact@v6 steps to Linux, macOS, Windows, and WASM workflows using bazel-testlogs/ with if-no-files-found: ignore. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
docs/BUILD_SYSTEM.md — new section “AddressSanitizer and ThreadSanitizer (macOS)” covering: The three-way clang major coupling (MODULE.bazel, Xcode, .bazelrc TSAN rpath) Why ASAN uses the full Xcode toolchain vs TSAN using LLVM compile + Xcode runtime Steps when upgrading LLVM or Xcode Smoke-test commands Also corrected the outdated LLVM version note (darwin was listed as 20.1.8; it’s 21.1.8). .bazelrc — short pointers to BUILD_SYSTEM.md; TSAN rpath comment notes the hardcoded clang/21 must stay in sync. MODULE.bazel — comment on llvm_versions to update the TSAN rpath and verify Xcode when bumping LLVM.
…arking global arrays as static. Commented out unused SORT_SOLVE_STRENGTH and SORT_SOLVE_STRENGTH_CUTOFF
The BUILD change ensures a stats test links only system_util_stats, not system + system_util_stats. One system variant → one ThreadMgr → one set of mutexes. That matches the intent: a process-wide thread manager.
Wire a Bazel config_setting into DDS_LOCAL_DEFINES so AB stats can be turned on without --cxxopt=-DDDS_AB_STATS; refresh MODULE.bazel.lock. Co-authored-by: Cursor <cursoragent@cursor.com>
Pass dtest_effective_threads through to calc batches so calc threading matches solve and responds to the -n flag. Co-authored-by: Cursor <cursoragent@cursor.com>
SetResources ignores maxThreads; threading is handled via dtest_effective_threads and the *N batch APIs. Passing -n only triggered a misleading library warning. itest.cpp will be removed in a future PR.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The file was excluded from the dtest build, had no Bazel target, and referenced a nonexistent realMain entry point. Co-authored-by: Cursor <cursoragent@cursor.com>
Dereference the transposition-table NodeCards pointer to match the DumpRetrieved(const NodeCards&) signature when DDS_AB_HITS is enabled. Co-authored-by: Cursor <cursoragent@cursor.com>
Re-add the DDS_AB_HITS dump helpers in dump.cpp so debug_all links succeed when ab_search logs transposition-table hits. Co-authored-by: Cursor <cursoragent@cursor.com>
The heuristic extraction refactor changed weight_alloc_trump_void1's first branch from `lead_suit == trump` to `suit == trump`. Since that is exhaustive with the following `else if (suit != trump)`, the three ruffing branches (using the `24 - rank + ...` formula) became dead code, and trump ruffs were scored with side-suit discard weights instead. This mis-ordered ruffs, costing alpha-beta cutoffs. The effect is small for solve but compounds heavily in calc's warm-TT iterative deepening: calc explored ~34% more nodes than v2.9. Restoring the original `lead_suit == trump` pitch branch makes the ruffing branches reachable again and cuts calc time ~25% (gap to v2.9: 1.37x -> 1.02x). Ordering-only change; double-dummy results are unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
Per Copilot. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The heuristic/quick-tricks refactor introduced static_cast<unsigned char>
wrappers on values that v2.9 used as signed, changing search behavior:
- make_3 / make_3_ctx: winner[]/second_best[] .hand and .rank were cast
to unsigned char, turning the -1 "no card" sentinel into 255. This broke
winner[trump].hand == -1 style checks in QuickTricks, losing cutoffs.
- weight_alloc_trump_void2 / _void3: rel_rank[aggr[suit]][...] indexed
through static_cast<unsigned char>(aggr[suit]), truncating the 13-bit
aggregate holding to 8 bits and reading the wrong rel_rank row.
- QuickTricksPartnerHand{Trump,NT}: bit_map_rank index cast the signed
rank through unsigned char.
With these reverted to v2.9's signed handling, the per-move-generation
ordering trace now matches v2.9 exactly (0 divergences on list1), closing
the residual calc gap to parity. Ordering/pruning-only change; double-dummy
results are unchanged and all library tests pass.
Co-authored-by: Cursor <cursoragent@cursor.com>
Whitespace-only cleanup of misindented if/else-if chains and wrapped conditions left over from the v2.9 port; no logic change. Co-authored-by: Cursor <cursoragent@cursor.com>
tameware
force-pushed
the
opus-two-percent
branch
from
6234595 to
b8143cd
Compare
Collaborator
Author
|
The rebase was a disaster. I made a new PR #223 instead. |
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.
Regains roughly 2% of performance.
Compare is the speedup-opus branch. Branch is this branch.