Repository navigation
perf(offsets): stop walking every offset when only distance-based encoders are active - #106
Conversation
…oders are active OffsetSimultaneousEncoder.invoke() visited every offset between the committed base and the highest succeeded offset. When the range spans a large gap - up to ~2.1 billion - that is a multi-second scan on the commit path, and it is exactly the case where BitSet encoders cannot be constructed, leaving only RunLength. RunLengthEncoder accumulates the open run by the DELTA to the previously seen offset, not by counting calls. So for a maximal run [a,b], visiting a (opens the run) and b (adds b-a) yields the identical run length to visiting every offset, and extra visits inside the run telescope harmlessly. invoke() therefore only needs each run's first and last offset, which the incompletes already imply. The full scan is retained whenever any active encoder requires every offset (BitSet, ByteBuffer). That is expressed as OffsetEncoder.requiresEveryOffset(), defaulting to the safe value so a future encoder opts into sparse deliberately. Byte-identical output is enforced by a new 130-case differential test that runs both strategies over generated scenarios (fixed seed) and compares encodings byte-for-byte, and also asserts the sparse path is genuinely taken. RunLengthEncoderTest: 62.3s -> 0.2s. It was the slowest class in the unit suite and, because surefire cannot split one class across forks, a hard floor on the CI gate - as well as its largest source of run-to-run variance. Resolves the encoder's own long-standing TODO (VERY large offset ranges is slow). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Claude finished @astubbs's task in 4m 5s —— View job Claude is reviewing this PR
|
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
|
[superseded - a quarantined test changed outcome] 🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its Superseded by a newer quarantine lane report. |
A bare #NN means THIS fork. A bare number above the fork's own highest issue is therefore almost always an upstream reference pointing at the wrong repo - harmless-looking today, and wrong for real once the fork's numbering reaches it. Now that every upstream issue is mirrored, there is nearly always a fork number to use instead. The gate runs in the existing PR Checklist job, on ADDED lines only, and adapts as the repo grows: it reads the fork's current highest issue number from the API rather than hard-coding a threshold that rots. Three files are exempt because a bare number legitimately means upstream in them - CHANGELOG.adoc (its own header says entries below 0.6.0.0 predate the fork), upstream-map.yaml (every number is upstream by construction), and upstream-pr-analysis.adoc. Those are named in the module rather than left to judgement because a naive sweep on 2026-08-05 rewrote exactly those three wrongly. Same reason the rule skips javadoc member links, code spans and URLs. Escape hatch mirrors the changelog gate: "issue-refs: N/A - <reason>". Logic is a testable module with 11 assertions running before the gate, following changelog-ref-gate.js - that gate has shipped two real misjudgements, so this one proves its own rules first. PHASE 5 finished. Five single-mirror PRs carry both numbers in the title (#29, #31, #51, #57, #116). #53 and #106 span or only partly address several mirrors, so they get a Related issues section listing them WITHOUT a Fixes keyword - verified 0 closing references on each. A title listing three mirror numbers helps nobody, and Fixes on partial work is the error the #233 / #326 / confluentinc#857 mirrors already document. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
Review note: the changelog hunk needs removing
Why removing them loses nothing: the section for the release being cut is generated from What it asks of you instead: the commit message is now the raw material. A subject that says what changed and, where it matters, what it changed for a user - with the diagnosis and rejected alternatives in the body. Specific to this PRThe performance work is clearly user-visible - a multi-second scan on the commit path for large offset ranges is exactly what a changelog exists to tell people about - so this will get an entry at release time. What changes is the shape. The current hunk runs to roughly 150 words covering The mechanism detail belongs in the commit message and the PR body, where the generator will find it and where a reader who wants it can follow the link. |
Same class as the previous commit, three more sites the local gate found once the audit-branch fix merged up: - "#149, #160, #236" - the shorthand after the first qualified number still reads as a bare reference to the gate, and it is right to: nothing about position makes #160 unambiguous. - The salvaged OOM diagnostics carried "PR #49" verbatim from its branch. A salvage is not a reason to import a coin-flip reference, so it is qualified here too. - The plan's dependency diagram had "#106 / #207" for the same reason as the first item. bin/check-issue-refs.sh passes across all 20 changed files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SizQDD2hVUjD7EhESe9Hkb
…hanges) Directory move only, so every path is 100% similar and git's exact-rename detection cannot fail on it. The content edits follow in the next commit. Generated by bin/rename-packages.sh.
Text edits only. No file moves in this commit, so it cannot dilute the rename detection in its parent. Generated by bin/rename-packages.sh.
The branch has sat since early August while master gained several hundred commits, including the whole-repo io.confluent.* -> bz.stub.* package rename. The rename had already been applied to this branch by an earlier sweep, so both sides agree on where every file lives - which is what made this merge tractable rather than a rewrite. It still produced 115 conflicts, and all but a handful were artefacts of that rename landing on both sides independently. What master brings that this branch needs ----------------------------------------- #207, the offsets decode-path rework this PR declares a dependency on, is now in the tree: EncodedOffsetPair routes every unreadable payload through handleUnreadableMetadata, and the new typed exceptions (CorruptOffsetMetadataException, UnknownOffsetMetadataMagicException, UnsupportedOffsetEncodingException) arrive with it. Master also fixed three log statements in OffsetSimultaneousEncoder - they now pass the exception rather than its getMessage(), and the ByteBuffer arm no longer names BitSetEncoder in its own warning. All three merged into the branch's rewritten invoke() cleanly. How the mechanical majority was resolved, and why that was safe --------------------------------------------------------------- 82 of the 115 conflicts were proved to be rename-only before anything was touched: our side of each was normalised back to the pre-rename spelling (which has to collapse more than the top-level package, because the rename also folded io.confluent.csid.utils into ...parallelconsumer.internal.utils and io.confluent.csid.testcontainers into ...internal.testcontainers) and compared byte-for-byte against the merge base at that file's pre-rename path. Where the two matched, our side contributed nothing but the rename and master's outcome was taken - master's blob where master still has the path, a delete where master had removed or moved the file. This is the only condition under which taking the other side wholesale is defensible: a merge that takes theirs renders as nothing at all in review, so a silent loss would leave no trace to find. The remaining conflicts were resolved individually: Master's outright renames. InternalRuntimeException -> PCInternalRuntimeException and its test, and LongPollingMockConsumer moving from the test tree into src/main/java/.../internal/utils/. Each of these is a rename/rename conflict, and git's ort strategy had already content-merged them: the stage-2 blob at our path was byte-identical to master's blob at master's path, which is the proof that our side carried only the package rename. Master's path and blob kept; ours deleted. TestConventionsArchTest. Git mis-paired the near-identical per-module copies - it matched mutiny's base against reactor's ours and examples-core's theirs, and reactor's base against vertx's ours and examples-vertx's theirs. A checkout --theirs here writes a file that still contains conflict markers, and the markers carry paths rather than the plain form, so the habitual grep for '<<<<<<< HEAD' cannot see them. Master's blob was taken verbatim at all nine module paths and each was then diffed against master to prove no mis-pair survived - a mis-paired arch test compiles happily while pointing ArchUnit at another module's packages, which is silent loss of coverage rather than a build failure. The eight logback test configs. Our side's only non-rename change was deleting the dead io.confluent.csid logger entries; master made the same deletion in #289 and then rebuilt these files around the pc.log.level property and the shared logback-silos.xml include. Confirmed no csid logger survives on master's side, so master's is a strict superset. bin/check-copyright-headers.sh, bin/ci-mutation-test.sh, bin/rename-packages.sh. The branch took the rename tooling from master; master has since moved on. The copyright script's blob was found verbatim in master's own history, and the other two carried nothing our side originated - ci-mutation-test.sh differed from the merge base by the package regex alone, which master's version already spells bz.stub. Master's taken for all three. .idea/runConfigurations/. Master deleted these; our only change was inside a file that no longer exists. Master's deletion accepted. The two files carrying the branch's own work were resolved hunk by hunk rather than by taking a side. OffsetSimultaneousEncoder.java had a single conflict, on the lombok.AccessLevel import the sparse-path test seam needs - ours kept. RunLengthEncoderTest.java had one javadoc hunk where master had re-pointed the "needs a delta-aware invoke()" tracking citation from docs/inflight.md to docs/refactoring.md; ours kept, because the paragraph master was editing describes a limitation this branch removes. OffsetEncoder.java, RunLengthEncoder.java and OffsetSimultaneousEncoderSparseIterationTest.java did not conflict at all. The result differs from the merged master commit by exactly the branch's own change plus a regenerated docs/todo-index.md - nothing else survived the merge from our side, which is the check that the rename-only resolutions neither lost nor smuggled anything. CHANGELOG.adoc: the branch's entry is dropped --------------------------------------------- Repo policy changed while this branch sat. #113 moved release notes to generation from the commit log at release time, and AGENTS.md's Changelog section now states that a PR never adds an entry and never adds an Unreleased section. The review note on #106 asks for exactly this. Nothing is lost: the generator reads the commit log, and this branch's commit message already carries the full substance - the delta-accumulation argument, the byte-identical guarantee, the differential test, and the RunLengthEncoderTest 62.3s -> 0.2s measurement. README.adoc is generated from src/docs/README_TEMPLATE.adoc, so master's is taken. The template's only non-rename change on this branch - stop embedding the changelog in the README - had already been made independently on master, word for word, so master's side loses nothing there either. docs/inflight.md: master's deletion accepted -------------------------------------------- Master deleted the single-file ledger; in-flight state is now one file per item under docs/inflight/, and that directory tracks only what is OPEN. This branch's only edits to the old file were closing two items out as DONE - the RunLengthEncoderTest ~59s floor, and the delta-aware invoke() follow-up - both of which this PR resolves. A DONE entry has no home in the new directory, and no open item was carried on those lines, so the deletion loses nothing. None of master's docs/inflight/ notes track this mechanism as open. docs/todo-index.md is generated, and bin/todo-index.sh --check went stale because the branch removed two markers from OffsetSimultaneousEncoder.java: the "VERY large offset ranges is slow" TODO this PR resolves, and the "refactor this loop into the encoders" note attached to the loop it rewrote. Regenerated. Verified on JDK 17: whole-reactor ./mvnw clean install -DskipTests succeeds, and the core unit suite runs 833 tests with 0 failures and 0 errors (8 pre-existing assumption-based skips, none added here). The tree carries no conflict markers - swept for the marker anchored at line start, since a rename/rename marker carries paths rather than the plain form - and no io.confluent residue outside the copyright script's deliberate upstream-path tables and AGENTS.md's own prose about the rename. No test was weakened, skipped, excluded or quarantined. bin/check-issue-refs.sh is red on this commit and its findings are all inherited. The gate scopes itself to files changed since the merge base, and mid-merge that base is still the OLD one, so every file master touched reads as a line this branch added - the same artefact bin/check-branch-self-reference.sh documents in its own header ("MID-MERGE IT ACCUSES YOU OF MASTER'S SENTENCES ... the fix is to finish the merge"). Its three tree findings are the bare confluentinc#857 in parallel-consumer-core/src/test/resources/logback-test.xml and the bare #101/#110 in pom.xml; both files are byte-identical here to the master commit being merged, and neither line existed at the branch tip before it. The remaining six are in the PR body, which predates the gate and is the author's to settle. Once this commit exists the merge base is master's tip and the tree arm has nothing to report. Every other gate passes, including the copyright header check run with COPYRIGHT_CHECK_REQUIRE_FORK_POINT=1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
… merged tree bin/check-branch-self-reference.sh flagged the paragraph in bug-162 that names this PR. The single-line marker already there covered only its first line, so the sentence making the actual claim about this PR was unattested - the block form covers a paragraph, and this is a paragraph. The claim is that #106 does not address either truncation case, despite the mirror body implying it might. Re-checked rather than waved through: against master this branch changes only the offsets/ encoders and their tests, touches no state/ file, and PartitionState#maybeTruncateBelowOrAbove appears nowhere in its diff. The claim holds, and the sentence now says how it was established, so the next reader does not have to redo it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #106 +/- ##
============================================
+ Coverage 81.91% 82.64% +0.73%
- Complexity 1466 1491 +25
============================================
Files 95 95
Lines 5148 5192 +44
Branches 502 509 +7
============================================
+ Hits 4217 4291 +74
+ Misses 732 707 -25
+ Partials 199 194 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its No quarantined test changed outcome since the previous push. Updated for |
🟢 Throughput — OKThis branch measured about 4% slower than master, on the one test this measures. That is INSIDE this test's own run-to-run spread of about 17%, so read it as a reading and not as a result - re-running the same commit moves it by about as much.
Allowable range 🟢 ≥ 0.70 · 🟡 0.50–0.70 (about a 30% loss) · 🔴 < 0.50 (about a 50% loss) What the numbers mean, and what they cannot tell youThe one that gets misread. Why a shape and not a rate. A rate depends on which runner you drew. A shape does not: every test here processes a fixed number of records, so a runner twice as slow doubles the subject and the controls together and leaves their ratio alone. That is the whole trick, and it is why the reported rate is shown last and labelled as this machine only. Reading the comparison. By conservation, not by correction. Every test in this lane processes a fixed number of records, so within one run the ratio of one test's time to another's is invariant under machine speed — a runner twice as slow doubles both terms and leaves the ratio alone. There is no machine-index correction to be wrong, because nothing needed correcting. Per-method times, not class times. A class time is Reference is the median of 10 recent What this still cannot do. It removes machine-to-machine variance. It does not remove this test's own run-to-run variance, measured at about 30% on a single unchanged commit while its controls stayed within 5%. That is a property of the test, not of the comparison, and no arithmetic here can touch it — which is why the reference is a median and the bounds are deliberately coarse. 🟡 means look at this; only 🔴 is outside the measured spread. Runs used: ce6f39a, 70a88bf, 27211b9, c5dde06, 92364a1, 867c407, 7a8dd92, df3bd71, 440b9bd, 11e0500 Since the previous push: ratio 0.856 -> 0.96, share 1.951 -> 1.77, rate 71655 -> 65075 (-9.2%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
|
@codex review This is the cross-model pass, so please weigh the correctness argument independently rather than
Where I would look hardest:
Context worth having: this branch has just merged master across the The wall-clock figures the body used to quote are withdrawn and being re-measured; the class-level |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3255c6a879
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review finding on #105. The write-up's upkeep caveat only covered one direction - a newly added class is unranked and sorts last - and the reviewer pointed out that the failure mode actually in front of us is the opposite one, which is worse because it leaves nothing to see. Adding a class at least puts something unranked on disk. A class that gets much FASTER, or disappears, changes nothing observable: surefire's runOrder=balanced keeps sorting, by recorded costs that describe a suite which no longer exists, and the fork packing decays toward the unordered scan it replaced. No build goes red, because class ordering is not something an assertion can be wrong about. The live instance is core's file, ranked around RunLengthEncoderTest at roughly three times the next-slowest entry. #106 collapses exactly that class, so landing it without regenerating the statistics would quietly hand most of this change back. That obligation is recorded in docs/inflight/pr-blockers-and-collisions.md - the register a session actually inherits - rather than only in a PR body that stops existing at merge, and it names the remedy: a full bin/ci-unit-test.sh run on core, committing the rewritten files. Also states plainly that this change and #106 OVERLAP rather than sum. Packing removes the idle fork at the end of the run; #106 removes the work that made that fork long. Once the work is gone the tail is a fraction of its size, so packing keeps helping by proportionally less against whatever class is next in line. Neither makes the other pointless - a class scheduled last strands a fork for its own length, whatever that length is - but adding their gains together would double-count, and a reader of either PR alone would be inclined to. No wall-clock figure is added, restated or defended here: the PR's headline seconds remain withdrawn pending re-measurement against a control arm that #265 uncontaminated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
…w read them per line Corrects a claim ee9a3b2 carried in from the automated review on #105. That review compared the wrong unit and the error survived into both files that commit touched, so it is fixed here in the same terms it was made. THE MISTAKE, BECAUSE IT IS THE REUSABLE PART. Each line of a .surefire-pc-unit-times file is one test METHOD, and a heavy class is spread over many of them. surefire's runOrder=balanced sorts CLASSES - pom.xml's own comment beside the setting says so, and forking cannot split a class, so the class is the scheduling unit either way. Ranking the raw lines therefore compares one method of one class against a whole other class, and it badly overstates how far the top class stands above the next. The review read core's file that way, put RunLengthEncoderTest at roughly three times the next entry, and named a METHOD as the class that would define the tail after #106. Aggregated per class, the top two are close rather than a factor apart, and the third is well behind: awk -F, '{t[$3]+=$2} END {for (c in t) printf "%9.0f %s\n", t[c], c}' \ parallel-consumer-core/.surefire-pc-unit-times | sort -rn | head -5 Both files now state the shape and give that command rather than baking in a ratio, which would be wrong at the next stats refresh anyway. WHAT IT CHANGES ABOUT THE ARGUMENT, IN BOTH DIRECTIONS. The staleness risk is real and stays - an ordering built around a class that has since collapsed decays silently, with no red build - but it is materially smaller than the review stated. It also cuts the other way, in this change's favour: because forking cannot split a class, #106 moves the floor down to the second-heaviest class and no further, and that class is substantial. So the residual tail is most of the present one and packing keeps most of its value rather than being largely obsoleted. The "overlap, not sum" paragraph stays correct and stays put; only the implication that the residual tail is small is removed. No figure is asserted for #106's own collapse. The review took "62.3s -> 0.2s" from that branch's commit message, but that measurement predates #265's contaminated-arm fix and disagrees with both the committed statistics and a run on the merged tree, so the class is described as collapsing by an order of magnitude and the re-measurement now in flight can supply the number. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
|
@claude review this The previous Claude review on this PR is from 2026-08-03 and Two defects have already been found here by Codex and independently verified - please treat them as
The agreed fix bounds by the INCLUSIVE last offset ( Where I would like fresh eyes:
Context: this branch has merged master across the |
|
Claude finished @astubbs's task in 4m 16s —— View job Claude is reviewing this PR
|
|
Thanks - this is a substantive pass, and the reconstruction of the boundary proof is the part that Taking the points in turn. Where I am overriding you: dropping the early exitYou argue that removing the I checked the production call site, and the cost claim does not hold there. The
long highestSucceeded = partitionState.getOffsetHighestSucceeded();
var incompleteOffsets = partitionState.getIncompleteOffsetsBelow(highestSucceeded);
...
simultaneousEncoder = new OffsetSimultaneousEncoder(baseOffsetForPartition, highestSucceeded, incompleteOffsets);
So the break condition The backlog scan you describe is real, but it is attached to the wrong branch. It comes from the That makes the trade backwards: keeping the early exit would preserve an optimisation that provably The fix will carry a short comment at the loop saying this, so the next reader does not re-add the Worth noting on the same axis: One correctionFinding #1 is latent, not reachable in production. The trigger needs The third-defect sweepI ran the same sweep independently before this review posted, and reached the same negative. Stating
Two independent passes agreeing on "none, and here is the ground covered" is a real result, though TestsBoth regression scenarios are going in regardless of the convergence between your suggestion, Agreed too that The fix is not applied yet: a wall-clock measurement is currently using this worktree, so the tree |
…ng the incompletes iterate ascending The in-range filter added by the sparse-iteration change carried two defects, both found by cross-model review (Codex, then confirmed independently by a Claude pass). Both break the same premise, which is what makes them correctness bugs rather than edge cases: the boundary proof behind sparse iteration holds only if the visit set contains EVERY in-range incomplete. Miss one and the run boundary beside it is never visited, so the run is absorbed into its neighbour and the encoding silently diverges from the full scan - no exception, a bad commit. FIRST, THE EXCLUSIVE BOUND WRAPPED. getEndOffsetExclusive() is lowWaterMark + length, i.e. highestSucceededOffset + 1, which is Long.MIN_VALUE when the highest succeeded offset is Long.MAX_VALUE. That is reachable without tripping the constructor's length-negativity guard: a base of Long.MAX_VALUE - Integer.MAX_VALUE gives a positive length one wider than the bitset limit, which is itself what forces the sparse path. Every incomplete then compared as out of range, the loop exited on its first entry, and the whole span encoded as one completed run - incomplete offsets encoded as complete. That run also overflows the v2 int run length, so the symptom was an empty encoding map: a commit with no encoding at all. The bound is now the INCLUSIVE last offset, which reconstructs highestSucceededOffset - a long that already existed - and so cannot wrap. SECOND, THE EARLY EXIT ASSUMED ASCENDING ITERATION. incompleteOffsets is caller-supplied on a public constructor, and the field's own javadoc promises "no order requirement, but SortedSet just in case". A reverse-ordered set presenting an above-range offset first exited the loop before reaching a valid in-range incomplete. Note SortedSet#subSet, which this hand-written loop replaced, would have honoured the set's comparator - avoiding subSet for its own good reason must not import an ordering assumption it never had. THE EARLY EXIT IS REMOVED RATHER THAN REPAIRED, AND THE REVIEW ARGUED OTHERWISE. The Claude pass proposed keeping the break and asserting natural ordering in the constructor, on the grounds that a full scan reintroduces an unbounded walk of the incompletes backlog. Checked at the call site, that cost does not exist: OffsetMapCodecManager#encodeOffsetsCompressed samples highestSucceeded ONCE and derives both the set (getIncompleteOffsetsBelow, keeping x < that sample) and this encoder's range top from it, so the set's maximum is highestSucceeded - 1 while the break needs > highestSucceeded. It cannot fire in production. The backlog scan is the `continue` for entries below lowWaterMark, identical before and after. Both shapes are O(number of incompletes), neither is O(range), so the method's docstring claim survives. Keeping the break would preserve a no-op and pay for it with a precondition the class explicitly disclaims. The loop now carries that reasoning so it is not re-added. TESTS THAT FAIL WITHOUT THE FIX. Two regression guards, both verified by sabotage: with the old loop restored, each fails with the predicted symptom - an empty encoding map for the Long.MAX_VALUE range, and a byte mismatch against the full scan for the reverse-ordered set - while all 130 pre-existing scenarios still pass, which is the direct evidence that the fixed-seed differential test could never have caught either. It generates neither shape: every scenario is a small range at a modest base offset, naturally ordered. Also fixes two SpotBugs findings on lines this change wrote (an unpresized collection, and a helper folded into its only caller). The third, DMI_RANDOM_USED_ONLY_ONCE, is a false positive on a deliberately seeded generator - four formulations were tried and it fires on all of them; the javadoc records which, so the next person does not repeat them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
|
Outcome of your review, now landed in The early-exit disagreement: it went the other way, and here is the checkYou argued for keeping the
The backlog scan you described is real, but it belongs to the So keeping the early exit would have preserved an optimisation that provably does nothing, and paid Your test-coverage point was right, and is now settled by evidence rather than argumentYou said the differential test is structurally incapable of catching finding #1. Rather than
That last line is the proof - not that the suite happens not to generate these shapes, but that it Accepted without changeYour reading of the boundary proof's unstated premise - that it holds only if the visit set contains One correctionYou described finding #1 as "reachable in production". It is latent - the trigger needs Thanks - the review moved the change, including on the point where I ended up disagreeing with it. |
… success Merging master across the package rename produced rename/rename conflicts on the near-identical per-module TestConventionsArchTest files. Resolving them with `git checkout --theirs` staged files that still contained conflict markers: the checkout said nothing, `git add` said nothing, and the marker sweep that followed said nothing either. javac caught it. Two things make that worth a document rather than a commit message. The sweep missed it because the markers are not the shape everyone greps for. On a rename/rename conflict git writes eight angle brackets and a <ref>:<path> label; a directory-rename conflict writes seven, also with the label. Both were reproduced with `git merge-tree --write-tree --messages` for this write-up. So neither the bracket count nor the label is a safe discriminator, and `grep '<<<<<<< HEAD'` matches neither. Only a prefix-anchored `^<<<<<<<` is stable - which is also what the still-unwritten gate proposed in docs/inflight/ci-conflict-marker-gate.md will have to match, since that note describes only the seven-character form. And it was the second occurrence, not the first. A session on 2026-09-02 hit the same flag on add/add conflicts from a squash-merged parent, diagnosed that the index was still unmerged, and worked around it. Nothing was written down, so it had to be rediscovered. The mechanism is now recorded: in a rename/rename conflict stages 2 and 3 are held at DIFFERENT paths, so the path named in --theirs has no stage 3 to write. The guidance that generalises is resolve by explicit ref, not by side. The repo already used that technique - `git checkout origin/master -- <path>` appears in bin/rename-packages.sh, in the red-proof solution doc and in the package-rename sweep note - but always as a way to fetch a file, never written down as the way to resolve a conflict. This sharpens rather than repeats the existing --theirs write-up. That one is the flag taking too much; this one is the flag taking nothing and reporting success. It also contradicts AGENTS.md's PR Discipline bullet, which says both flags take the whole file - true for the ordinary case, and exactly the wrong preparation for this one, because a reader primed for over-application checks what was lost, finds nothing missing, and moves on. Rides with this PR rather than opening its own: the learning came out of this branch's merge, and a separate PR would cost a review cycle to land one document. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
…y, the git add after it does not The grounding validator reproduced the incident on git 2.51.2 and falsified the headline of the document committed a few minutes ago. Corrected here rather than left standing, because a solutions doc is read by agents who will not re-derive it. WHAT WAS WRONG. The doc said `git checkout --theirs` resolves nothing and reports success. It does not report success - on a rename/rename conflict it exits 1 with `error: path '<p>' does not have their version`, because stages 2 and 3 are held at different paths and the path you named has no stage 3. The silent step is the `git add` that follows: it exits 0, stages whatever the merge left in the worktree, and resolves the index around markers. `git ls-files -u` is then clean. Also wrong, and repeated in three places: that `grep '<<<<<<< HEAD'` matches neither marker form. It matches both whenever the ours side is labelled HEAD, which is what an ordinary merge writes - seven angle brackets plus a space is a substring of eight plus a space. That claim was asserted and never measured. What actually defeats the prior sessions' sweeps is pinning the bracket count, not the trailing space they happened to carry. WHAT THE REPRODUCTION ADDED. A batched `git checkout --theirs -- <many paths>` validates the whole pathspec first and aborts on the first path lacking a stage 3, so ordinary content conflicts sharing that command line are left unresolved too, and the following blind `git add` stages markers into files that had nothing wrong with them. That is a larger blast radius than the doc described and the strongest argument in it for resolving per path and checking every exit status. Three smaller corrections: the nine arch-test copies are not uniform - eight match, core differs by an import and a javadoc word; the pairing with the existing --theirs write-up is a trio once the add/add abstraction doc is counted; and the claim that a history-rewrite hook prevented reproducing the exit code was untrue, which is what let the wrong headline through in the first place. The guidance is unchanged and now rests on the real mechanism: resolve by explicit ref, and check the status of the command rather than the look of the tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
#106 has landed, which is the event this branch was waiting for. It collapses the runtime of RunLengthEncoderTest - the class this branch's checked-in .surefire-pc-unit-times ordering is built around - so the statistics here now rank the suite by a cost that no longer exists. Regenerating them is the next commit; without it the packing decays toward the unordered scan with no red build to say so. ONE CONFLICT, RESOLVED BY KEEPING BOTH SIDES. docs/inflight/ci-codecov-flags-not-like-for-like.md gained a different 2026-09-07 sighting on each side. Ours records that a PR and its base disagree on the unit flag's file set even though the base is a git ancestor of the head. Master's records that master's own unit history oscillates between two stable bands across consecutive commits, independent of diff content. They are not the same finding and neither subsumes the other - master's text says so itself, naming this branch's sighting and explaining why it is a different shape. Both are kept, ours first and master's as the follow-on it is written as. Resolved by editing the hunk rather than taking a side, which is the rule the write-up that merged with #106 exists to state. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
…ed base #106 landed, and the obligation it recorded comes due here. These files rank test classes by recorded runtime so surefire can schedule the slowest first; core's ranking was built around RunLengthEncoderTest, whose cost that PR removed. Left alone they would sort the suite by a cost that no longer exists, and the packing would decay toward the unordered scan it replaced - with nothing going red, because ordering is not something an assertion can be wrong about. Regenerated by a full bin/ci-unit-test.sh run on this tree, so the data describes exactly the workload the gate executes. Reproduce the ranking with: awk -F, '{t[$3]+=$2} END {for (c in t) printf "%9.0f %s\n", t[c], c}' \ parallel-consumer-core/.surefire-pc-unit-times | sort -rn | head Two things the new ranking settles. RunLengthEncoderTest drops by roughly two orders of magnitude, which is the first measurement of #106's effect taken on the merged tree rather than quoted from a gate that no longer exists. And the class that now sets the tail, ParallelEoSStreamProcessorTest, is only slightly cheaper than the one it replaces - so the tail this branch packs around did not collapse with the class. A review of #105 had estimated it would fall to about a third, by comparing per-METHOD lines when runOrder=balanced sorts per CLASS; that estimate was wrong in the direction that would have argued against this branch. Whether the packing is worth its keep is still open and still a measurement, not an argument. The comparison that settles it - this tree against master, interleaved, repeated - is what these statistics now make possible for the first time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
Master moved 21 commits under this branch in two days. Three touch the rider's own files and none argues with it: #450 registers a record's offset before publishing it to its shard (PartitionState, adjacent to but not inside the commit path this branch changed); #452 makes the codec's encodingCounters a ConcurrentHashMap (the encode-path static this branch's tryToEncodeOffsets calls through); #106 walks only the offsets where completion state changes when no encoder needs every offset (the overhead table's domain is unchanged by it, and its tests pass on the merge). Two text conflicts, both in append-only records, both resolved in master's favour plus this branch's additions: config/infer-known-findings.txt keeps master's retirement of every THREAD_SAFETY_VIOLATION identity (#433) and this branch's three BUFFER_OVERRUN_L1 identities; docs/refactoring.md keeps master's rewritten thread-safety entry for the codec (#452 replaced the "not thread-safe" heading and body) and this branch's two entries above it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkRtYPnDuAPLEmHFYSnAiJ
…rop it, keep the findings The comparison this branch was waiting for became possible when #106 merged, and it went against the change. Two arms sharing that base, statistics regenerated on the very tree under test, interleaved, three repetitions each: the packing arm was slower in every paired repetition. A small, consistent loss rather than no measurable effect. So the mechanism goes and the knowledge stays. Removed: the ci-profile runOrder properties, the nine checked-in statistics files, and the copyright-scanner exemption that existed only to classify them. What is left is documentation. WHY IT COULD NOT HAVE BEEN MEASURED BEFORE, which is the part worth keeping. Packing was built around RunLengthEncoderTest, an un-splittable class that dominated both the wall time and its variance. Judged with that class intact, the ordering was being measured in the arrangement that repo's own integration-lane write-up says buys nothing for any arrangement. #106 collapsed the class; only then could the ordering be re-derived and the two compared. Shrink first, re-derive, then measure - the same result from the other direction. AND #106 DID NOT ONLY MAKE THE GATE FASTER, IT MADE IT MEASURABLE. Repeat runs of identical code used to differ by up to a minute and a half; across all six runs here the spread was seconds. The variance was that one class landing in different fork slots. That is why a difference of a few seconds resolves now when ninety could not before, and it is worth more than the verdict. The write-up keeps its two original findings - Maven -T module parallelism loses on a 2-core gate and manufactures flakes, and a harness must not mutate its own input - and now carries the null result. Its pre-measurement prediction, that packing would keep most of its value once the dominant class shrank, is left visible and marked wrong: the per-class arithmetic behind it was right and the conclusion still did not follow. A mechanism that is real is not the same as a mechanism that pays. The reproduce recipe is written out rather than cited, because the harness lives under the ignored .context/ directory and a pointer there resolves for nobody. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
…only became measurable after #106 (#105) test(ci): fork-tail packing measured and rejected - and why the gate only became measurable after #106 This branch set out to cut wall time off the required Unit Tests gate by packing the surefire fork tail: run the slowest test classes first so no fork idles at the end. The mechanism was built, the plumbing that makes it work on CI was found, and it was then measured properly and REJECTED. What lands is documentation - a null result and the findings around it. No Java, no pom.xml change, no checked-in data. WHY IT LOOKED RIGHT. Surefire's forks pull classes from one shared queue and the run ends when the last fork finishes, so scheduling the longest job first is textbook longest-processing-time packing. Surefire 3.5.5's runOrderStatisticsFileChecksum made it usable on CI for the first time by pinning the statistics filename, so the recorded runtimes could be committed and survive a cold checkout. WHY IT COULD NOT BE JUDGED UNTIL NOW, which is the part worth keeping. The packing was built around one un-splittable class that dominated both the gate's wall time and its variance. Measured with that class intact, the ordering was being judged in exactly the arrangement this repo's integration-lane write-up says buys nothing for ANY arrangement - the dominant class still setting the floor. #106 collapsed that class; only then could the ordering be regenerated on the merged tree and the two compared. Shrink the dominant class first, re-derive, then measure. THE RESULT. Two arms sharing that base, statistics regenerated on the very tree under test, interleaved, three repetitions each, in a container capped to the runner's core count: the packing arm was slower in every paired repetition, by a few seconds at the median. A small consistent loss rather than no measurable effect. The write-up states the shape and the reproduce recipe rather than the numbers, which go stale on the next run. THE FINDING THAT OUTLIVES IT. #106 did not only make the gate faster, it made the gate MEASURABLE. Repeat runs of identical code used to differ by up to a minute and a half; across these runs the spread was seconds. The variance everyone had been fighting was that one class landing in different fork slots. That is why a difference of a few seconds resolves now where ninety could not before, and it changes what any future measurement on this gate is worth. ALSO HERE. Maven -T module parallelism loses on a 2-core gate, measured, and starves wait-heavy tests into awaitility timeouts - pick one parallelism axis per core budget. A benchmark harness must not mutate its own input, since surefire rewrites the ordering statistics in place and a run then inherits the previous run's fork placement. A codecov sighting where a PR containing no Java, compared against a base that is a proven git ancestor of its head, was still reported as carrying more files and more lines than that base - recorded with its evidence and explicit about which half of the existing note it does not test. And an AGENTS.md worktree rule: name the tree on every commit, because a bare git commit resolves against the session root, which is some other worktree. The write-up keeps its own pre-measurement prediction, marked WRONG and left visible: it argued packing would keep most of its value once the dominant class shrank. The per-class arithmetic behind that was right and the conclusion still did not follow. A mechanism that is real is not the same as a mechanism that pays, and only a measurement tells the two apart. The wall-clock figures this work originally quoted are withdrawn rather than corrected: they predate #265 closing a leak that made dependent modules run thread-parallel regardless of their own setting, so they describe a gate that no longer exists. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Description
OffsetSimultaneousEncoder.invoke()visited every offset between the committed base offset and the highest succeeded offset. When that range spans a large gap - up to ~2.1 billion - it becomes a multi-second scan on the commit path. This PR makes it visit only the offsets where the completion state actually changes, when it is safe to do so.This resolves the encoder's own long-standing TODO: "VERY large offset ranges is slow (Integer.MAX_VALUE) - encoding scans could be avoided".
Why this is safe
RunLengthEncoderaccumulates the open run by the delta to the previously-seen offset, not by counting calls:So for a maximal run of same-state offsets
[a,b]:asees a state change, closes the previous run and opens a new one of size 1;badds deltab-a, giving exactlyb-a+1;(a,b)telescope to the same total, so redundant visits are harmless.Visiting each run's first and last offset is therefore byte-identical to visiting all of them. Both ends are required - visiting only run starts would close every run at length 1 - and offset
0is required as the initial anchor, sincepreviousRangeIndexstarts atKAFKA_OFFSET_ABSENCE.Run boundaries can only occur at an incomplete offset or immediately next to one, so the visit set is
{0, length-1}plus each in-range incomplete's own offset and its two neighbours. It is sized by the number of incompletes, never by the range - which is the whole point when the range can be ~2.1 billion wide.What is NOT changed
BitSetandByteBufferencoders write one unit of output per call and genuinely need every offset. The full scan is retained whenever any active encoder requires it. That is expressed asOffsetEncoder.requiresEveryOffset(), which defaults to the safe value (true) and is overridden only inRunLengthEncoder- so any future encoder gets correct behaviour without knowing this optimisation exists, and opts in deliberately.Note the sparse path is reachable in production precisely because the large-range case is the one where
BitSetEncodercannot even be constructed, leaving only theRunLengthencoders.Verification
A new differential test (
OffsetSimultaneousEncoderSparseIterationTest, 130 cases, fixed seed) runs both strategies over generated scenarios and asserts the encodings are byte-for-byte identical, that the key sets match, and - importantly - that the sparse path was actually taken (otherwise the test could pass while testing nothing). Scenarios cover: zero-length and single-offset ranges, no/all incompletes, incompletes at first/last/middle, adjacent blocks at both ends, incompletes outside the range, alternating runs of width {1,2,3,17,500}, and randomised lengths {1,2,3,7,64,999,5000} × densities {0, .01, .1, .5, .9, 1.0} with varied base offsets. Plus an independent round-trip throughpackSmallest→decodeCompressedOffsets, a test that the full scan is used while a BitSet encoder is active, and the ~2.1B-range production trigger.No existing test is weakened, skipped or excluded.
RunLengthEncoderTestchanges are javadoc only - its assertions (hasSize(2)/hasSize(0)) are untouched; the old comment asserted the encoder "walks every offset", which this PR makes false.The two defects review found, and how the tests were proven
Codex found two defects in the first version of the in-range filter, both confirmed by an independent Claude review, both fixed in
0583b385a. They break the same premise, which is what makes them correctness bugs rather than edge cases: the boundary proof holds only if the visit set contains every in-range incomplete. Miss one and the run boundary beside it is never visited, the run is absorbed into its neighbour, and the encoding silently diverges from the full scan - no exception, a bad commit.Both are latent, not reachable by a running deployment: the first needs
highestSucceededOffset == Long.MAX_VALUE, and the only production supplier of the incompletes set collects into a natural-orderTreeSet.The regression tests were verified by sabotage, not assumed. With the old loop restored, each new test fails with its predicted symptom - an empty encoding map for the
Long.MAX_VALUErange, a byte mismatch against the full scan for the reverse-ordered set - while all 130 pre-existing scenarios still pass. That is direct proof the fixed-seed differential test could never have caught either defect, rather than an assertion that it does not happen to generate the shapes.Core suite green on the merged tree - 0 failures, 0 errors, with only the suite's pre-existing assumption-based skips. Reproduce with
./mvnw -Pci -pl parallel-consumer-core -am test.Side benefit: the CI unit gate
RunLengthEncoderTestdrops from tens of seconds to a fraction of one. It is the slowest class in the unit suite and - because surefire cannot split a single class across forks - a hard floor on the 2-core gate, so removing it moves the whole gate rather than just that class. It is also the suite's largest source of run-to-run variance (the same class has been measured anywhere from 67s to 166s for identical code, depending on fork placement), which is what had made the gate hard to benchmark at all.Warning
The gate-level seconds originally quoted here have been withdrawn pending a re-measurement. They were taken before #265 closed a leak in which core's
junit-platform.propertieswas packaged at the root of the core tests jar, so every dependent module ran thread-parallel regardless of its ownparallel-testssetting. This branch has now merged that fix, so the old baseline describes a gate that no longer exists. The class-level win is not in doubt - it is a change in algorithmic complexity, visible in isolation - but the whole-gate delta is being re-taken on the merged tree. Tracked indocs/inflight/branch-parallel-measurement-contamination.md.Reviewer notes
dropEncodersRequiringEveryOffset()is a package-private test seam on a production class - it reproduces the production encoder line-up on small, fast ranges so the differential test doesn't need a 2.1B-wide range. It is a mutator that production code must never call; flagging it explicitly rather than burying it.activeEncoderson first throw and so never reachesregisterEncodings, leaving it absent fromencodingMapeither way - and the overflow predicate is the same, since both paths reach the same total run length. Covered by the differential test and bytestSimultaneousWithOverflowErrors.SortedSet#subSet, deliberately:subSetthrowsIllegalArgumentExceptionif it is ever handed an already-restricted view, and that would surface as a failure to encode a commit. That rationale stands; the first version of the loop did not. It bounded by an exclusive end offset and exited early on the first out-of-range entry, and cross-model review found two defects in exactly those two decisions (see below). It is worth being explicit thatsubSetwould have honoured the set's own comparator - so the hand-written replacement was worse on precisely the axis the original note claimed safety on. The loop now bounds by the inclusive last offset, which reconstructshighestSucceededOffsetand so cannot wrap, and it does not exit early: the early exit encoded an ordering assumption the class explicitly disclaims, and could never fire in production anyway, becauseencodeOffsetsCompressedsamples the high-water mark once and derives both the incompletes set and this range top from it. The reasoning is in the code, so it is not re-added.Relationship to #105
Independent - different files, and either can merge first. Both have since merged master and dropped their changelog entries under the policy that a PR never adds one, so the rebase they once needed against each other is gone. #105 covers CI test scheduling; this PR reduces the actual CPU work, and the two compose.
Related issues
Mirrors this touches, without a
Fixeskeyword - it improves the offset-encoding hot path butdoes not resolve either issue.
consumer". This reduces one cost on the commit path; that report is dominated by other factors
(see the diagnosis on the mirror).
run-length/bitset encoders exist, and what constrains replacing them.
Checklist
CHANGELOG.adoc, under== Unreleased) -N/A - the entry this PR originally carried was dropped when it merged master. Repo policy changed while this branch was open: a PR never adds a changelog entry, because the file is generated at release time from commit messages. The substance is in the commit body.docs/inflight.mdentries this closed out went with that file when master split it intodocs/inflight/; nothing in master's per-item notes tracks this work as open, anddocs/todo-index.mdis regenerated because this PR deletes two markersN/A - no CI, workflow, runner or permission changes; this is a library code change.🤖 Generated with Claude Code
https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC