Conversation
TransTableP keys positions by suit-length shape and stores, under each shape, the relative-rank patterns that decided the result (the cards at or above the lowest winning rank per suit, by owner), following the cache design of macroxue's bridge-solver. Patterns are ordered most general first and bucketed by the owner of the first relevant suit's top card, so a lookup scans only the buckets it can match. Blocks are cache-line aligned and pooled per size class; when the memory maximum is reached the table is cleared rather than harvested. Results are identical to TransTableL. Performance is at parity on random deals and markedly better on void-heavy deals and under tight memory limits, where TransTableL's fixed per-shape blocks overflow and lookups degrade to long linear scans. TTKind::Pattern (2) is the new SolverConfig default; DDS_TT_KIND= small|large|pattern overrides it. Specs and C API docs updated. Co-authored-by: Cursor <cursoragent@cursor.com>
alignas(CacheLine) rounded the 76-byte header up to 128 bytes implicitly, which MSVC reports as C4324 and /WX turns into an error. Make the padding an explicit member and static_assert the resulting layout, so the block header is identical on every compiler. Co-authored-by: Cursor <cursoragent@cursor.com>
Collaborator
Author
Collaborator
Author
|
Multithreaded benchmark: |
Collaborator
Author
|
Single-threaded benchmark: |
setenv/unsetenv do not exist on MSVC; use _putenv_s there, as args_test already does. Co-authored-by: Cursor <cursoragent@cursor.com>
Collaborator
Author
|
"Freak" deal timing: develop: This branch: bridge-solver: |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Duplicate inserts can trigger unnecessary table resets, and lowering the hard memory limit is not immediately enforced.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds the shape-pattern transposition table, makes it the default, and exposes configuration through existing APIs.
Changes:
- Implements pooled, shape-keyed relative-rank pattern caching.
- Adds comprehensive table and API tests.
- Updates defaults, build targets, and documentation.
File summaries
| File | Description |
|---|---|
specs/transposition-table.md |
Documents Pattern table behavior. |
specs/solver-context.md |
Documents the new default. |
library/tests/trans_table/trans_table_p_test.cpp |
Tests matching, storage, memory, and parity. |
library/tests/trans_table/BUILD.bazel |
Registers Pattern table tests. |
library/tests/system/configure_tt_api_test.cpp |
Tests configuration and environment overrides. |
library/tests/dds_c_api_test.cpp |
Tests Pattern through the C API. |
library/src/trans_table/trans_table_p.hpp |
Declares TransTableP. |
library/src/trans_table/trans_table_p.cpp |
Implements pattern caching and memory management. |
library/src/trans_table/BUILD.bazel |
Adds Pattern sources to build targets. |
library/src/solver_context/solver_context.hpp |
Adds TTKind::Pattern and changes the default. |
library/src/solver_context/solver_context.cpp |
Creates and configures Pattern tables. |
library/src/api/dds_c_api.h |
Documents Pattern’s C ABI value. |
docs/c++_interface.md |
Updates C++ configuration documentation. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
set_memory_maximum() on a live TransTableP that is already above the new limit now clears the table immediately instead of leaving it over budget until the next block allocation; inserts into blocks with spare capacity never consult the budget, so without this a lowered hard cap could go unenforced indefinitely. add() now searches the bucket for an identical pattern before reserving capacity. Re-adding an existing pattern to a full block used to double the block, or at the memory limit clear the whole table, for an update that needed no allocation. Spec: distinguish ordinary resets, which pool the pattern blocks, from MemoryExhausted resets, which also free the pool. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Memory-exhaustion and teardown paths retain storage and may allocate beyond the configured hard cap.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
library/src/trans_table/trans_table_p.cpp:155
return_all_memory()leaves substantial dynamic storage behind:ownership_keeps its 8,192-entry capacity, andfree_spare_trees()only clears each pointer vector without releasing its backing allocation. This violates the base-class contract that all structures are deallocated and leavesmemory_in_use()at roughly 384 KiB even after the call. Release these vector capacities as well.
auto TransTableP::return_all_memory() -> void
{
release_trees();
free_spare_trees();
std::vector<ShapeSlot>().swap(shapes_);
}
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
…own. A MemoryExhausted reset used to push every active block into the spare pool, which may allocate, only to delete the pool immediately. It now deletes the blocks outright, so the over-budget recovery path never allocates. make_tt() uses the same path. return_all_memory() now also drops the ownership table and the pool vectors' capacity, so memory_in_use() is zero afterwards as the base contract requires; init() rebuilds the ownership table per deal anyway. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Shape-table allocation can transiently exceed the documented hard memory cap, and environment tests leak process state.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
library/tests/system/configure_tt_api_test.cpp:54
- This permanently removes a caller-provided
DDS_TT_KIND, so subsequent tests in this executable no longer observe the environment they were launched with and become order-dependent. Use a guard that captures and restores the original value (asargs_test.cpp:98does);ScopedEnvabove must likewise restore the previous value rather than always unsetting it.
library/src/trans_table/trans_table_p.cpp:440
- This growth check budgets only the final replacement table, but
freshis allocated while the oldshapes_storage is still live. A rehash therefore exceeds the advertised hard cap by the entire old table and can fail under the memory pressure the cap is meant to control. Include current dynamic usage plus the new allocation in the check so the table resets when there is insufficient peak headroom.
if (new_size * sizeof(ShapeSlot) + tree_bytes_ > maximum_bytes_) {
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
reset_memory() now frees the old shape table before allocating the fresh one, and grow_shapes() budgets the peak of old and new tables together, so neither path allocates beyond the configured maximum. configure_tt_api_test's ScopedEnv restores the previous value of the variable (or its absence) instead of always unsetting it, and the default-kind test uses it rather than unsetting DDS_TT_KIND for the rest of the process. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The owning table remains implicitly copyable, and one allocation-failure path leaks a newly allocated tree.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
library/src/trans_table/trans_table_p.cpp:419
release_tree(old)can allocate while growing the spare-vector and throw afterfreshhas been allocated but before it is attached toslot. On that pathfreshis unreachable, so its aligned allocation leaks andtree_bytes_remains permanently inflated. Clean upfreshif pooling the old block fails (or hold it in an RAII owner until commit).
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
The table owns raw pattern blocks, so the implicit copy operations would alias and later double-free them; delete them. reserve_one_more() now attaches the grown block to its slot before pooling the old one, and deletes the old block if pooling throws, so an allocation failure at that point leaks nothing and keeps the byte accounting exact. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Spare-pool bookkeeping is excluded from hard-limit accounting, allowing retained allocation to exceed the configured maximum.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
library/src/trans_table/trans_table_p.cpp:174
- The hard-cap calculation omits the backing allocations owned by
spare_trees_. Those vectors grow when old blocks are pooled, andfree_spare_trees()only callsclear(), so a large pooled table can retain megabytes of pointer storage after aMemoryExhaustedreset or a lower maximum whiledynamic_bytes()reports that the cap is satisfied. Include the spare-vector capacities in memory accounting and release or budget their capacity on hard-cap paths.
library/src/trans_table/trans_table_p.cpp:26 - This says equal-weight patterns are newest-first, but insertion walks past all equal-weight nonmatches before inserting, so they remain oldest-first. Update the comment (or change insertion ordering) so the documented lookup order matches the implementation.
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…g note. dynamic_bytes() now includes the capacity of the spare-block pointer vectors, and free_spare_trees() (over-budget and teardown paths) releases that capacity as well, so nothing the table retains escapes the hard cap. Patterns of equal weight are scanned oldest first, as the insertion code has always done; the file comment claimed newest first. A test now pins the actual order. Co-authored-by: Cursor <cursoragent@cursor.com>
Collaborator
Author
|
Addressed the two suppressed notes from the last review in 0338f36: |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Two moderate configuration and lifecycle issues in solver_context.cpp remain unresolved.
Review details
Suppressed comments (2)
library/src/solver_context/solver_context.cpp:289
- When an already-created Pattern table is reconfigured with the unset
defMB/maxMBvalues (for exampleconfigure_tt(TTKind::Pattern, 0, 0)), this same-kind path forwards zero toresize_tt. Lazy creation resolves zero to the built-in limits, butTransTableP::set_memory_maximum(0)leavesmaximum_bytes_ == 0, soreserve_one_more()/grow_shapes()treat every allocation as over-budget and repeatedly clear the table. Normalize non-positive limits to the effective defaults (includingDDS_TT_LIMIT_MB) before an in-place resize, or preserve the resolved live limits.
if (tt_kind_of(tt) != tt_kind_from_environment(kind)) {
library/src/solver_context/solver_context.cpp:292
- Recreating the table here loses the deal-specific
init(hand_lookup)state. If this happens after a solve and the next solve uses the same deal,newDealis false soSetDealTablesis skipped; the freshTransTablePhas an emptyownership_, making every lookup miss and every add return attrans_table_p.cpp:606-610. Reinitialize a newly created table for the current deal (or otherwise preserve/replay the hand lookup) so reconfiguration does not silently disable the cache.
if (tt_kind_of(tt) != tt_kind_from_environment(kind)) {
dispose_trans_table();
// Force immediate creation with new config to keep behavior explicit.
(void)trans_table();
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
…ed TT on the next solve - configure_tt() runs the same fill_unset_limits() as lazy creation, so configure_tt(kind, 0, 0) on a live table no longer sets a zero maximum that clears the table on every allocation (test first). - dispose_trans_table() forgets the thread's remembered deal, so a table recreated between two solves of the same deal is init()-ed by the next solve instead of staying inert (test first, end to end via solve_board_pbn). Co-authored-by: Cursor <cursoragent@cursor.com>
Collaborator
Author
|
Suppressed findings from review 5185961798, both confirmed and fixed in c996b81:
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Cached patterns must be cleared when ownership is reinitialized to prevent results from a previous deal being reused.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Lite
…on the relevant cards Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The change modifies core solver caching behavior, and a documentation discrepancy remains unresolved.
Review details
Suppressed comments (1)
specs/transposition-table.md:74
- This paragraph still says the header documents
0as "unlimited" and that reconciling the Doxygen is out of scope, but this PR updatestrans_table.hpp:134-136to explicitly say0is unsupported. The spec now contradicts the API documentation; remove the stale header claim.
header's doxygen is out of scope here.) On `TransTableP` only the maximum is a
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Cursor <cursoragent@cursor.com>
Collaborator
Author
|
Suppressed finding from review 5186024081: the spec paragraph no longer claims the header calls 0 "unlimited"; it now says 0 is unsupported, matching the updated |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The broad new default implementation warrants final human review, including clarification of the reset-memory documentation contract.
Review details
Suppressed comments (1)
library/src/trans_table/trans_table.hpp:97
- This new implementation-specific memory section does not reconcile the inherited
TransTable::reset_memory()contract below, which still says that reset structures are retained for reuse.TransTableP::reset_memory(ResetReason::MemoryExhausted)explicitly deletes active and pooled blocks, so the public Doxygen is contradictory for the new default implementation; qualify the base reset contract (or otherwise document the reason-specific exception).
/// Implementations use different memory strategies. TransTableP grows on
/// demand and clears itself when the next allocation would exceed the maximum,
/// TransTableL uses paged memory with harvesting, and TransTableS uses a
/// pool-based approach with malloc/calloc. All support configurable memory
/// limits and graceful degradation.
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
…ansTableP Co-authored-by: Cursor <cursoragent@cursor.com>
Collaborator
Author
|
Suppressed finding from review 5186170629: the base |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes the default solver cache across core logic, lifecycle, API, tests, and build systems, warranting final human review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It replaces the default hot-path cache with substantial new solver-critical memory and matching logic that warrants final human review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Balanced
tameware
added
the
Clean Copilot review
zzcgumn
approved these changes
Sep 12, 2026
zzcgumn
left a comment
zzcgumn
left a comment
Collaborator
There was a problem hiding this comment.
This feels promising. At some point we will have to consolidate to only have type of transposition table.
tameware
deleted the
fable/bridge-solver-shape-pattern-transposition
branch
September 12, 2026 17:29
wopdevries
pushed a commit
to wopdevries/dds
that referenced
this pull request
Sep 12, 2026
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.
Summary
TransTableP, a transposition table organised as suit-length shape → relative-rank patterns, following the cache design of macroxue's bridge-solver. A pattern records only the cards that decided a result (those at or above the lowest winning rank per suit, by owner), so one entry generalises to many positions.TTKind::Pattern(2) theSolverConfigdefault.DDS_TT_KIND=small|large|patternoverrides it at runtime. C API signatures unchanged (tt_kinddoc updated).specs/transposition-table.md,specs/solver-context.md,docs/c++_interface.md.Bridge-solver's subsumption tree was implemented first and profiled; it was a net loss for DDS's access pattern, so the flat generic-first list is what ships.
Results
Identical answers to
TransTableLon list100, list1000 and the freak0 deal.Parity on random deals; large wins on void-heavy deals and under memory pressure, where
TransTableL's fixed per-shape blocks overflow (61% of adds on freak0) and lookups degrade to ~94-entry linear scans.Not in this PR
SolverConfigstill defaults toTTKind.Largeand its enum lacksPattern = 2; the web WASM build pinsSmall.Test plan
bazelisk test //...(93/93)//library/tests/trans_table:trans_table(trans_table_p_test.cpp)configure_tt_api_test: default kind, Pattern create/switch/resize, env override;dds_c_api_testwithtt_kind = 2dtest -s solve/calconhands/list100.txt,hands/list1000.txtand freak0 withDDS_TT_KINDunset,large,pattern: no differences