lgj-abi: hop sweeps the store ONCE, not 32 times — 3.5× at scale - #39
Merged
Conversation
Acts on ISS-LGJ-HOP-SWEEPS-FULL-POPULATION, which bench Component G opened
hours earlier. The measurement authorised this; nothing here is taste.
THE ARITHMETIC IS WHY, and it is not the obvious "too many rows". 32 x 65_536
strided u32 compares is ~2M operations -- nowhere near the 24 ms observed. But
32 passes over a 33 MB store, each re-reading every 512-byte row to look at 4
bytes of it, is ~1 GB of memory traffic. THE LOOP ORDER WAS THE COST, NOT THE
ROW COUNT. That distinction is what makes the fix a re-ordering rather than a
new kernel.
WHAT CHANGED. `lgj_hop` no longer loops `for facet in 0..32 { sweep(all n
rows) }`. It calls `simd_rowstore_facet_match` ONCE -- all 32 facets per row,
one `U32x16::eq_bitmask` per 64-byte chunk via `MultiLaneColumn` -- and then
walks SRC'S SET ROWS rather than every row x facet, taking only the facets
that both matched and participate (`facet_bits[row] & effective`).
NO NEW KERNEL. `simd_rowstore_facet_match` already existed and is the same
sanctioned `ndarray::simd` surface (abi.md §8). It was simply being consumed
the wrong way round. No ABI change, no signature change, no minor bump: this
is strictly an internal re-ordering behind an unchanged contract.
MEASURED, same instrument, same command:
native_hop 1%/4096 1%/65536 25%/4096 25%/65536
before 479.0 24798.3 521.2 23633.9 us/op
after 374.7 7120.3 375.8 8076.9
speed-up 1.28x 3.48x 1.39x 2.93x
THE CONTROL MATTERS HERE. The two scalar arms are untouched code. Had the
second run merely landed on a faster host they would have moved too --
measured, `classidScan` 150.6 -> 164.2 and `facetMatches` 7142.3 -> 7698.8 at
1%/65536, within ~9% and slightly SLOWER. So the native gain is real and if
anything understated. (JMH's banner does report a different CPU string between
the runs, which is exactly why the control arms are quoted rather than the box
being assumed identical.)
BEHAVIOUR UNCHANGED, verified rather than assumed: lgj-abi 134/134 (the hop's
own aliasing and semantics tests among them), AllTests 304, GraphHopTest 66
INCLUDING G3 at the identical 384-byte allocation floor, TradesParity 12,
TradesAllocation 3, BricksAuth 62 = 447 Java checks. clippy -D warnings and
fmt clean.
STILL SLOWER THAN THE BEST SCALAR ARM, and the issue is regraded RESOLVED IN
PART rather than closed. At 1%/65536 native is 7120 us against classidScan's
164 us -- 43x. The catastrophic term is gone; a structural one is not:
`simd_rowstore_facet_match` still sweeps the WHOLE population, so the decode
half is now frontier-bounded and the compare half is not.
The next rung is a genuinely different shape -- gather per src row, O(frontier)
instead of O(population) -- and it is NOT obviously better, because a dense
frontier should favour the sweep's sequential access. That is a real density
crossover, so it is a measurement rather than a judgement call, and Component G
is now the instrument that can find it.
WHY NOT BOTH AT ONCE. The one-pass change is bounded, needs no new kernel, has
no crossover, and is strictly less work at every point in the measured space.
The gather rewrite is none of those. Landing them together would have made a
regression in either impossible to attribute.
Board artifacts in the same commit per the hygiene rule: ISSUES regraded with
what remains open and why the split, RESULTS.md § G carries the before/after
table, the control-arm reasoning and the named remaining gap.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 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 |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_5f5314c8-3626-4810-9ad4-f47f31ca2e21) |
AdaWorldAPI
marked this pull request as ready for review
August 27, 2026 13:23
AdaWorldAPI
pushed a commit
that referenced
this pull request
Aug 27, 2026
PR_ARC_INVENTORY had entries for #1-#12, #14, #16, #18, #20 and #32, and nothing else. Missing: #13, #22-#31, #33-#41 — twenty PRs. (#15/#17/#19/#21 are also absent and correctly so: each is itself an arc-entry-only PR, exempt under the termination clause.) #32's own entry still read "(draft, opened …)" with no merge sha. Corrects this branch's first count, which said nineteen and read the gap as starting at #21. Both were wrong — it is twenty, and it starts at #13. The first count was a range subtraction over a file with holes; the enumeration is what found the difference. Method, which is the point rather than an aside: each entry drafted from that PR's own body and diff, five parallel agents over four PRs each, none permitted to work from a later session's recall. Every backfilled entry's Confidence bullet ends "Backfilled 2026-08-27 from the PR body and diff, not written at merge time", so reconstructed entries are distinguishable from ones written at merge time; several state which claims are the PR body's own and were not re-verified. Every cited sha, date and merge-vs- squash label machine-checked against git. Four things the backfill turned up, each recorded in the entry it belongs to: #25's body asserts "no code, no reproducer changes" and its own diff contradicts it; #39 left its lgj_hop doc comment describing the pre-change design; #34's banked evidence file did not identify its own JDK; and #41 is on main while its own title reads [DO NOT MERGE AS-IS], recorded as unresolved disposition rather than an endorsement. ISS-LGJ-ARC-INVENTORY-STOPPED-AT-32 moves to RESOLVED with the corrected count and the standing rule restated: the entry goes in at open, in the PR's own commit. The backfill is the repair, not the process. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DCfrD5y19cvFc4AoyydXYv
AdaWorldAPI
pushed a commit
that referenced
this pull request
Aug 27, 2026
The "Kernel composition" paragraph still described the single-predicate shape: one classid match per facet, with the structured-edge gate absent because it was an `if` in the walk rather than an operand. R1 makes selection `src AND class_f AND struct_f`, so §13 now names both predicates, the one primitive that produces both (same call, offsets +0 and +12 into the facet), and the reason the scatter alone stays scalar — the destination is decoded from the selected row's payload, which makes it the operand of a permutation rather than a decision about membership. Caught by re-reading the diff adversarially rather than by a gate. Leaving prose behind a changed function is exactly what #39 did and #40 had to fix one PR later; no reason to make the same reviewer spend that cycle twice. No ABI change: no symbol, no signature, no minor bump. Gates re-run unchanged — 134/134, clippy -D warnings, fmt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DCfrD5y19cvFc4AoyydXYv
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.
Acts on
ISS-LGJ-HOP-SWEEPS-FULL-POPULATION, which bench Component G opened hours earlier (#38). The measurement authorises this; nothing here is taste.The arithmetic is why — and it isn't "too many rows"
32 × 65 536 strided
u32compares is ~2 M operations. That is nowhere near the 24 ms observed. But 32 passes over a 33 MB store, each re-reading every 512-byte row to look at 4 bytes of it, is ~1 GB of memory traffic.The loop order was the cost, not the row count. That distinction is what makes this a re-ordering rather than a new kernel.
What changed
lgj_hopno longer loopsfor facet in 0..32 { sweep(all n rows) }. It callssimd_rowstore_facet_matchonce — all 32 facets per row, oneU32x16::eq_bitmaskper 64-byte chunk viaMultiLaneColumn— then walks src's set rows rather than every row × facet, taking only facets that both matched and participate (facet_bits[row] & effective).No new kernel.
simd_rowstore_facet_matchalready existed and is the same sanctionedndarray::simdsurface (abi.md §8) — it was being consumed the wrong way round. No ABI change, no signature change, no minor bump: strictly an internal re-ordering behind an unchanged contract.Measured — same instrument, same command
native_hopThe control matters here. The two scalar arms are untouched code — had the second run merely landed on a faster host, they would have moved too. Measured:
classidScan150.6 → 164.2 µs andfacetMatches7 142.3 → 7 698.8 µs at 1 %/65 536 — within ~9 %, and slightly slower. So the native gain is real and if anything understated. (JMH's banner does report a different CPU string between runs, which is precisely why the control arms are quoted rather than the box assumed identical.)Behaviour unchanged — verified, not assumed
lgj-abi 134/134 (the hop's own aliasing and semantics tests among them), AllTests 304, GraphHopTest 66 including G3 at the identical 384-byte allocation floor, TradesParity 12, TradesAllocation 3, BricksAuth 62 — 447 Java checks.
clippy -D warningsandfmtclean.Still slower than the best scalar arm — regraded, not closed
At 1 %/65 536 native is 7 120 µs against
classidScan's 164 µs — 43×. The catastrophic term is gone; a structural one is not.simd_rowstore_facet_matchstill sweeps the whole population: the decode half is now frontier-bounded, the compare half is not. The issue is marked RESOLVED IN PART, not resolved.The next rung is a genuinely different shape — gather per src row, O(frontier) instead of O(population) — and it is not obviously better, because a dense frontier should favour the sweep's sequential access. That is a real density crossover, so it is a measurement rather than a judgement call, and Component G is now the instrument that can find it.
Why not both at once
The one-pass change is bounded, needs no new kernel, has no crossover, and is strictly less work at every point in the measured space. The gather rewrite is none of those. Landing them together would make a regression in either impossible to attribute.
Board hygiene
Same commit:
ISSUESregraded with what remains open and why the split;RESULTS.md§ G carries the before/after table, the control-arm reasoning, and the named remaining gap.Generated by Claude Code