Add the shape-pattern transposition table and make it the default - #368
Open
tameware wants to merge 10 commits into
Open
Add the shape-pattern transposition table and make it the default#368tameware wants to merge 10 commits into
tameware wants to merge 10 commits into
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.
🟡 Changes recommended
Pool metadata growth can violate the configured hard memory cap.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
release_tree() now checks, when the pointer vector would have to grow, that the growth fits under the hard cap; otherwise the block is returned to the allocator instead of pooled. With the growth reserved up front, push_back cannot throw. A strict-cap test asserts memory_in_use() <= maximum after every add on a deep, block-heavy workload with no slack for pool bookkeeping. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Environment-resolved kind handling is inconsistent during reconfiguration, and the base transposition-table documentation remains outdated.
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/solver_context/solver_context.cpp:290
- Compare against the environment-resolved kind. With
DDS_TT_KIND=pattern, callingconfigure_tt(Small, ...)currently destroys and recreates the cache even though the effective kind remains Pattern; if the environment changes to a new override while the configured kind equals the current table, this branch instead leaves the old implementation in place. Resolving the requested kind here keepsconfigure_ttconsistent with the documented override and recreation semantics.
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
…leP. configure_tt() now resolves the DDS_TT_KIND override before deciding whether to recreate the table, as creation does, so a request that leaves the effective kind unchanged resizes in place and a changed override recreates even when the configured kind is the same. The TransTable base doxygen now lists all three implementations and their memory strategies. Co-authored-by: Cursor <cursoragent@cursor.com>
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