Bit-pack Milnor basis elements into a u64 - #280
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe PR replaces vector-based Milnor p-parts with packed ChangesMilnor algebra representation and ranking
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ext/crates/algebra/src/algebra/milnor_algebra.rs`:
- Around line 2348-2377: Strengthen basis_is_derived_at_p2 by asserting the
expected canonical ordering independently of ppart_table(t), using the element
names or excess ordering produced by the previously stored basis path. Keep the
existing field-consistency checks, but construct expected values from fixed
ordering data so reordering ppart_table entries causes the test to fail.
- Around line 799-821: In ext/crates/algebra/src/algebra/milnor_algebra.rs lines
799-821, update the P^ parsing closure to use checked exponentiation for entry,
reject it against PPart::max_entry(t - 1) before calculating degree, then use
checked multiplication for the degree and reject overflow or values above
PPart::MAX_DEGREE. In ext/crates/algebra/src/algebra/milnor_algebra.rs lines
1199-1212, move the x > PPart::max_entry(0) validation in try_beps_pn before
computing degree.
- Around line 1159-1172: Update generate_basis_2 so the derived basis preserves
the historical unsorted ppart_table order used by stable p = 2 index mappings.
Remove the excess-based sorting from this method while retaining table
construction and extension behavior, ensuring basis_element_from_index remains
compatible with existing saved resolutions and magic().
- Around line 182-195: Prevent oversized profiles from producing invalid packed
signatures: update MilnorSubalgebra::new, from_bytes, and SubalgebraIterator to
enforce PPart::MAX_LEN, and ensure packed_signature rejects or safely handles
indices at or beyond that bound before calling PPart::width or PPart::shift.
Preserve valid signature packing for indices below PPart::MAX_LEN.
In `@ext/src/nassau.rs`:
- Around line 103-119: Update packed_signature to detect when any signature
entry has bits outside its field mask and return no-match for the entire
signature instead of truncating or leaking those bits; ensure signature_mask
propagates this result as an empty iterator. Add a regression test covering an
oversized entry in a narrow field and verify it matches no basis elements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6d4ddeba-d3bf-4edf-a9f4-36b959dba386
📒 Files selected for processing (14)
ext/crates/algebra/Cargo.tomlext/crates/algebra/benches/milnor.rsext/crates/algebra/benches/milnor_rank.rsext/crates/algebra/src/algebra/milnor_algebra.rsext/crates/algebra/src/algebra/milnor_rank.rsext/crates/algebra/src/algebra/mod.rsext/crates/algebra/src/algebra/pair_algebra.rsext/crates/algebra/src/module/rpn.rsext/crates/algebra/src/steenrod_evaluator.rsext/crates/algebra/src/steenrod_parser.rsext/examples/bruner.rsext/examples/sq0.rsext/src/nassau.rsext/src/yoneda.rs
The p-part of a Milnor basis element was a `Vec<u32>`, costing a heap
allocation and a pointer chase per element. At p = 2 the internal degree of
P(R) is sum_i r_i (2^i - 1) with non-negative terms, so r_i <= deg/(2^i - 1);
sizing each field by that bound packs the whole exponent sequence into 64 bits
for every degree up to 2045. At odd primes the same bound applies divided by
q = 2(p-1), so one layout serves every prime.
`MilnorBasisElement` is now 16 bytes, `Copy`, and entirely inline. Measured
over degrees 0..=300 at p = 2, `basis_table` drops from 252 MiB in 5,036,688
allocations to 77 MiB in none.
Three things fall out of the packing:
- The packed value is a canonical key, so the hand-rolled `MilnorHashMap`
specialization for `not(odd-primes)` is gone; a plain `HashMap` now hashes a
single word on every path. That code also assumed a degree bound of 1536
without enforcing it. `compute_basis` now asserts the bound up front, which
is what lets everything downstream skip range checks.
- Trailing zeros are not represented, so the "pop trailing zeros" loops after
building a product disappear.
- `PPartMultiplier` no longer borrows its inputs, so its lifetime parameter is
gone, and `PPartAllocation` loses the buffer it existed to recycle.
In `ext`, `MilnorSubalgebra`'s signature test becomes one masked comparison on
the packed word instead of a loop over entries, with the mask hoisted out of
`signature_mask`'s inner loop.
Two behaviour changes worth noting:
- `basis_element_from_string("P0")` and `("Sq0")` now return the identity
rather than `None`. P(0) is the identity, and `AdemAlgebra::try_beps_pn`
already special-cases `x == 0` this way; the old `None` came from `vec![0]`
and `vec![]` hashing differently, an artifact of the representation.
- `increment_p_part` now carries before incrementing. The old order
transiently stored `max[i] + 1`, which need not fit a field whose width is
exactly saturated by `max[i]`. The enumeration is unchanged.
The observation that every Milnor exponent sequence up to degree 512 fits in 64
bits is due to Lixiong Wu; this implementation works out the widths, finds that
the same layout holds all the way to degree 2045, and carries it through the
algebra.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XK8YCdHkCqUV9YM7D957hV
The first packing pass regressed `milnor_ppart` by up to 8% at odd primes and mod 4, because assembling the answer went from a memcpy plus a vectorized add to a per-entry read-modify-write through the checked `PPart::set`, and because `PPart::get`'s range branch landed in `update`'s inner loop. Two changes, both confined to the kernel: - Assemble the answer in a plain `u64` and store it once. Entries are written in increasing index order into a value that starts at zero, so a shift and an `or` suffice; the range checks become debug assertions backed by `compute_basis`'s degree gate. - Pad the layout tables to 16 entries so the private `PPart::entry` can mask its index rather than branch on it. Padded entries have width zero and so read as zero, which is the answer `get` would have returned anyway. The public `get` keeps its explicit check, since callers outside the multiplier index it with a q-part-derived length that is not bounded by `MAX_LEN`. This recovers the regression (`ppart_4/a` and `ppart_3/a` back to baseline, `ppart_4/b` -8%) and improves the Nassau regime further. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XK8YCdHkCqUV9YM7D957hV
`basis_table` held a `MilnorBasisElement` per basis element. At p = 2 with unstable support off, that element is exactly `from_p(ppart_table[t][i], t)` -- the p-part again, with a q-part that is always zero and a degree that is the index. It was a redundant copy. Deriving it on demand costs nothing now that `MilnorBasisElement` is `Copy` and 16 bytes: `basis_element_from_index` returns by value and builds it in registers rather than handing out a reference into a table. The multiply family takes the element by value for the same reason. The table is still built at odd primes, where the q-part varies within a degree, and when unstable support is on, where the basis is re-sorted by excess. Neither is a re-wrapping of `ppart_table`. Measured over degrees 0..=250 at p = 2 (1,958,958 elements), RSS growth from `compute_basis` drops 125.0 MB -> 95.0 MB, i.e. 66.9 -> 50.8 bytes per element. Projected to degree 500 that is 5.24 GB -> 3.95 GB. Unlike the ranker, this needs no basis renumbering and costs nothing at lookup time. A test verifies the derivation matches what the table used to hold, for every element, so the redundancy is asserted rather than assumed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XK8YCdHkCqUV9YM7D957hV
`basis_element_to_index` runs once per term of every product, and is a hash map
storing an entry per basis element. The index it returns is a position in an
enumeration, so a canonical key alone cannot replace the map -- but the position
can be computed.
Let counts[i][d] be the number of exponent sequences of degree d using only
xi_1..xi_i. Splitting on whether r_i is zero gives the coin-change recurrence
counts[i][d] = counts[i-1][d] + counts[i][d - xi_i]. Ranking needs the number of
sequences with r_i > v, and substituting r_i -> r_i - (v+1) is a bijection onto
all sequences of degree d - (v+1)*xi_i, so that count is a single table lookup
rather than a sum. Walking the entries downward ranks a p-part in one lookup
each, against a table covering every degree at once, where the map it would
replace grows with the basis.
Whether that is worth it depends entirely on scale, which took some measuring to
see. Against the map, at p = 2:
degree per-degree map hashmap ranker ratio
120 0.10 MB 11.6us 26.7us 0.43x
300 3.12 MB 792us 1384us 0.57x
400 12.50 MB 4490us 5310us 0.85x
500 37.50 MB 33118us 15865us 2.09x
A lookup probes only its own degree's map. While that fits in cache the map wins
easily: one hash round and one probe, against six to ten dependent table reads.
Once it does not -- the map is 37 MB in degree 500 -- every probe misses to DRAM
at ~33 ns, whereas the ranker's table is ~43 KB, stays in L1, and costs ~16 ns
regardless of degree. Benchmarking only up to degree 120, where the map is
0.1 MB, shows a 2x loss and hides all of this; the sweep here deliberately spans
the crossover. Tuning does not move the small-degree end: nested vs flat table,
a zero-padded prefix to drop the branch, and one- vs two-pass to break the
dependency chain were all measured, and the padded variant was worst, because
doubling the table pushed it out of L1.
So the two suit opposite ends of the range, and the ranker is on the right side
of the end where the algebra's memory is the problem worth solving: replacing the
map there is 3.3 GB smaller and 2x faster.
It stays off by default and unwired even when enabled, because it numbers the
basis in colex order rather than the order compute_ppart emits, which would
invalidate saved resolutions. That order is rankable in principle, but its
natural recursion has depth equal to the sum of the entries, which is worse than
hashing. The unstable path, which re-sorts each degree by excess, is not modelled
either.
Tests verify the table reproduces the algebra's own p-part counts and that the
rank is a bijection onto 0..dim in every degree, at p = 2 and p = 3, plus one
pinning down that it really does disagree with the current basis order.
Also measured and rejected: an `unrank` recovering the p-part at a given index,
which would let basis_element_from_index drop ppart_table entirely. It ran ~15x
slower than the array read it would replace, at every degree, with none of the
crossover above -- ppart_table is 8 bytes per element against ~43 for the map, so
it stays cache-resident. The bit-packing that makes rank worth having is the same
thing that makes unrank not. The likelier route, if it is ever revisited, is
enumerating the basis in index order, which is O(1) amortised and matches how
callers actually walk it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XK8YCdHkCqUV9YM7D957hV
Three paths computed with unvalidated input before checking it against the
packing bounds, so the intermediate arithmetic went wrong first. All three are
reachable from public, non-panicking entry points.
- `basis_element_from_string("P^s_t")` indexed the xi-degree table with `t`,
which has exactly `MAX_LEN` entries, so `t = MAX_LEN` was out of bounds. `p^s`
and the degree product could also overflow. Now `t` is bounded by the table
itself and both are computed with checked arithmetic.
- `try_beps_pn` computed `q * x + e` before bounding `x`, which overflows for a
large `x`. The bound moves above the computation.
- `MilnorSubalgebra::packed_signature` assumed the profile was no longer than
`PPart::MAX_LEN` and that each signature entry fit its field. Neither holds:
`SubalgebraIterator` grows a profile without limit and `from_bytes` reads
whatever length a file gives. Out of range, `PPart::shift` returns 64 and the
shift overflowed; an oversized entry silently spilled into the neighbouring
field, which could select unrelated basis elements. It now returns `None` for
a signature no element can have, and `signature_mask` yields nothing.
`basis_element_from_string` is documented as total and `try_beps_pn` is the
non-panicking half of `beps_pn`, so these were contract violations rather than
merely untidy. Tests cover each.
The signature test checks the packed mask against the per-entry comparison it
replaced, over every element up to degree 60, for profiles that are narrower
than their fields, wider than their fields, and longer than a p-part can be.
Also adds `basis_order_at_p2_is_stable`, which pins the first nine degrees to
fixed element names. The basis order is a wire format -- saved resolutions store
coefficients by index -- so it needs a guard that does not read from
`ppart_table`, which is the thing being guarded. Verified separately that the
order is unchanged from the base commit: identical for all 4156 elements in
degrees 0..=60.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XK8YCdHkCqUV9YM7D957hV
4d06e09 to
d09141e
Compare
|
Thanks — four of the five were real, and three were genuine panics reachable from entry points documented as total. Fixed in Fixed — exponent validation ordering. Both sites panicked, not merely truncated. Confirmed with tests before fixing:
Fixed — signature masks past Fixed at the packing boundary rather than by bounding profile length in three constructors, since that's where the assumption lives and it keeps The regression test checks the packed mask against the per-entry comparison it replaced, over every element up to degree 60, for profiles narrower than their fields, wider than their fields, and longer than a p-part can be. Fixed — test strengthening. Fair point, and it applies to the order rather than the derivation. Added Declining — "preserve the stable Verified rather than argued: dumping every basis element in index order at both the base commit and this branch gives identical output for all 4156 elements in degrees I'd also flag that the suggested fix — removing the excess sort from Separately, Generated by Claude Code |
CI lints with the nightly toolchain, where the unstable options in `rustfmt.toml` -- `reorder_impl_items` among them -- actually take effect. Stable rustfmt skips them with a warning, so `cargo fmt --check` passed locally and failed in CI. Formatting only: the constants are sorted and the blank lines between them dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XK8YCdHkCqUV9YM7D957hV
2 participants
The p-part of a Milnor basis element was a
Vec<u32>, costing a heap allocation and a pointer chase per element. Atp = 2the internal degree ofP(R)issum_i r_i (2^i - 1)with non-negative terms, sor_i <= deg / (2^i - 1); sizing each field by that bound packs the whole exponent sequence into 64 bits for every degree up to 2045. At odd primes the same bound applies divided byq = 2(p-1), so one layout serves every prime.MilnorBasisElementis now 16 bytes,Copy, and entirely inline.The observation that every exponent sequence up to degree 512 fits in 64 bits is due to Lixiong Wu. This PR works out the widths, finds the same layout holds all the way to degree 2045, and carries it through the algebra.
Results
Memory, measured as RSS growth from
compute_basis,p = 2:0..=250Two things get it there. Packing removes the per-element allocation and the
Vecheader. Separately,basis_tableturns out to be redundant atp = 2:basis_table[t][i]is exactlyfrom_p(ppart_table[t][i], t), so it is dropped and derived on demand, which is free now that the type fits in registers. The table is still built at odd primes, where the q-part varies within a degree, and when unstable support is on, where the basis is re-sorted by excess.The basis index order is unchanged — verified element-by-element against the base commit, identical for all 4156 elements in degrees
0..=60— so saved resolutions stay valid. A test pins the first nine degrees to fixed element names to keep it that way.Speed, on
nassau_milnor(the bench documented as capturing Nassau's regime), mostly improved — the largest movements wereop8xel8-20%,op24xel16-11%,op20xel24-10%. Note that this machine's run-to-run noise onmilnor_ppartreached 6%, so treat small movements there as unresolved.Fallout worth reviewing
Three things follow from the packing rather than being incidental:
compute_basisassertsmax_degree <= 2045up front, which is what lets everything downstream assume entries fit. The previous hand-rolled packing inMilnorHashMap::codeassumed 1536 without checking.MilnorHashMapspecialization is gone. The packed value is a canonical key, so thenot(odd-primes)fork is unnecessary; a plainHashMapnow hashes a single word on every path.PPartMultiplierno longer borrows its inputs, so its lifetime parameter is gone, andPPartAllocationloses the buffer it existed to recycle. The multiply family takesMilnorBasisElementby value.In
ext,MilnorSubalgebra's signature test becomes one masked comparison on the packed word instead of a loop over entries, with the mask hoisted out ofsignature_mask's inner loop.Two behaviour changes
basis_element_from_string("P0")and("Sq0")now return the identity rather thanNone.P(0)is the identity, andAdemAlgebra::try_beps_pnalready special-casesx == 0this way; the oldNonecame fromvec[0]andvec[]hashing differently, an artifact of the representation. The test is updated.increment_p_partcarries before incrementing. The old order transiently storedmax[i] + 1, which need not fit a field whose width is exactly saturated bymax[i]. The enumeration is unchanged.Input validation
The last commit fixes three paths that computed with unvalidated input before checking it against the packing bounds, so the intermediate arithmetic went wrong first — an out-of-bounds index into the xi-degree table, two integer overflows, and a shift by 64. All are reachable from entry points documented as total (
basis_element_from_string) or non-panicking (try_beps_pn), pluspacked_signature, which had assumed a bound on profile length that nothing enforces. Thanks to CodeRabbit for catching these.PPartRanker (opt-in, not wired in)
One commit adds an arithmetic alternative to the basis index map, behind the off-by-default
milnor-rankfeature. It is not hooked intobasis_element_to_indexeven when enabled — it is there so the design and its measurements survive.It is worth having eventually because the two strategies scale in opposite directions: a lookup probes only its own degree's map, and once that leaves cache (37 MB in degree 500) every probe misses to DRAM, whereas the ranker's table is ~43 KB for all degrees. Measured at
p = 2: 0.43x at degree 120, 0.85x at 400, 2.09x at 500. Adopting it would renumber the basis in colex order and so invalidate saved resolutions, which is a separate decision — hence inert for now. The commit message records the full measurement, including anunrankthat was tried and rejected.Reviewers can ignore this commit entirely without affecting the rest; nothing in the default build compiles it.
Testing
just test,just lintandjust docsall reproduced locally and pass, including the--no-default-featuresand--all-featuresconfigurations and rustdoc under-D warnings. 64 tests by default, 69 with--features milnor-rank.New tests cover the layout invariant (widths against the xi-degrees, so changing
MAX_DEGREEwithoutWIDTHSfails loudly), packing faithfulness over every basis element up to degree 120 atp = 2and 200 atp = 3, the odometer's saturated-field case, thebasis_tablederivation and index order, the rejected overflow inputs, and the packed signature mask against the per-entry comparison it replaced.One pre-existing failure is unrelated:
save_load_resolution::test_tempdir_lockexpects a permission error and does not get one when the suite runs as root. It fails identically onmaster.🤖 Generated with Claude Code
https://claude.ai/code/session_01XK8YCdHkCqUV9YM7D957hV