perf(operator): ⚡ index the term store with a persistent fingerprint table - #353
Draft
diagonal-hamiltonian wants to merge 3 commits into
Draft
diagonal-hamiltonian wants to merge 3 commits into
diagonal-hamiltonian wants to merge 3 commits into
Conversation
The store's own open-addressing index over row values is replaced by TermTable, a persistent table keyed on the top 32 bits of a term's routing fingerprint, behind the call sites main already had: the gate probe, the miss insert and the two by-value lookups. Slots are 4 bytes rather than the 8 of an (index, hash) pair -- a row index in the low log2(slots) bits, the next hash bits above it as a compare prefilter -- and no key is resident: the table folds a row's key off the row when it indexes it (OperatorIndex::key_of_row), streaming the store on a rebuild and reading the rows a gate has just written on an append. Every key match is confirmed against the query's positions, so a collision costs a compare and can never produce a wrong partner. The table is lazy and owned by MPOperator exactly as the inverted index is: materialised on first use, appended by reindex_after_growth, rebuilt by its staleness guard after a growth that bypassed that door. Two things do not move. Miss indices are still assigned base+j in query-stream order, so a gate's row layout is unchanged; and the by-value lookups (get_operator's init-map drain, update_initial_operator) go through the same table instead of a transient monomial map, measured neutral at 4.3 M terms. The key fold lives in the new operator/RowKey.h, so the store does not include the MPI routing header. indexing_bytes keeps its name and now reports the table: it is the index. At the L1 Hubbard cell (9.95 M terms) it is 64 MiB against 128 MiB, the ledger total 0.848x and VmHWM 0.899x, at 1.011x the propagate wall. Assisted-by: ClaudeCode:claude-opus-5
…e rows Seven cases over the persistent table: every stored row found under its own key whichever tier holds it, absent keys missing, growth holding the load bound while keeping every earlier row, and the staleness guard catching a growth that bypassed reindex_after_growth. Two of them are the properties the gate resolve rests on. The batched probe is compared to an unordered_map over the same rows, answer for answer and in the miss order the resolve assigns row indices in -- a miss takes base+j in query-stream order -- so a pipeline that reordered or lost a query would show up as a different row layout. And a key collision is constructed rather than hoped for: Gaussian elimination over the labels' top halves gives a term whose key is 0, so s and s ^ d share both a home slot and a prefilter, and only the row confirm can separate them. Assisted-by: ClaudeCode:claude-opus-5
The rank-routing section already derives the fingerprint; the join key is its top half, and this says what the table hashed by it costs, why the key is a plain projection rather than a mixed one (a receiver folds the same number off positions it decoded from the wire), why it is the high half, and that a structured collision is still only a prefilter. The ledger section names indexing_bytes, which reports that table. cpp/tests/README.md's operator-store bullet is rewritten for the cases that exist: term_table_tests.cpp, the row-key and block accessors in operator_index_tests.cpp, and bulk_insert_tests.cpp gone with the insert it tested. Assisted-by: ClaudeCode:claude-opus-5
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
github-actions
Bot
added
documentation
|
Docs preview: https://pr-353.monoprop-docs.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## perf/stack-3-sparse-exchange #353 +/- ##
===============================================================
Coverage ? 97.70%
===============================================================
Files ? 14
Lines ? 742
Branches ? 98
===============================================================
Hits ? 725
Misses ? 12
Partials ? 5
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
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
1 participant
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.
🤖 AI text below 🤖
Summary
Main indexes the term store with an open-addressing hash table of 8-byte slots keyed on the row's
folded value. This PR replaces it with
TermTable(new, 4-byte fingerprint-keyed slots, batchedprobe), sitting behind main's own accessor protocol:
find_batch,find_batch_positions,find,and
emplace/bulk_insert/bulk_insert_hashedare replaced at their call sites byTermTable::find_batch/findandMPOperator::index_appended_rows(main's existingreindex_after_growthdoor), without changing what the protocol does. The join key itself isfolded off the row by free functions in a new
RowKey.h(join_tag,key_of_positions,key_of),so
OperatorIndexdepends on routing through exactly one header.Two measured optimizations land alongside the table: an empty-table early return in
find_batch(two branches per query removed), and the two by-value operator lookups going throughTermTable::finddirectly rather than building a store-wideTermLookupmap — dropping thattransient structure entirely, with no rebuild forced and no timing cost (see Measurements).
This is PR 4 of 7, based on
perf/stack-3-sparse-exchange. The next PR(
perf/stack-5-one-round-join) is the protocol rewrite this table exists to serve. This level's time budget was 1.05× main; its worst rung is within noise.Changes
Engine
cpp/monoprop/detail/operator/TermTable.h(new, 274 lines): 4-byte slots, fingerprint key,batched probe;
find_batchearly-returns on an empty table.cpp/monoprop/detail/operator/RowKey.h(new, 58 lines):join_tag,labels<NumBits>(),key_of_positions<NumBits>(pos, k),key_of(bitset)as free functions.cpp/monoprop/detail/operator/OperatorIndex.h(860 → 619 lines):Slot/Table/find*/bulk_insert*/emplace/fold_hash*deleted;key_of_row(i),RowBlock/row_block/positions_at, publicrow_eq_positionsadded.for_eachnow walks rows in index order (thetable it used to walk is gone) rather than main's table-slot order.
cpp/monoprop/detail/operator/MPOperator.h: lazily rebuiltterm_table_;insert_incoming_misses/insert_deferred_self_missesdrop thebulk_insert_hashedcall andkeep the existing
reindex_after_growthdoor; the two by-value lookups go throughTermTable::find(store, mono).cpp/monoprop/detail/evolution/layer_build/Engine.h,Resolve.h: minimal adapter — fold the keyper query, call
find_batch; the protocol itself is untouched..../monomial_propagator/MonomialPropagator.inl: drops theemplacecall from construction.Tests
cpp/tests/term_table_tests.cpp(new, 424 lines): the batched probe pinned directly (5 cases plusa collision and a staleness-guard case) against a dense-map oracle over the same rows.
cpp/tests/operator_index_tests.cpp: index cases replaced with key/for_each/row_eq_positions/row_blockcases (row_block_agrees_with_the_per_row_accessors).cpp/tests/bulk_insert_tests.cppdeleted (188 lines) — main'sbulk_insert/bulk_insert_hashedare removed from the store's public surface by this PR.cpp/tests/mp_operator_tests.cpp,evolution_detail_tests.cpp,mpi_utils_tests.cpp,sparse_resolve_tests.cpp,simulator_copy_tests.cpp: adapted to the new store signatures.cpp/tests/README.md: "Operator store" section rewritten fresh.Docs
docs/content/docs/features/parallelism.mdx: the join key and persistent term table paragraphs.Measurements
Gated multiset vs origin/main c5e88c8's raw bits:
for_each's new index-ordered walk moves thegolden monomial WALK ORDER (
monoseq) at everyP, though it moves no value and no term set —routing already required multiset from PR 2 onward, but this PR would need it independently even at
P=1.gates.shrecord md5a7a612809122770771d1d9f8604a14ff(rebased onto PR 3, tip69a137d).3 interleaved reps, ratios only:
Time is within the noise floor on every rung, comfortably inside this level's 1.05× time budget —
the worst intermediate time this level was flagged to risk did not materialize;
TermTable::find_batchbehind main's protocol costs nothing measurable.indexing_bytes(thefield this PR redefines) is exactly 0.500× main on every rung (8-byte slots → 4-byte), which is
also most of this PR's peak-RSS win on top of PR 1's store gain.
O6 (dropping
TermLookup.hfor by-value lookups throughTermTable::find) was measuredseparately, not inferred:
update_initial_operatorcame out at 1.0005× and 0.991× the predecessor(PR 3) across two interleaved rounds at 4.32 M terms — no measurable difference;
get_operatorforces no rebuild because it only reaches the table while
init_op_mapis non-empty, drained afterthe first call.
Notes for reviewers
RowBlock/row_block/positions_atland here but have no engine consumer until PR 5(
perf/stack-5-one-round-join), whereScan.huses them — this is the one piece of this PR thatnothing in this PR calls;
coverage is
row_block_agrees_with_the_per_row_accessorsagainst the per-row accessors directly.indexing_byteskeeps its name and now reports the term table (it is the index now) —no
term_table_byteskey was added, and no vestige field was kept. Main also folded insizeof(OperatorIndex)(~200 B); this PR drops that rather than keeping it as an unused vestige.bulk_insert_tests.cppis deleted outright, not adapted, because the calls it tested no longerexist on the store's public surface.
_<digits>exclusion pattern (cpp/tests/boostAddTests.cmake:209drops any case name matching it) would have been silently never registered with ctest; named to
avoid it from the start (
term_table_settles_a_key_collision_by_the_row_confirm).grow_rows_geometric+set_positions, and misses still takebase + jin query-stream order(pinned against an unordered_map oracle in
term_table_tests.cppand against the dense insertpath in
sparse_resolve_tests.cpp). The table is a prefilter plus a positions confirm, so itanswers exactly what main's value-keyed index answered for any query; a 32-bit key collision costs
one extra compare (constructed and pinned in tests).
Checklist
docs/,CONTRIBUTING.md) if neededCHANGELOG/ release notes updated if applicable (n/a — the repository has noCHANGELOG)AI/LLM disclosure
Important
By opening this PR I confirm that I have read CONTRIBUTING.md and I agree to the terms of the Contributor License Agreement.
Warning
If you're contributing on behalf of your employer, contact cla@algorithmiq.fi to arrange a Corporate CLA.