Release 3.3.0: scratch geometry ceilings, archive peek, and one float32 kernel-selection rule - #2
Merged
Merged
Conversation
ScratchBank::Create (both overloads) and Grow computed the arena size from caller-supplied capacity and dims before bounding either. ArenaBytes multiplies those in signed int64, so a large valid-typed request overflowed the computation rather than failing to allocate. Both now apply the format's own ceilings (kMaxBankRows rows, kMaxCrossDeviceDims dims) before any size arithmetic runs, returning InvalidArgument. The archive loader already applied these caps; direct construction did not, and Grow bounded its request only against the current capacity. PeekScratchArchive reports a serialized archive's geometry, channel table, and the exact byte length a Load consumes, reading only the header from a byte span. Hosts that append a trailer after the archive can now validate that trailer before committing the load instead of discovering a broken one with the rows already replaced. Load and the peek share one header validator so the two cannot disagree about which archives are well-formed. Suite: 85,075 checks, 0 failures. The geometry cells were confirmed to fail against the unbounded code (13 failures) before the caps landed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ValidateBank required the per-channel inverse sub-norm array whenever a Cosine bank carried a channel table. Those norms are one per row, so a bank with no rows requires none, and no scan reads the array at zero rows. The requirement now applies only when count > 0. The shape is not degenerate. It is what a channel-carrying scratch bank graduates into once every row has been removed, and rejecting it forced that graduation to drop its channel table rather than fail. Also pins version.h at 3.2.2 in the coherence cell. Suite: 85,081 checks, 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ci/__pycache__/_cxx_decls.cpython-313.pyc was committed alongside the coherence checks. It is interpreter- and machine-specific, regenerates on every ci run, and nothing in .gitignore prevented the next one. Untracked, and the class is now ignored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The float32 dispatchers DotF32/L2F32 select AVX2+FMA only when the length they are handed is a multiple of 8. ResolveRowKernels, whose table feeds the segmented scan and DecomposeRowScore, wired the AVX2 kernels in directly with no such rule. Float32 strides are multiples of 4, so at paddedDims 20, 36, 52, 100 and 132 the whole-row scan computed a 4-lane mul-add where the segmented scan computed an 8-lane FMA, and the two disagreed in the last ulp on the same row. That falsified two published guarantees: that a degenerate one-segment query equals the whole-row scan, and that decomposition contributions sum bit-exactly to the scan's own score because no second code path exists to drift. ResolveRowKernels now holds the dispatchers themselves, so the rule is applied once. It has to key on the length rather than on bank shape, because the segmented scan calls the kernel once per scan range and a bank whose stride is a multiple of 8 can still present a range that is not. int8 is untouched: channel and segment ranges sit on the 16-byte element grid, so int8 lengths are always multiples of 16 and that dispatcher carries no stride condition. Exactness::CrossDevice was therefore never affected and its cross-machine contract is unchanged. No pinned golden moves - every one sits at a width or quantization where the two paths already agreed. TestSegmentedEqualsWholeRowAcrossWidths sweeps the widths that can break it and compares the two PATHS to each other, which is the check the suite lacked: T11 compares each kernel against its own scalar mirror, and that stays green while the paths pick different kernels, because each is individually correct. The existing degenerate-segment assertion in TestSegmentedScan covers the mechanism but runs only at dims 32, where the stride is a multiple of 8 and the two paths cannot disagree. Confirmed to fail against the unfixed selector at exactly paddedDims 20, 36 and 52 on Dot and L2, with 32 and 64 clean. Corrects a comment in the T11b block asserting that f32 paddedDims is always a multiple of 8, which is what let this go unnoticed. Suite: 85,441 checks, 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…self-dot hoist Addresses the core half of a whole-project review's lower-severity findings. graph.cpp (F5): MutualFilter and ConnectedComponents bound caller-supplied neighbour and duplicate-group values to [0, count) before using them as indices into the neighbour list and the union-find scratch. -1 stays the "no neighbour" sentinel; any other out-of-range value degrades to a dropped edge rather than an out-of-bounds read or write. Reachable only through a hand-built neighbour list, but that is the trust posture the other core entry points already hold. analytics.cpp (F8): the int8 off-grid channel guard that novelty.cpp applies before detail::DotI8I8 was missing on the channel legs here. Added once in ChannelSubRange, the single point all three legs resolve the range through, so an off-grid hand-built BankView is refused rather than reading SIMD-padded slop. pca.cpp (F11, F12): the covariance-apply accumulator is now double, matching the mean's precision (a float accumulator rounded once per row and degraded the power iteration with bank size); the scratch buffer is double accordingly. ProjectRowsOntoComponents now bounds componentCount by dims like its sibling, so an over-large count is refused rather than reading past the components buffer. Scale decode (F9): compose.cpp, pca.cpp, and analytics.cpp's projection report now decode bank.scales[] through detail::FloatBitsToDouble, matching every other scale read, so a subnormal scale decodes identically regardless of the thread's FTZ/DAZ mode. analytics.cpp (F15): the channel NN divergence pre-lifts the target sub-rows once into the Workspace's XdQuery scratch, so each target's self-dot is computed once rather than once per source row. novelty.cpp (F14): a comment at CalibrateNoveltyBaseline documents that sampleLimit is a ceiling, not a down-sampler, matching the header. Suite: 85,470 checks, 0 failures. The two memory-safety guards gained tests: F8's is a clean status oracle (confirmed to fail on all four channel legs with the guard removed); F5's asserts the guarded result on poisoned input and defers the unguarded-access crash to the ASan CI job, since OOB in a normal build is UB that does not reliably fault. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Documents the graph/analytics/pca/decode fixes committed in 958c50a under the 3.2.2 entry (they were made after that commit's own message was written). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ms to reference A retained row is by construction the post-normalization row the quantizer consumed, so Load replays the bake on each retained row and requires it to reproduce the stored row exactly (allocation-free, mirroring QuantizeRowsInt8/PadRowsFloat32). A fabricated-but-finite retained array previously loaded clean and handed MeasureScratchRecall an invented reference to audit the quantized rows against. Non-finite retained values are rejected the same way. Suite: 85,532 checks, 0 failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PeekScratchArchive and ScratchArchiveInfo are a new public capability, so the release advances the minor version per this project's stated policy (minor = new capability, patch = fix).
…rce break Rename the [3.2.2] heading to [3.3.0]: PeekScratchArchive/ScratchArchiveInfo are a new public capability, and this project's own stated policy is minor = new capability, patch = fix. Move ComputePrincipalComponents' scratch parameter type change (float* to double*) out of the "lower-severity" Fixed bullet and into a new Changed section, stated plainly as source-breaking: every caller must retype the parameter and double the buffer it allocates. Add a Changed note that MeanNNCrossDeviceChannel/MaxNNCrossDeviceChannel can now return OutOfMemory, a status neither could previously return, since both now stage a Workspace pre-lift of the target's rows. Add the two tagged releases the changelog was missing entries for (v3.0.1, v3.1.1), derived from git history, restoring the "versions follow the git tags" claim the file opens with. v3.1.1's entry states plainly that the AVX2 sub-8 segment-remainder fix changed numeric results for affected widths, so a consumer bisecting a score change across 3.1.0-3.1.2 has something to read.
…bled word
MeanNNCrossDeviceChannel/MaxNNCrossDeviceChannel stage a pre-lift of the
target's non-excluded sub-rows into the Workspace argument (each target's
self-dot computed once rather than once per source row); if that
reservation fails they return OutOfMemory, which neither function could
return before. Document it alongside the existing InvalidArgument cases.
Also fixes a doubled word in the channel-scoped analytics doc comment
("scratch scratch-buffer" -> "scratch-buffer").
…fMemory Four inline version markers (ScratchArchiveInfo, PeekScratchArchive, the geometry-ceilings note, the archive-peek note) still read v3.2.2 after the release was retitled 3.3.0; retargeted to match. Note that MeanNNCrossDeviceChannel/MaxNNCrossDeviceChannel can now also return OutOfMemory (v3.3), matching the header doc comment.
The 3.1.1 entry said ResolveRowKernels "wires the AVX2 dispatchers in directly" at that release; it wired the AVX2 kernels in directly (no dispatcher existed there yet). The 3.3.0 entry documents ResolveRowKernels gaining the dispatchers as its own fix, so the two entries described the same call site inconsistently. Corrected 3.1.1 to say "kernels". Also removes the false claim that paddedDims is always a multiple of 8 for banks large enough to matter: PaddedDims (types.h) grids Float32 at 4 elements (kAlignment 16 / ElementSize 4), and the 3.3.0 entry's own kernel- selection bug is scoped to exactly the non-multiple-of-8 float32 widths (20, 36, 52, 100, 132, ...) this claim said were unreachable. The remaining clause -- the dispatchers route non-multiple-of-8 paddedDims to the SSE path -- is correct and sufficient on its own.
TestChannelQueryOnlyStorm, TestChannelScopedReductionStorm, and TestRelabelExclusiveDrainStorm each spawned a writer thread and then ran a fixed-iteration reader loop, asserting afterward that the reader had observed the writer publish past its initial row count. Nothing guaranteed the writer was scheduled before the reader's iterations completed, so under concurrent machine load the reader could finish first and the assertion tripped on scheduling rather than on a real defect. Add WaitForPublishPastCount(), a bounded poll on ScratchBank::Count() (already an atomic acquire load, safe to call unpinned), and call it before each cell's measured loop so the loop cannot start until the writer has actually published a row. The wait is bounded so a genuine regression in the append/publish path still fails the test loudly instead of hanging or passing vacuously. This mirrors the readersReady start-barrier already used by TestConcurrencyStorm and TestPinDrainLitmus, adapted to a single-writer/ single-reader shape where the reader waits on the writer's actual publish rather than on thread startup alone. In TestChannelQueryOnlyStorm, also collapse the per-hit CHECK(std::isfinite(...)) loop into one CHECK per outer iteration. The hit count returned by a query moves with how far the writer has appended by that iteration, which made the suite's total check count vary run to run (85470 vs 85532 observed) and defeated the "N checks" summary line as a regression signal. Every returned score is still examined; the number of CHECK calls is now fixed at one per iteration regardless of how many hits came back. Retarget the version-identity test and a stale section-marker comment from v3.2.2 to v3.3.0, matching the version.h bump landing in this branch. Verified with three consecutive full-suite runs: 82835 checks, 0 failures, identical count each time.
…g checks The comment above CHECK_MSG(maxCountSeen > appended, ...) in the three scratch concurrency storm cells stated the check was "guaranteed by the wait above," implying it could never fail and existed only as inert documentation. That is wrong: WaitForPublishPastCount polls bank.Count() directly, while maxCountSeen is accumulated from snap.count via bank.Snapshot() -- a different read path. The trailing check is the one that would catch a regression in Snapshot's own acquire ordering (Snapshot returning a stale count even though Count() has already advanced), which the gate does not observe at all. Replace the comment in all three cells with one that states what the check actually guards and makes explicit that it reads a different path from the gate, so it is not mistaken for redundant and removed. No assertion or gate logic changed. Verified with a full-suite run: 82835 checks, 0 failures, identical to the prior run.
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
2 participants
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.
What this release is
PeekScratchArchiveis a new public capability, so this ships as a minor (3.3.0), not a patch — matching the versioning policy stated at the top ofCHANGELOG.md.Added
PeekScratchArchive/ScratchArchiveInfo— read a serialized scratch archive's geometry and its exactarchiveBytesfrom a byte span, without allocating or reading the payload. Shares one header validator withLoad, so the two cannot drift. Lets a host validate its own appended trailer before committing a load.Changed (source-breaking)
ComputePrincipalComponents'scratchparameter is nowdouble*(wasfloat*). Callers must retype it and double the buffer. The covariance-apply accumulator now matches the mean's precision.MeanNNCrossDeviceChannel/MaxNNCrossDeviceChannelnow use theWorkspaceto stage a target pre-lift and can returnOutOfMemory, which they could not before.Fixed
ScratchBank::Loadvalidates the retention region by replaying the bake on each retained row, closing a path where a fabricated-but-finite retained array handedMeasureScratchRecallan invented reference.ResolveRowKernelsnow holds the dispatchers, so one rule applies to the length actually in hand.Exactness::CrossDevicewas never affected — int8 lengths are always multiples of 16 — so the cross-device contract is unchanged and no pinned golden moves.ScratchBank::Create/Growbound capacity and dims against the format's own ceilings before the arena size is computed, replacing signed-overflow UB with a refused allocation.doublecovariance accumulator, and DAZ-safe scale decode on the remaining paths.3.0.1and3.1.1changelog entries.3.1.1changed numeric results at sub-8 float32 widths and had no public record.Verification