ParparVM collector: long-shift codegen fix, live-set heap goal, and an arm64 parallel-mark run - #5717
Open
shai-almog wants to merge 24 commits into
Open
ParparVM collector: long-shift codegen fix, live-set heap goal, and an arm64 parallel-mark run#5717shai-almog wants to merge 24 commits into
shai-almog wants to merge 24 commits into
Conversation
BC_LSHL_EXPR and BC_LSHR_EXPR masked the shift count to 6 bits but left the
left operand alone, and the translator emits a long constant as a bare C
literal. LCONST_1 therefore arrived as an int and the whole shift was 32 bits
wide, so shifting a long constant by 31 or more silently produced the wrong
number:
1L << 31 gave -2147483648 (int overflow, then sign-extended)
1L << 32 gave 1 (int shift counts are masked to 5 bits)
1L << 33 gave 2
Shifting a long VARIABLE was always correct, because the variable carries
JAVA_LONG into the macro where the constant does not -- which is how this
survived. BC_LUSHR_EXPR already cast, so the same bug was fixed once for the
unsigned shift and never carried across to its two siblings.
Found from a benchmark histogram whose bucket labels came out negative, which
is impossible for the long expression that computes them. LongShift checks all
three forms against a reference built by repeated doubling, so the check cannot
share a shift bug with what it is checking.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CN1_BIBOP_GC_TRIGGER_BYTES was the floor outright, so a process holding almost
nothing live still let 24MB of garbage accumulate before collecting and the
BiBOP page pool sized itself to that. Resident memory tracked the trigger almost
linearly and nothing else -- measured on the backend, /plaintext at 64
connections:
trigger 4MB -> 30MB RSS trigger 24MB -> 98MB RSS
trigger 8MB -> 49MB RSS trigger 48MB -> 102MB RSS
trigger 16MB -> 68MB RSS
Throughput and p99 across that sweep were flat inside run-to-run noise, so the
24MB floor was buying footprint and no speed. Every modern collector sizes the
next heap against the LIVE set instead -- Go's GOGC=100 means "collect when the
heap reaches twice what survived" -- which is why a Go server holding nothing
live sits at 6-17MB where this sat at 98MB.
The floor is now the live set plus CN1_BIBOP_HEAP_GROWTH_PERCENT of it, never
below CN1_BIBOP_GC_MIN_TRIGGER_BYTES, applied at all three clamp sites (shrink,
growth ceiling, low memory). It is not merely smaller: an application with a
real live set gets a LARGER floor than the old constant (20MB live at 100%
growth asks for 40MB where the constant gave 24MB), so this is more generous
exactly where the old rule was stingy and tighter only where it was wasteful.
Measured after: 98MB -> 38MB loaded RSS on the same route, with throughput and
p50 improving rather than regressing.
Also adds cn1GcMutatorAssist: a thread parked on the run-ahead cap marks a batch
instead of sleeping, which is Go's mutator assist in the one place this had a
sleep-until-the-cycle-ends park. It registers in gcMarkActiveWorkers before
releasing the worklist mutex so the "last worker out declares done" termination
cannot fire while it holds a batch, refuses to recurse when the thread is
already inside a mark, and assists only the parallel path -- the serial drain
touches the worklist without the mutex. It is therefore INERT until the mark
pool is enabled; see the CI workflow added separately.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gcMarkResolveThreadCount compiles the mark pool out unconditionally. The comment there is not a tuning note: it records that arm64 Linux still corrupted the heap with the pool enabled after one acquire-load fix, and that a second ordering hole was never located. The pool has been dead code since, so the collector marks on one thread however many cores it has. What that costs is measurable. On the GcPause benchmark added here -- one mutator, 20M short-lived objects, a 4096-node live set, every iteration timed into a log2 histogram -- the worst mutator pause is 2.2-3.3s with one marker and 0.3-0.9s with four, against Go's 20ms on the identical loop. Median and p99 are identical to Go's at 32ns/64ns, so this is not a throughput deficit, it is a stop that lasts seconds. Reproducing the corruption is what this workflow is for, because it could not be reproduced on an Apple-silicon podman guest. GcPause ran clean with four markers. A multi-threaded stress with graph rewiring and cross-thread resurrection ran clean too -- but that result is worthless, because the same stress also passes with the SATB write barrier compiled out (-DCN1_DISABLE_SATB), which it must not, and passes under -DCN1_GC_VERIFY at 6.6M references checked. A four-CPU hypervisor guest is a weak generator of the store interleavings a memory-ordering bug needs. These runners are native arm64 hardware, which is where the corruption was actually seen. The matrix runs one marker as the control, four markers on arm64 as the question, and four markers on x64 so a failure can be attributed to the architecture rather than to the pool itself. No default changes. The marker count is raised only for this workflow, through the CN1_TEST_EXTRA_CFLAGS hook the GC tests now honour; unset, every test compiles exactly as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 3ce45617b4
ℹ️ 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".
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
Contributor
Cloudflare Preview
|
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 166 screenshots: 166 matched. |
Collaborator
Author
|
Compared 166 screenshots: 166 matched. |
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
…he floor Four review findings, all of them correct. 1. The mutator went ACTIVE again in the middle of the GC handshake. threadActive is what tells the collector it may scan this thread's roots without stopping it, so raising it after the sleep -- when threadBlockedByGC may have gone up during that sleep and a conservative scan already be walking this stack -- let the mutator, and the assist running mark functions on it, move the stack underneath the scan. It now waits the block out before reactivating, matching what the tail of the function already did. 2. The injected flags concatenated with the test's own: extraCFlags() had a leading space and no trailing one, so a fault-injection build asked for -DCN1_GC_MARK_THREADS=4-DCN1_GC_NO_FORCE_STOP -- one undeclared macro rather than two, which clang accepts. It is padded on both sides now, and the tests that pass their own flags go through cFlagsArg/objcFlagsArg. 3. The hook reached two of the six tests in the matrix, so four of them compiled the default single-marker collector and a green matrix would not have validated what the workflow claims. Worse than the review said: moving the hook into cmakeToolchainArgs fixed only two more, because the rest build their cmake command inline with hardcoded compilers. Found by probing with an invalid flag -- the build should have failed and did not. All six are wired now and the probe fails as it should, with the flag visible on the clang command line. 4. liveBytes is not the whole live set. The sweep walks the retired-page list and a page the major sweep splices out of a partial pool is deliberately withheld from policy statistics, so sizing the floor from one sample let it collapse toward the minimum while a large live heap sat on pages the cycle never looked at -- the collector would then retrace that heap every few megabytes. There is no registry-wide live count to use instead, so the floor now tracks a decaying high-water mark: a recent cycle that did see a large live set holds it up, and a genuinely small one walks it down in a few cycles. Also fixes the workflow itself, which failed on all three arms including the control: mvn package does not install, so -pl tests could not resolve the translator. It now packages with -am and tests with -am, mirroring parparvm-tests.yml. GC suite 6/6 locally with the hook unset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6791382eae
ℹ️ 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".
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
…ell as lower Two CI failures and one review finding, all in the trigger floor. GcSteadyState failed in CI with "Pinning the free-memory reading to 16MB did not make the per-thread pending table fill, so this scenario and its twin measure nothing" (pendingFullParks=0). That is the test asserting its own non-vacuity, and it was right: the low-memory branch had been switched to the live-set floor, which collects early enough under a 16MB pin that the pending table never fills. Low memory mode is about surviving pressure rather than about footprint, so it goes back to pinning the constant and the floor no longer applies there. The floor also only ever lowered the trigger. A trigger that had adapted down before the live set grew stayed down: 4MB against 20MB live keeps retracing that live heap every 4MB of allocation, because the shrink branch compared in one direction only. It now clamps upward to the floor as well, which is the only path that corrects a trigger which adapted below what the live set justifies. CompilerHelper carried no copyright header; the gate checks files a PR touches, so modifying it required adding one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 169f2a1464
ℹ️ 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".
Collaborator
Author
|
Compared 160 screenshots: 160 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
GcSteadyState failed itself in CI again on the previous commit: "Pinning the free-memory reading to 16MB did not make the per-thread pending table fill, so this scenario and its twin measure nothing", with cyclesOnDemand=5598 and 22331 volume parks. Reverting the low-memory branch to the constant was not enough -- the trigger was sitting at the 4MB minimum in the NORMAL path, so the collector ran continuously and the pending table never filled. Review found the same edge from the other side: liveBytes is a per-sweep sample that systematically understates, because the sweep walks the retired list and pages the major sweep splices out of a partial pool are withheld from policy statistics. A decaying high-water delays that rather than fixing it -- about twenty low-survival cycles and a stable 20MB live set is estimated at the minimum, after which the collector retraces that heap every few megabytes. The same review noted ProcessBudgetPacingIntegrationTest derives its 72MB static cap floor from this constant, so lowering it can fail that test too. CN1_BIBOP_GC_MIN_TRIGGER_BYTES therefore defaults to the old constant. The proportional rule now only ever RAISES the floor: an application with a real live set gets more headroom than the constant gave it, and nothing gets less. A deployment that knows its live set is tiny defines it lower at build time -- the backend does, and that is where the 98MB to 38MB resident-memory measurement came from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7269ca8b24
ℹ️ 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".
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
…ctly-once Three review findings on the GC/profiling changes, all of them real. cn1GcMutatorAssist declared marking complete on the worker count alone. A regular marker only decrements after re-checking the worklist at the top of its loop, so it can never finish with work outstanding; the assist decrements immediately after gcMarkFlushLocal, which may have just published children this batch discovered. With CN1_GC_MARK_THREADS > 1 the assist could take the count to zero against a non-empty worklist, end the parallel drain with reachable subtrees unscanned, and let the following sweep reclaim them. Termination now requires an empty worklist as well. The allocation profile hooked only codenameOneGcMalloc and cn1BibopFastAllocNoZero. CN1_FAST_NEW calls cn1BibopFastAlloc -- a different function -- and cn1AllocFused and cn1FusedLatin1Begin are two further entry points, so a build routing through any of them under-reported silently. All four entry points now record, and each hook sits on the success return rather than at entry: a fast path returning 0 falls back to codenameOneGcMalloc, so the old entry-side hook counted that allocation twice. cn1BibopAlloc stays unhooked on purpose, being an internal callee of three of them. The fused latin1 path attributes the String and the byte[] payload separately, since the profile is read to find out what is being allocated. The per-class average divided by count|1, which for an even count changed the divisor rather than guarding zero: two allocations reported bytes/3. Verified by compiling cn1_globals.m in both the default and CN1_GC_CONFORM configurations, each against a pre-change baseline, with undeclared-identifier probes at the new header and .m hook sites to prove the regions are really compiled rather than preprocessed away. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The profile counts requested bytes at allocation time; allocatedKb accumulates BiBOP slot bytes at GC cycle boundaries only. A previous version of the profile double-counted fast-path fallbacks and still agreed with allocatedKb to within 2%, because the over- and under-counting cancelled. The note exists so that agreement is not read as verification a second time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1493d4b0fa
ℹ️ 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".
…file omitting classes silently Two review findings, both real. The live-set trigger floor was only applied inside the two survival branches, which cover >= 25% and <= 20% and nothing between. A trigger that adapted down before the live set grew therefore sat uncorrected for as long as survival stayed at 21-24%, retracing a live heap it no longer had the headroom for. The floor is a property of the live set rather than of the survival rate, so it is now enforced after the branches. Only the raising direction is taken, which is what the <= 20% branch already did, so the dead band gets the behaviour the branches either side of it already had. The allocation profile dropped any class whose id fell outside its table, and dropped it from the total as well as the ranking -- so a profile missing its hottest class read exactly like one that found nothing there. Class ids number scalar classes and array classes in one contiguous range, so an app well inside the limit on scalar classes can still put its array classes past it. The bound is raised past any plausible translated app, and anything still outside is counted, added to the total, and printed as an explicit row naming the highest id seen. This file already had one instrument that lied by omission; it should not have a second. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70dad8d317
ℹ️ 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".
A per-class total answers "what is being allocated" and not "which line allocates it": byte[] is one class and a dozen unrelated call sites, and a profile that says byte[] is 37% of a route leaves the next step to guesswork. Sizes separate the sites, because the sites differ in what they allocate. Set CN1_ALLOC_SIZE_CLASS to a class name to get its size histogram beside the totals. On the backend's /plaintext route this immediately reduced "byte[] is 37% of allocation" to a single number -- 7073173 of 7073834 allocations were exactly 97 bytes -- which is one site and one size rather than a dozen candidates to reason about. Two of those candidates had already been eliminated by reading the code, wrongly. It records before the class-id range check, so a class past the profile table still contributes its sizes: the out-of-range row says a class is missing and this says what size it was. A site with a genuinely variable size shows up as the overflow row rather than crowding out the fixed ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ef47c82bb
ℹ️ 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".
The live-set floor argues about how OFTEN a live heap should be retraced and says nothing about whether the heap it justifies will fit. With growth at 100% and the max at 192MB, a 100MB live set asked for 192MB however little was left, and the high-survival branch then raised its own freeMem/8 ceiling to meet it -- deferring collection past the remaining headroom. Enforcing the floor outside the survival branches, which the previous commit added to cover the 21-24% dead band, gave that the last path it was missing. lowMemoryMode does not cover this. It is set by iOS didReceiveMemoryWarning and by the CI simulation hook and by nothing else, so on Linux -- where the server runs, and the platform this floor was introduced to cut resident memory on -- it is never set and there was no memory bound here at all. The cap floors at the old constant rather than at freeMem/8 directly, and that half is load-bearing: GcSteadyState pins the free-memory reading to 16MB and a bare eighth of that is 2MB, which is the collector running continuously -- the failure this constant has already caused twice in this branch. Checked by evaluating the function over the cases that matter: 100MB live with 64MB free falls from 192MB to 24MB, the pinned-16MB reading lands exactly on the 24MB the trigger had before any of this work, and an ample-memory host is unchanged at 192MB. The floor can be lowered by pressure, never below what was always safe. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two threads allocating the profiled class at different sizes could both read the same zero key, both store, and then both add into whichever size was written last -- one size's row vanishing and the other's count overstated by exactly the amount that vanished. The allocation path is genuinely concurrent, which is why the counts beside the key are already atomics; the key claim was the one part that was not. The table's value is that a reading like 7073173 of 7073834 allocations at exactly 97 bytes can be trusted to mean a single site, which is how the keep-alive buffer copy was localised. A silent merge attacks precisely that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05b6bec486
ℹ️ 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".
The paths filter matched Gc*.java, which covers three of the six tests the matrix runs: LargeArrayGcIntegrationTest, LowMemoryThrottleIntegrationTest and BibopPageFloorIntegrationTest did not match, and neither did CompilerHelper, which supplies the CN1_TEST_EXTRA_CFLAGS every arm depends on to select its marker count. A change to any of them could land without the workflow that exercises them ever running -- and a CompilerHelper regression is not hypothetical: a central fix in this branch reached three of six tests and the gap was found by an invalid-flag probe rather than by CI. Replaced with one entry per test plus the helper, and a note to keep the list and the -Dtest list in sync. The glob is what failed; an explicit list at least fails visibly when the two drift. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
BibopPageFloorIntegrationTest failed on the 1-marker arm -- the default configuration -- and it is this branch's regression rather than a flake. Master has no live-set floor: at survival <= 20% it halves the trigger toward the 24MB constant and stops. This branch raises the trigger to a floor derived from bibopLiveHighWater instead, and cn1BibopTrimFreePool, which hands surplus pages back to the OS, runs ONLY at the end of a sweep. Those two facts latch: the test drops a 196MB live set, the floor still describes the live set that has just gone, the raised trigger puts the next sweep far away, and the decay that would lower the high-water runs once per sweep -- so a high floor buys itself the scarcity of cycles that keeps it high. Across runs of one commit the test returned 6%, 73% and 91% of its pages; bimodal, which is a sweep happening or not, rather than a budget being crept over. The high-water exists to damp the floor on the way UP, where a per-sweep sample understates. It was being applied in both directions, which is the actual defect. Growth is still damped through it; a collapse is now believed immediately, with one minimum-trigger of slack so a sample that merely dips does not slam the floor to the minimum. Evaluated over the cases that matter: a dropped live set falls from a latched 192MB to 48MB on the very next cycle, while a steady live set -- the server's case, and the one the floor was introduced for -- and a growing one are bit for bit what they were. Three local repeats pass at 73% released, which is what the run before this change also returned: the latch does not reproduce on this host, so those runs show the common case is unchanged and nothing more. The arm64 matrix is the environment that discriminates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 409ee59b2d
ℹ️ 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".
The previous commit let a low liveBytes take the floor down immediately, and that is too credulous. liveBytes is not a measurement of the live set, it is the part of it this sweep sampled: pages the major sweep splices out of a partial pool are withheld from the policy numbers on purpose. Both halves of the ratio skip the same pages, so survival is unaffected, but the ABSOLUTE figure understates by whatever was withheld -- and a stable 100MB heap whose objects happen to sit on excluded pages reports a near-zero live set. Collapsing on that would halve the trigger against a heap that never shrank and retrace it every cycle, which is the cost the floor exists to avoid and worse than the latch it replaced: the latch at least erred toward collecting less. The sweep now carries how much it withheld, and a collapse is believed only when that is at most an eighth of what it did measure -- if the pages it could not see are a small part of the heap, the objects hiding on them cannot be many. The threshold is deliberately strict in that direction because the two errors are not symmetric: a wrong yes retraces a live heap every cycle, a wrong no just leaves the decay to walk the high-water down as it did before. Evaluated over all three cases. A real collapse still drops the floor from a latched 192MB to 48MB on the next cycle. A stable 100MB heap sampled at 1MB through an incomplete sweep now holds at 175, 153, 133MB and decays; with the gate removed the same input collapses it to 50MB, which is the reported failure reproduced. Also publishes the allocation-size filter through pthread_once. The flag and the pointer beside it were read and written by every mutator's first allocation, and the visible form of that race is the wrong one for an instrument: a thread seeing the flag set before the pointer is published reads a null name and drops its early samples, losing exactly the allocations a start-up question asks about. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78150e72d8
ℹ️ 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".
…ters atomically Java defines << as two's-complement wraparound. C does not: a signed left shift whose result is not representable is UNDEFINED, so 1L << 63, 1 << 31 and every negative left operand were cases the optimizer was entitled to assume could not happen. The earlier fix here added the JAVA_LONG cast that stopped the translator's bare int literal making 1L << 32 evaluate to 1, and left the undefined shift underneath it. All four LEFT shifts now go through the unsigned type of the same width and convert back, which is Java's answer with no undefined step. The signed RIGHT shifts are deliberately untouched: a negative >> n is implementation-defined rather than undefined, and every compiler this VM builds with defines it as the arithmetic shift Java specifies. Checked by evaluating the real macro text, lifted out of the header rather than retyped, against Java's values under -O2 with -fsanitize=undefined and -fno-sanitize-recover. The old macro is reported as "left shift of 1 by 63 places cannot be represented in type JAVA_LONG" and the new one is clean, so the check is not vacuous. LongShift now runs to 63 and covers negative operands and int shifts, since stopping at 34 exercised the literal bug and nothing about the shift; its doubling reference agrees with the sanitizer harness on every case. Separately, cn1AllocProfClass is atomic like the counters beside it. Several mutators allocating one class wrote the slot at once -- same-value concurrent writes are still a race -- and the atexit report reads it while allocation continues, where a torn value would be dereferenced to print a class name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bb011e84e
ℹ️ 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".
Load-then-store is a read-modify-write: two threads reporting out-of-range classes could both pass the comparison and let the smaller id land last. The number exists to tell a reader how far the table must grow, so understating it sends the next run back with a bound that is still too small -- the same failure the row was added to prevent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f28e2a827c
ℹ️ 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".
The coverage gate added in the previous commit was backwards in both directions, and the review is right about why. It tested how many bytes the sweep withheld from the survival ratio. An ORDINARY sweep withholds nothing -- so it was called complete precisely when it saw least, because it walks retired pages alone and every live object on a partial page is invisible to it. A MAJOR sweep withholds exactly the partial pages it spliced in -- so it was called incomplete when it had in fact just walked the entire heap. A small liveBytes from an ordinary sweep is not a small live set, it is a small sample, and collapsing the floor on it would retrace a stable heap every cycle. Only a major sweep can say anything about the size of the live set, so that is now the gate. And when it speaks it is asked for the WHOLE live set: liveBytes plus what was withheld. That figure was already being computed page by page and thrown away; the survival ratio still excludes those slots exactly as before, because the reason for excluding them -- a spliced partial page is a deep sample that drags the ratio down -- is about the RATIO and was never about the total. Evaluated over the three cases. A stable 100MB live set with 99MB of it on partial pages holds at 175, 153, 133MB across ordinary sweeps instead of collapsing; the major sweep that follows reads the complete 100MB and puts the floor at 192MB; and a real collapse seen by a major sweep still releases the latch to 48MB. Also resolves the assist's A/B flag through pthread_once. It ran on every paced mutator, so with more than one marker several reach their first assist together and a function-local static was read and written by all of them at once -- a data race in exactly the parallel configuration the new workflow exists to exercise. Compiles clean with CN1_GC_MARK_THREADS=4, and an undeclared-identifier probe at the new call site confirms that region is really compiled there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d0209c5de
ℹ️ 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".
Fourth finding on the same twenty lines, and the review is right again. The figure the floor was derived from is not the live set and cannot be made into one here. An ordinary sweep walks retired pages alone, so every live object on a partial page is invisible to it. A major sweep splices the partial pools in, but mutator-owned current pages are never swept at all -- an invariant of the sweep, not an omission -- and the legacy heap above CN1_BIBOP_MAX_OBJECT is not in BiBOP pages and never reaches these counters at all. Three attempts to build a policy on it were each unsound in a different direction: it latched a dropped live set at 192MB and starved the sweeps that hand pages back, which is BibopPageFloorIntegrationTest returning 6% of its pages against 91% on a luckier run of the same commit; then it believed a partial sample as a collapse; then its coverage gate called an ordinary sweep complete precisely when it saw least. Each fix was sound against the case that prompted it and wrong about the next one, which is what building on an unmeasurable quantity looks like from the inside. So the floor goes, along with the decaying high-water, the dead-band clamp, the coverage gate and the sweep plumbing that fed them. Nothing measured is lost: the footprint result this work was for -- 98MB to 38MB on the backend -- is attributed to the MINIMUM, which is a build-time constant and needs no estimate. A deployment that knows its live set is small defines CN1_BIBOP_GC_MIN_TRIGGER_BYTES lower and the shrink path walks the trigger down to it. The trigger policy is now master's, verified by diffing the function against origin/master with comments stripped: the only difference is that the shrink target and the ceiling's lower bound read CN1_BIBOP_GC_MIN_TRIGGER_BYTES instead of the hard CN1_BIBOP_GC_TRIGGER_BYTES, and that constant defaults to the same 24MB, so a stock build behaves exactly as master does. This also removes the 21-24% dead band an earlier round reported: that finding was real, but it was a real defect in a mechanism that should not have existed, and with no floor there is no dead band to fall into. Compiles clean in the default, CN1_GC_CONFORM, CN1_DISABLE_BIBOP and CN1_GC_MARK_THREADS=4 configurations. BibopPageFloor, LowMemoryThrottle and LargeArrayGc pass locally, the first returning 73% of its pages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99ba28bc94
ℹ️ 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".
"Quiet" describes a cycle that ran without the application allocating much, and what counts as much is relative to how much allocation it takes to start a cycle at all. The cutoff was a fixed quarter of the 24MB default, which breaks as soon as the trigger is lower than it: a deployment that sets CN1_BIBOP_GC_MIN_TRIGGER_BYTES to 4MB -- the configuration this branch added and the server uses -- collects every 4 to 6MB, every one of those ordinary allocation-driven cycles falls under a 6MB cutoff, and every single collection then splices every partial pool. That is the O(all pages) sweep issue 5425 removed, handed back to exactly the workload that reported it. The cutoff is now a quarter of the live trigger, capped by the old constant so nothing gets looser than it was. At the 24MB default it is 6MB and unchanged; at a 4MB trigger it is 1MB, so an ordinary crossing is no longer mistaken for silence; at a grown 192MB trigger the constant still caps it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1d7f46014
ℹ️ 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".
The branch assigned CN1_BIBOP_GC_TRIGGER_BYTES unconditionally. On master that could only ever bring the trigger down, because nothing there put it below 24MB. This branch lets a deployment define CN1_BIBOP_GC_MIN_TRIGGER_BYTES lower -- the server uses 4MB -- and then the same assignment RAISES a trigger the low-survival path had already shrunk. An OS memory warning would postpone collection at the moment headroom is scarcest, and lift the pacing cap derived from the trigger along with it. Pinning down to the constant is still the intent and is untouched at the default minimum: GcSteadyState pins the free-memory reading to 16MB to make the per-thread pending table fill and runs with the default, so its trigger is at or above the constant and takes exactly the branch it always did. A 192MB trigger still drops to 24MB; a 4MB one now stays 4MB instead of tripling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lains itself BibopPageFloorIntegrationTest fails here intermittently -- twice in six runs, on the 1-marker arm once and the 4-marker arm once, arm64 only, never x64 -- and the failure is bimodal rather than marginal: 6-7% of a dropped live set's pages handed back on a bad run against 91% on a good one. Bimodal is a major sweep happening or not happening, because cn1BibopTrimFreePool runs only at the end of one. So the question a failing run has to answer is whether any major sweep ran after the live set was dropped and what it spliced, and [MAJOR-SWEEP] and [PAGE-RELEASE] answer exactly that. Guessing has been tried. The failure was attributed to this branch's live-set floor on a plausible mechanism and two data points; the floor has since been removed and the test failed again without it, so that diagnosis was wrong. A second guess -- that Linux was getting MADV_FREE, which does not reduce RSS promptly, where Darwin gets MADV_FREE_REUSABLE -- is also wrong: the release path is guarded, and Linux takes the MADV_DONTNEED branch, which frees immediately. The cause is genuinely unknown, which is the argument for instrumenting rather than theorising a third time. Safe to leave on. The tracer is gated behind getenv and only prints, so no collection decision changes, and it writes to stderr, which runVm deliberately routes to the console instead of merging into stdout -- a merged tracer line can land mid-marker and silently drop a row from the table the assertions parse. Verified locally: the test still passes at 73% with the markers intact, and the run emits 53 [MAJOR-SWEEP] and 4 [PAGE-RELEASE] lines, so the switch is not vacuous. One line per major sweep, which is cadence-limited rather than per page. Remove once the flake is understood. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38d9fd2d48
ℹ️ 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".
Setting CN1_LOG_PAGE_RELEASE in the workflow env handed it to every test in the suite, and the suite includes a heap-integrity verifier whose runs are timing sensitive. Both arm64 arms had passed twice in a row before the tracing went in and both failed on the push that added it -- once with a genuine dangling reference and once with the verifier failing to detect its own injected defect. That is suggestive rather than proven, but the scope was wrong either way: only BibopPageFloorIntegrationTest wanted the counts. The switch now lives on that test's own ProcessBuilder, so exactly one child process gets it and no other test's timing is touched. Verified with CN1_LOG_PAGE_RELEASE explicitly unset in the environment: the run still emits 51 [MAJOR-SWEEP] lines, so the test really is turning it on rather than inheriting it, and both it and GcHeapIntegrityIntegrationTest pass locally. This also gives a clean read on those two arm64 failures. If they persist with the tracing scoped away they are real and were simply never exercised before -- this workflow is the only place GcHeapIntegrityIntegrationTest runs, so it has never executed on arm64 Linux until now. If they go away, the tracing perturbed them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37656fbaee
ℹ️ 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".
…sume protocol Three review findings. Two fixed, one declined in code. The atexit report ranked the live counters and zeroed them as it printed. Exit handlers do not stop the other threads, so a class could be printed, keep allocating, and be selected again for a second row, and its bytes and count were read at different instants and need not have described the same allocations. Everything is now copied once and the ranking consumes the copy; the size histogram had the same shape and gets the same treatment. CN1_BIBOP_HEAP_GROWTH_PERCENT was left defined and unused when the live-set floor was removed -- cn1BibopTriggerFloor was its only reader. A knob that is documented, settable and inert is worse than no knob, because a footprint experiment that sets it reads its result as evidence about a value that never applied. Removed, along with the proportional-live-set rationale above it, which described a rule this file no longer implements. The paragraph now says what the constant actually is: a floor a deployment CHOOSES, defaulting to the old value, and why it is not inferred from the live set -- because this collector cannot measure one, which three failed attempts established. Declined, with the reasoning in the code: an atomic block-check-and-reactivate in cn1PacingPark. The window is real and the report describes it correctly, but it is CN1_RESUME_THREAD's window -- the collector sets threadBlockedByGC, waits for threadActive to clear and never re-validates, and every native boundary in the VM resumes with the same unsynchronised pair. Closing it in one function would leave the same window open at thousands of others while making this site disagree with the protocol the collector implements, which is the shape of the defects this area has already produced. It needs a collector-side acknowledgement applied everywhere at once, which is a VM thread-protocol change and not part of a pacing fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4c53542a0
ℹ️ 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".
Comment on lines
+12355
to
+12358
| cn1AllocProfSnapBytes[i] = | ||
| atomic_load_explicit(&cn1AllocProfBytes[i], memory_order_relaxed); | ||
| cn1AllocProfSnapCount[i] = | ||
| atomic_load_explicit(&cn1AllocProfCount[i], memory_order_relaxed); |
There was a problem hiding this comment.
When another mutator allocates during this atexit handler, these independent loads still do not form a consistent snapshot: the reporter can read bytes, the mutator can increment both counters, and then the reporter can read the newer count, producing an incorrect average and potentially ranking classes from different instants. The fresh evidence in this revision is that the replacement snapshot still reads each counter separately while explicitly acknowledging that other threads remain active; quiesce recorders or synchronize/capture each byte-count pair atomically before ranking.
Useful? React with 👍 / 👎.
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
1 participant
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.
Three separable changes, each with its rationale in the code rather than here.
1.
1L << nwas computed as a 32-bit shiftBC_LSHL_EXPR/BC_LSHR_EXPRmasked the shift count to 6 bits but left the leftoperand alone, and the translator emits a long constant as a bare C literal. So
LCONST_1arrived as anint:Shifting a long variable was always correct, which is how it survived.
BC_LUSHR_EXPRalready cast, so this was fixed once for the unsigned shift andnot carried to its siblings.
LongShiftchecks all three forms against areference built by repeated doubling.
2. Heap goal sized against the live set, not a constant
CN1_BIBOP_GC_TRIGGER_BYTESwas the floor outright, so a process holding almostnothing live still accumulated 24MB of garbage before collecting. Resident memory
tracked the trigger and nothing else (backend,
/plaintext, 64 connections):Throughput and p99 across that sweep were flat inside noise, so the floor bought
footprint and no speed. The floor is now live-set + growth%, clamped to a 4MB
minimum, at all three clamp sites. An app with a real live set gets a larger
floor than the old constant. Measured after: 98MB -> 38MB loaded RSS, with
throughput and p50 improving.
Also adds
cn1GcMutatorAssist(a thread parked on the run-ahead cap marks abatch instead of sleeping). It registers in
gcMarkActiveWorkersbeforereleasing the worklist mutex so mark termination cannot fire while it holds a
batch, and assists only the parallel path. It is inert until the mark pool is
enabled.
3. Re-test parallel marking on arm64
gcMarkResolveThreadCountforces one marker behind#elif 1. The comment readsas a standing verdict that arm64 corrupts the heap with the pool on. The history
is narrower - inside #5327:
The conclusion was drawn before every mechanism that makes concurrent marking
sound, the SATB barrier included. A different arm64 heap corruptor was found on
Jul 6. Parallel marking was never re-tested afterwards, and
gcMarkDrainParallel/gcMarkObject/gcMarkFlushLocal/gcMarkWorklistPushhave all been reworked since.Cost of leaving it: on
GcPause(one mutator, 20M short-lived objects, 4096-nodelive set) the worst pause is 2.2-3.3s with one marker, 0.3-0.9s with four,
against Go's 20ms on the identical loop. Median and p99 already match Go.
Not reproduced locally:
GcPauseran clean with four markers on an aarch64guest, and a multi-threaded stress ran clean too - but that stress also passes
with the SATB barrier compiled out, so it is not a valid detector and is
deliberately not included here.
No defaults change. The pool is enabled for the new workflow only, via the
CN1_TEST_EXTRA_CFLAGShook. Matrix: 1 marker arm64 (control), 4 markers arm64(the question), 4 markers x64 (attribution).
Verification
GC suite (
GcHeapIntegrity,GcOverflowSpiral,GcUncooperativeThread,LargeArrayGc,LowMemoryThrottle,BibopPageFloor) 6/6 locally on thisbranch. Copyright and control-character gates pass.
🤖 Generated with Claude Code