Conversation
|
Docs preview: https://pr-303.monoprop-docs.pages.dev |
403debb to
8ded3bd
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #303 +/- ##
=======================================
Coverage 97.70% 97.70%
=======================================
Files 14 14
Lines 742 742
Branches 98 98
=======================================
Hits 725 725
Misses 12 12
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more. |
8ded3bd to
e4b1217
Compare
e4b1217 to
8ba993f
Compare
8ba993f to
1b55f23
Compare
1b55f23 to
fe0c971
Compare
fab96fb to
99a4c81
Compare
99a4c81 to
e271733
Compare
e271733 to
b9af47b
Compare
1a0b8be to
d99049a
Compare
|
Warning Review limit reachedNext included review available in 17 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Thanks 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 |
ludmilaasb
left a comment
There was a problem hiding this comment.
LGMT
Just remove the narrative comments and I think we can merge.
| // Operator-term store: entropy-packed position-list rows plus a RowHashTable keyed over them. The rows | ||
| // are this class's business and the index is not: nothing below reads a slot, and nothing in | ||
| // RowHashTable reads a row -- the two meet only through the hash and equality callables passed in. | ||
| // Row layout: slot 0 = popcount c (or kOverflowMarker if c > inline_width_), slots 1..c = |
There was a problem hiding this comment.
I think we can remove this
| // Operator-term store: entropy-packed position-list rows plus a RowHashTable keyed over them. The rows | |
| // are this class's business and the index is not: nothing below reads a slot, and nothing in | |
| // RowHashTable reads a row -- the two meet only through the hash and equality callables passed in. | |
| // Row layout: slot 0 = popcount c (or kOverflowMarker if c > inline_width_), slots 1..c = |
| // The keyless open-addressing index a row store puts over its rows: power-of-2 slot count, linear | ||
| // probing, max load factor 0.7 (the group-prefetch win erodes at higher load -- longer probe chains add | ||
| // un-prefetched reads). A slot holds a row index plus a 32-bit hash used only as an equality | ||
| // pre-filter, so the table never stores or compares a key itself. | ||
| // | ||
| // Keyless is why the hash and the equality test arrive as callables rather than as members: the whole | ||
| // point is that the caller owns the row representation. `eq(row_index)` confirms a pre-filter hit | ||
| // against the caller's rows, and no operation here reads a row. | ||
| // | ||
| // The layout this produces is load-bearing, not an implementation detail: it fixes the iteration order | ||
| // of for_each_slot(), which sets the order of a propagator's user-visible evolved-term list and | ||
| // therefore its floating-point accumulation order. A store that keyed rows through its own copy of this | ||
| // logic could diverge on that while still looking correct, which is the reason the index lives apart | ||
| // from the row representation rather than inside one. |
There was a problem hiding this comment.
| // The keyless open-addressing index a row store puts over its rows: power-of-2 slot count, linear | |
| // probing, max load factor 0.7 (the group-prefetch win erodes at higher load -- longer probe chains add | |
| // un-prefetched reads). A slot holds a row index plus a 32-bit hash used only as an equality | |
| // pre-filter, so the table never stores or compares a key itself. | |
| // | |
| // Keyless is why the hash and the equality test arrive as callables rather than as members: the whole | |
| // point is that the caller owns the row representation. `eq(row_index)` confirms a pre-filter hit | |
| // against the caller's rows, and no operation here reads a row. | |
| // | |
| // The layout this produces is load-bearing, not an implementation detail: it fixes the iteration order | |
| // of for_each_slot(), which sets the order of a propagator's user-visible evolved-term list and | |
| // therefore its floating-point accumulation order. A store that keyed rows through its own copy of this | |
| // logic could diverge on that while still looking correct, which is the reason the index lives apart | |
| // from the row representation rather than inside one. |
| // Refused before anything grows: an append that would pass the ceiling cannot be unwound, and a | ||
| // store's size must never reach a count whose last index is unrepresentable. Written as a | ||
| // subtraction because base + n is the sum that would wrap. |
There was a problem hiding this comment.
| // Refused before anything grows: an append that would pass the ceiling cannot be unwound, and a | |
| // store's size must never reach a count whose last index is unrepresentable. Written as a | |
| // subtraction because base + n is the sum that would wrap. |
| // Insert with no duplicate probe -- callers on this path insert provably distinct keys | ||
| // (+G-injective miss batches, clone re-insertion). | ||
| // insert_distinct over consecutive row indices [base, base + n), hashing each through hash_at(k). | ||
| // The stores' bulk_insert is this and nothing else, so it lives here rather than once per backend. |
There was a problem hiding this comment.
That's confusing, remove or change
| // Insert with no duplicate probe -- callers on this path insert provably distinct keys | |
| // (+G-injective miss batches, clone re-insertion). | |
| // insert_distinct over consecutive row indices [base, base + n), hashing each through hash_at(k). | |
| // The stores' bulk_insert is this and nothing else, so it lives here rather than once per backend. |
| // Group-prefetch batch find: out[i] = row index of keys[i], or kNotFound. Same result as n find() | ||
| // calls, but overlaps dram misses via a per-group hash/probe/confirm pipeline. An h collision falls | ||
| // back to an exact find. Must not run concurrently with inserts. | ||
| // | ||
| // The three callables are what make the pipeline possible without the table knowing a row: | ||
| // hash(key) -> uint32_t, prefetch_row(row_index) issued between probe and confirm (which is the | ||
| // whole reason confirmation is deferred rather than folded into the probe), and | ||
| // eq(row_index, key) -> bool. |
There was a problem hiding this comment.
| // Group-prefetch batch find: out[i] = row index of keys[i], or kNotFound. Same result as n find() | |
| // calls, but overlaps dram misses via a per-group hash/probe/confirm pipeline. An h collision falls | |
| // back to an exact find. Must not run concurrently with inserts. | |
| // | |
| // The three callables are what make the pipeline possible without the table knowing a row: | |
| // hash(key) -> uint32_t, prefetch_row(row_index) issued between probe and confirm (which is the | |
| // whole reason confirmation is deferred rather than folded into the probe), and | |
| // eq(row_index, key) -> bool. |
| // find_batch over keys reached through key_at(i) rather than an array: a query form that is not an | ||
| // array of keys (a flattened position list, say) keeps this one pipeline instead of copying it. | ||
| // key_at is called once per key in the hash pass and again in the confirm pass, so it must be cheap | ||
| // and stable. When hash_out is non-empty it receives every key's folded hash, which is what an | ||
| // insert of the misses hands back to insert_distinct_range. |
There was a problem hiding this comment.
Remove or simplify
| // find_batch over keys reached through key_at(i) rather than an array: a query form that is not an | |
| // array of keys (a flattened position list, say) keeps this one pipeline instead of copying it. | |
| // key_at is called once per key in the hash pass and again in the confirm pass, so it must be cheap | |
| // and stable. When hash_out is non-empty it receives every key's folded hash, which is what an | |
| // insert of the misses hands back to insert_distinct_range. |
| // First slot on h's probe chain whose stored hash matches, or kEmptySlot if the chain ends first. | ||
| // Matches on h alone and leaves confirmation to the caller -- that deferral is what lets find_batch | ||
| // prefetch the row between probe and confirm, so do not fold eq in here (find() deliberately keeps | ||
| // its own confirming variant). `start` must already be masked; the table must not be mutated | ||
| // concurrently. |
There was a problem hiding this comment.
| // First slot on h's probe chain whose stored hash matches, or kEmptySlot if the chain ends first. | |
| // Matches on h alone and leaves confirmation to the caller -- that deferral is what lets find_batch | |
| // prefetch the row between probe and confirm, so do not fold eq in here (find() deliberately keeps | |
| // its own confirming variant). `start` must already be masked; the table must not be mutated | |
| // concurrently. |
Added contribution guidelines including CLA information and contact details for Corporate CLA. ## Checklist - [ ] Tests added or updated to cover the changes - [X] Documentation updated (docstrings, `docs/`, `CONTRIBUTING.md`) if needed - [ ] `CHANGELOG` / release notes updated if applicable ## AI/LLM disclosure - [ ] I did not use LLM tooling, or used it only privately for ideation - [ ] I used the following tool to help write this PR description: - [ ] I used the following tool to generate or modify code: <!-- Any code generated or substantially modified by an LLM must be noted inline too. --> > [!IMPORTANT] > By opening this PR I confirm that I have read [CONTRIBUTING.md](https://github.com/Algorithmiq/monoprop/blob/fa820c3e5a90f773417c367fedbf2bd50683b496/CONTRIBUTING.md) and I agree to the terms of the [Contributor License Agreement](https://github.com/Algorithmiq/monoprop/blob/fa820c3e5a90f773417c367fedbf2bd50683b496/CLA.md). > [!WARNING] > If you're contributing on behalf of your employer, contact [cla@algorithmiq.fi](mailto:cla@algorithmiq.fi) to arrange a Corporate CLA. Signed-off-by: Roberto Di Remigio Eikås <robertodr@users.noreply.github.com>
OperatorIndex was two things: a packed position-list row representation and a keyless open-addressing index over those rows. RowHashTable is the second half, lifted out whole -- power-of-2 slots, linear probing, load factor 0.7, a 32-bit folded hash per slot used only as an equality pre-filter. Keyless is what makes the split possible: the table never stores or compares a key, so the hash and the equality test arrive as callables and the row representation stays entirely on the caller's side. find_batch keeps its pipeline by taking the row prefetch as a third callable -- deferring confirmation past the probe is the whole reason that prefetch has somewhere to go. The table's slot layout fixes for_each_slot's iteration order, which is the order of a propagator's evolved-term list and therefore its floating-point accumulation order, so this had to come out inert. Checked byte-wise against the previous implementation over 4000 random monomials: identical iteration order, find and find_batch results, clone order and memory_bytes. Assisted-by: ClaudeCode:claude-opus-5
d99049a to
3c0d67e
Compare
|



🤖 AI text below 🤖
Stacked on #302 — review that first; this PR's diff against it is
RowHashTable.hplus theOperatorIndexrewrite.Summary
Second of five PRs carved out of #226.
OperatorIndexcarries two things that have nothing to do with each other: a keyless open-addressing hash table, and the packed row storage the table indexes into. A second row backend is coming, and it needs the table and not the rows, so the table moves out first, on its own, with no behaviour change.RowHashTableis the table alone — power-of-two slots, linear probing, load factor 0.7, a 32-bit folded hash kept beside each slot as an equality pre-filter. It stores no keys: hashing, equality and prefetch arrive as callables from the owner, which is what lets a caller key its rows in whatever form it already has them in.OperatorIndexkeeps its own API and forwards to it.This is deliberately inert. The table's slot layout fixes
for_each_slotorder, which fixes evolved-term order, which fixes floating-point accumulation order — so a reordering here would move energies. Verified against main's implementation over 4000 random monomials: identical iteration order,find/find_batchresults, clone order andmemory_bytes.Changes
cpp/monoprop/detail/operator/RowHashTable.h: new. The table,TermIndexCeilingReached,kIndexCeiling,fold,find/find_batch/emplace/insert_distinct/insert_distinct_range,for_each_slot,reserve,slot_bytes.cpp/monoprop/detail/operator/OperatorIndex.h: keeps the rows, delegates the table.-195lines.cpp/monoprop/detail/operator/CMakeLists.txt: list the new header.Checklist
docs/,CONTRIBUTING.md) if neededCHANGELOG/ release notes updated if applicableCovered by the existing
operator_index_tests.cpp; the gate for a change of this kind isjust diff-baseline(#302), which must come out byte-identical.AI/LLM disclosure