Give ParparVM real weak and soft references - #5732
Conversation
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2445f3ce13
ℹ️ 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".
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Cloudflare Preview
|
|
Compared 166 screenshots: 166 matched. |
|
Compared 166 screenshots: 166 matched. |
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 181 screenshots: 181 matched. |
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90c056b415
ℹ️ 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".
|
Compared 149 screenshots: 149 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: 1682c86bb8
ℹ️ 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".
|
Compared 217 screenshots: 217 matched. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 847c9970e1
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fbcce3f84
ℹ️ 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".
|
Compared 160 screenshots: 160 matched. Benchmark Results
Detailed Performance Metrics
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98cdecee3d
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71b6e35152
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d4f5c9d69
ℹ️ 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".
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8b15f96a9
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69528f8004
ℹ️ 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".
|
Compared 144 screenshots: 144 matched. |
|
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: 1be524263f
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94b90f93be
ℹ️ 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".
94b90f9 to
351c7ac
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 351c7ac472
ℹ️ 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".
…ped log entry Two accepted review findings, both leftovers of earlier fixes on this branch rather than new ground. HALF A RACE WAS FIXED. Making the referent's accesses atomic converted the getter and the setter's own store but left CN1_SATB_DELETE, emitted immediately before that store, reading the same field through a plain JAVA_OBJECT volatile*. The collector writes JAVA_NULL there atomically from cn1GcProcessReferences, so the pair stayed a mixed atomic/non-atomic access -- the exact defect the earlier change set out to remove. CN1_SATB_DELETE_REF is the deletion barrier with an atomic load, used for this field only; every other field keeps the generic macro, which is correct for them because nothing else writes them concurrently. A DROPPED LOG ENTRY VOIDS THE CLEAR PASS'S EVIDENCE. cn1SatbEnqueue discards a reference when its stack cannot grow. The comment there argues that is survivable, and for an ordinary STORE it is -- "only re-opens the original race". It is not survivable for a referent Reference.get() has already handed to a mutator: the enqueue was the only record that it escaped, sub-pass B decides purely on mark state, and the final take stays empty so nothing re-opens, leaving the sweep free to reclaim an object a thread is holding. Registering the load in the termination handshake does not help here; it delays the take, it does not stop a drop. Nothing on that path can allocate its way out, so the pass now snapshots the drop count at cycle start and declines to clear at all if it moved. That costs one cycle of reclaim in a process that is already out of memory, and it deliberately does not disable the emergency soft-drop, which clears at DISCOVERY and never touches the log. Gates: run-gc-verify.sh green (RefPolicy clean over 71 verify passes, both fault-injection self-tests firing), run-gauntlet.sh green with all eleven tortures bit-identical and both GC stop modes; the emergency drop still fires under injected allocation failure and the driver's weak, alias and cache assertions are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two accepted review findings, and one pre-existing bug found next to the first. THE DROP COUNTER GUARDED HALF THE LOSS PATH. The previous commit invalidated reference clearing when cn1SatbEnqueue discarded an entry, but cn1SatbTake discards too: it resets gcSatbTop unconditionally and, when its scratch buffer cannot be grown, reports the batch as EMPTY. Entries that were logged successfully are then thrown away and the collector reads "nothing slipped in", which is exactly the signal that lets termination finish. To the referent that gets swept a lost enqueue and a lost batch are the same event, so the take now counts as a drop as well. PRE-EXISTING, in the same function and worth its own paragraph: realloc's result was assigned straight back over scratch, so a failure lost the buffer that was already there, and scratchCap was advanced whether or not the growth succeeded. After a single failure every later take saw n <= scratchCap, skipped the realloc, found scratch NULL and returned 0 -- the barrier logging into a stack nothing would ever drain again, permanently and silently. It now grows through a temporary and advances the cap only on success. -DCN1_DISABLE_SATB WAS UNSOUND, NOT MERELY SLOWER. That arm compiles out the load barrier that makes handing a weak referent to a mutator safe, but reference processing is not part of SATB and kept running: a thread released after its stack scan could load a referent, have the field cleared underneath it and the object swept before it could use the pointer. Reference processing now switches off with the barrier, degrading to the behaviour that preceded this feature -- referents traced strongly and never cleared -- which is the right fallback for an escape hatch and keeps the arm measuring the barrier's cost rather than a different collector. Verified rather than assumed: WEAK_DEAD_CLEARED reads 0/256 in that arm against 255/256 by default. Gates: run-gc-verify.sh green (RefPolicy clean over 70 verify passes, both fault-injection self-tests firing), run-gauntlet.sh green with all eleven tortures bit-identical and both GC stop modes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two accepted review findings. One is a heap overflow this branch introduced an hour ago; the other says the gate that was supposed to catch such things could not see the field in question at all. THE OVERFLOW WAS MINE. Growing cn1SatbTake's scratch buffer through a temporary keeps the old allocation when realloc fails -- which was the point, the previous shape leaked it -- and therefore leaves scratch NON-NULL and SMALLER than n. The guard still tested only scratch != 0, so the memcpy wrote n entries into an allocation sized for fewer: heap corruption written by the collector under memory pressure, strictly worse than the leak it replaced. The buffer is usable only if it exists AND scratchCap >= n. THE VERIFIER COULD NOT SEE THE REFERENT. cn1GcVerifyHeap walks survivors through the generated mark functions and relies on every reference field reaching gcMarkObject, whose verify branch classifies it. Suppressing that call for the referent -- the very thing that makes the edge weak -- also took the referent out of the verifier's reach, so a live Reference holding a pointer into reclaimed memory passed with violations=0. Every "clean over N verify passes" recorded on this branch before now was silent about the referent specifically, which is the one thing this work risks. cn1GcDiscoverReference now routes it to cn1GcVerifyChild, ahead of the ageing and the dedupe: a verify walk is not a collection cycle, and letting it age references or consume the dedupe stamp would corrupt what the next real cycle reads and drop exactly the repeat visits a whole-heap walk produces. PROVEN, not assumed. CN1_GC_FAULT=refnoclear leaves a dead referent in its field instead of clearing it, and the verifier reports DANGLING REFERENCE; clean it reports none. run-gc-verify.sh gains self-test3 so this cannot go quietly blind again. Note the obvious-looking fault is the wrong one and was tried first: clearing MORE references than liveness warrants only produces extra nulls, which are safe, and it reported violations=0 -- stopping there would have "confirmed" the hook while proving nothing. The dangling direction is clearing LESS. The fault's use is inside CN1_GC_VERIFY because the cn1GcFault* family is declared there. Unguarded it broke every ordinary build while the verifier build -- the one configuration in which the symbol exists -- kept passing, so all three shapes are now built explicitly: plain, CN1_GC_CONFORM and CN1_GC_VERIFY. Gates: run-gc-verify.sh green with all three self-tests firing, run-gauntlet.sh green with all eleven tortures bit-identical and both GC stop modes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Accepted review finding, and a repeat of a mistake this branch already corrected once. The drop-counter fallback returned without clearing, which leaves every discovered referent unmarked AND its field non-null -- so the sweep frees objects that live References still point at. That is the dangling read the pass exists to prevent, produced by the code meant to prevent it. "Skip the clear" is not "keep the referent alive". Nothing else marks a weak referent; that is what makes the edge weak. The note on the unrecorded-discovery path says exactly this, and the same confusion reappeared here. The fallback now marks every discovered referent and drains before returning. Gates: run-gc-verify.sh green with all three self-tests firing, run-gauntlet.sh green with all eleven tortures bit-identical and both GC stop modes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The barrier registers in the SATB termination handshake, which is two seq_cst
read-modify-writes on every Reference.get() and cannot be skipped soundly -- any
flag sampled before registering can go stale in the window the registration
exists to close. Whether that price is worth paying is a question for a number,
not an intuition, so this adds the means to get one.
-DCN1_REF_NO_LOAD_BARRIER compiles the registration and the enqueue out, leaving
the load and the touch stamp. It is UNSOUND and is not a shipping configuration;
it is the arm the cost is measured against.
cn1RefGets counts get() calls, and [GCREF-TOTAL] prints the run total at exit.
The per-cycle [GCREF] line cannot answer "how often does this workload call
get()", because it only prints when a collection happens -- so a get()-heavy but
allocation-light phase, which is exactly the shape that costs the barrier most,
leaves the last line stranded early in the run. Costing the barrier off that
number understates the calls and overstates the nanoseconds: it read 153,408 for
a run that made 6,155,728.
Measured on this host, RefPolicy, nine interleaved reps, both arms
-DCN1_GC_CONFORM -flto=thin:
get-dominated 64 keys x 64B, 6M reads, no churn 6,155,728 gets
+0.07% median, +0.35% floor -> +0.45 ns median, +2.14 ns floor per get()
image-cache 512 keys x 32KB, 400k reads, churn 556,416 gets
+0.10% median, -0.30% floor -> unmeasurable at this call volume
So the barrier costs roughly half a nanosecond to two nanoseconds per call, and
0.35% of wall time only at 1.6 MILLION get() calls per second. An image-heavy
screen redrawing fifty encoded images at 60fps calls it three thousand times a
second, some three orders of magnitude below the rate at which it becomes
visible.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five accepted review findings. -DCN1_DISABLE_BIBOP DID NOT LINK. bibopGcEpoch is defined inside cn1_globals.m's #ifndef CN1_DISABLE_BIBOP block, and the load barrier named it unguarded, so a supported A/B and fallback configuration failed with an undefined _bibopGcEpoch. Confirmed by building it rather than by reading guards. The epoch is only a filter -- dropping it enqueues referents that would have been skipped, which is more work and never less safety -- so the disabled build compares against a value the mark word cannot equal and the barrier keeps its single comparison. DROP RECOVERY DID NOT REACH A FIXPOINT. The same defect already fixed in sub-pass A, reappearing in the recovery loop added a commit earlier: marking a retained referent traces it, and an object kept alive only that way can itself hold references whose mark functions register after the loop has passed them. THE CAPPED SATB EXIT SKIPPED REFERENCES ENTIRELY. The CN1_SATB_MAX_REOPENS branch drains twice on its way out, and those drains can newly mark an object whose graph contains a Reference. It was the one exit that left without a reference pass, so a reachable reference kept an unmarked referent the sweep then freed. THE DROP CHECK COULD NOT SEE A DROP THAT HAD NOT HAPPENED YET. A get() starting after the pre-loop check loads an unmarked referent, and a failed enqueue moves the counter only once clearing has already decided it was safe. The pass now records what each entry cleared, quiesces -- every getter registers for the duration of its load, so an in-flight count of zero means every getter that overlapped has finished and published its drop -- and re-reads the counter, marking what it cleared if it moved. The stores are not undone and need not be: a cleared reference answering null is legal, the object being freed under a mutator is not. THE EMERGENCY PATH HAD NOWHERE TO RECORD. It clears at discovery precisely because the list could not grow, so the recovery above had nothing to consult and a racing get() with a failed enqueue would have been handed a freed pointer. It now remembers what it cleared in a fixed preallocated array -- allocation being the one thing unavailable there -- and REFUSES TO CLEAR when that is full, marking instead: memory the emergency wanted back is retained, which is worse than clearing and far better than a dangling read. Rebased onto master, which landed the parallel-mark collector work (#5717) in the same two files; the conflict was additive on both sides and the resolved tree was compiled before the rebase continued. Gates: run-gc-verify.sh green with all three self-tests firing, run-gauntlet.sh green with all eleven tortures bit-identical and both GC stop modes, and all five build shapes compile -- plain, CN1_GC_CONFORM, CN1_DISABLE_BIBOP, CN1_DISABLE_SATB and CN1_GC_VERIFY. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The load barrier needed bibopGcEpoch, which was defined inside cn1_globals.m's #ifndef CN1_DISABLE_BIBOP block, so that configuration failed to link. The first fix taught the barrier to compare against a sentinel when BiBOP was compiled out -- a configuration branch on a hot path to accommodate a symbol that had no reason to be conditional. The epoch is a plain mirror of currentGcMarkValue for mutator-side reads. Nothing about it belongs to the page heap; it sat inside that guard by accident of placement. Hoisting it out removes the macro entirely and returns the barrier to one comparison with no configuration in it. Keeping the arm alive rather than dropping it costs nothing now and keeps vm/CLAUDE.md honest: -DCN1_DISABLE_BIBOP is listed there as an ablation, and an ablation that does not link is a trap for whoever reaches for it. With BiBOP off nothing advances the epoch, so the barrier enqueues referents it would otherwise have skipped -- the epoch is a filter, and a stale one only ever declines to skip. Six build shapes verified: plain, CN1_GC_CONFORM, CN1_DISABLE_BIBOP, CN1_DISABLE_SATB, CN1_GC_VERIFY, and CN1_DISABLE_BIBOP with CN1_GC_CONFORM together. Gates: run-gc-verify.sh green with all three self-tests firing, run-gauntlet.sh green with all eleven tortures bit-identical and both GC stop modes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tale Four accepted review findings; three are defects in the recovery paths added a commit earlier. EMERGENCY SLOT RESERVATION WAS RACY. cn1RefEmergencyTop++ runs after cn1RefMutex has been released, and mark functions run on however many workers gcMarkDrainParallel is using -- so increments were lost and two workers could publish into one slot. An emergency clear that goes unrecorded is exactly the case the drop recovery cannot repair, leaving the sweep free to take the referent under a racing get(). The index is now an atomic reserve-then-publish, and a reservation that cannot be satisfied means the clear does not happen at all. THE EARLY DROP FALLBACK DID NOT RETAIN EMERGENCY CLEARS. It returns before the post-clear recovery, and a referent cleared by the emergency path is gone from its field -- it exists only in the recovery array. Retaining cn1RefDiscovered alone therefore left those objects to be swept under a getter that had already been handed one. THE POST-CLEAR RECOVERY DRAINED ONCE. The same fixpoint defect as the other two recovery loops: a referent brought back can hold a further Reference whose mark function registers during the drain, after the loops have run. It now iterates, and retains anything the drain discovered rather than clearing it -- by that point the pass is past where clearing is safe. THE STALE EPOCH WAS NOT HARMLESS. The previous commit hoisted bibopGcEpoch out of the BiBOP guard so -DCN1_DISABLE_BIBOP would link, and its comment claimed a frozen mirror cost only some extra enqueues. It costs convergence: CN1_SATB_REF_KEEP skips a referent whose mark equals the epoch, and against a mirror frozen at 1 that matches nothing from the second collection onward -- the unfiltered shape already measured on this branch to put over 10,000 entries a cycle into the log and reach CN1_SATB_MAX_REOPENS every cycle. A filter that silently stops filtering is a cliff, not a rounding error. The epoch is now published by codenameOneGCMark, which every cycle passes through whether or not the page heap is compiled in, leaving one writer instead of two. Verified rather than argued: with -DCN1_DISABLE_BIBOP -DCN1_GC_CONFORM the termination loop reports passes=1 on all 32 cycles. Gates: run-gc-verify.sh green with all three self-tests firing, run-gauntlet.sh green with all eleven tortures bit-identical and both GC stop modes; plain, CN1_GC_CONFORM, CN1_DISABLE_BIBOP, CN1_DISABLE_SATB and CN1_GC_VERIFY all build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Accepted review finding, and the defect was in a comment before it was in the code. The capped path -- reached when a mutator storms the barrier past CN1_SATB_MAX_REOPENS -- called cn1GcProcessReferences on the way out, justified by "the barrier is already down on this path, so a get() racing this pass cannot log". That sentence is false. cn1SatbBulkBegin answers gcSatbActive OR gcSatbTerminating, and gcSatbTerminating stays raised until after the loop, so a getter there registers, enqueues successfully, and lands its entry in a log this path never takes again. Clearing on that basis can free an object a getter is in the middle of being handed. Rather than correct the reasoning and keep clearing, the path now RETAINS. Nothing is cleared, so nothing can dangle however the race falls, and no argument about flag ordering is load-bearing. The cost is one cycle of reclaim on a path whose own comment records reaching it 0-4 times against a cap of 32. cn1GcRetainAllReferences factors out the retain-to-fixpoint walk that the drop fallback and this path both need -- written three times by hand across this branch, and the fixpoint was missing from two of them. It covers discovered referents, referents already cleared this cycle, and the emergency array, and iterates because marking a retained referent can reach a Reference whose mark function registers during the drain. Gates: run-gc-verify.sh green with all three self-tests firing, run-gauntlet.sh green with all eleven tortures bit-identical and both GC stop modes; plain, CN1_GC_CONFORM, CN1_DISABLE_BIBOP, CN1_DISABLE_SATB and CN1_GC_VERIFY all build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Accepted review finding, and the last unguarded corner of the same take-side loss. cn1SatbTake reports an empty batch for two different reasons: the log really was empty, or its scratch buffer could not grow and the entries were thrown away. It records the second case in cn1SatbDrops -- but the final catch in codenameOneGCMark runs AFTER cn1GcProcessReferences made its last drop check, and `if(n == 0) break` reads an empty batch as "the mark is closed". Nothing looked at the new counter value, so a Reference.get() that logged its referent successfully, and then had that batch discarded here, kept a pointer the following sweep freed. The break now retains when the counter has moved since the cycle began. Retaining rather than another clear pass, because the barrier is coming down at that point and there is no sound basis left for calling anything dead. Gates: run-gc-verify.sh green with all three self-tests firing, run-gauntlet.sh green with all eleven tortures bit-identical and both GC stop modes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Accepted review finding, and it catches this change breaking a rule stated in capitals a few lines above it. The drop recovery added by the previous commit ran cn1GcRetainAllReferences after gcSatbActive had already been cleared. Retaining MARKS referents that were white and gcMarkDrain then scans them, so those objects are grey at a moment when no barrier is watching -- and a mutator moving an old child out of one of them in that window logs nothing on either side, leaving the child unmarked, not fresh, and reachable only from a grey object the sweep will not protect. That is exactly the hazard the trial-clear comment above describes, and the reason the clear is a TRIAL rather than the end of the mark. Recovery is one more thing that can turn out to mark something new, so it now behaves like the catch it sits next to: re-arm gcSatbActive, retain, and go round the fixpoint again rather than draining underneath a lowered barrier. At the reopen cap it falls through to the same weaker invariant the cap already documents. recoveredDrops is what makes that terminate. Comparing against cn1RefDropsAtCycleStart would re-trigger the recovery on every pass, because that baseline never moves once a drop has happened -- the loop would then re-arm and retain until it hit CN1_SATB_MAX_REOPENS every time a single drop occurred. Recording the count each recovery consumed means another pass happens only if a NEW drop has since been recorded. Gates: run-gc-verify.sh green with all three self-tests firing, run-gauntlet.sh green with all eleven tortures bit-identical and both GC stop modes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Accepted review finding. cn1GcRetainAllReferences decided it had converged by comparing cn1RefDiscoveredTop alone, but discovery has two ways to make progress and that counter only sees one of them. During an emergency cycle the drain can reach another soft reference whose referent cn1GcDiscoverReference clears on the spot -- and that path deliberately does NOT append to the discovery list, because it exists precisely for the case where the list could not grow. It fills a cn1RefEmergencyCleared slot instead. So the length could be unchanged while a fresh recovery slot had just been written, the loop would read that as "nothing new" and return, and the referent recorded in that slot would never be marked. Reached from the CN1_SATB_MAX_REOPENS fallback, that leaves the sweep free to take a referent a concurrent Reference.get() is being handed. The loop now watches cn1RefEmergencyTop as well and continues while either counter moves. Gates: run-gc-verify.sh green with all three self-tests firing, run-gauntlet.sh green with all eleven tortures bit-identical and both GC stop modes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… reference Accepted review finding, and it is a livelock inside the path that exists to prevent one. The emergency clear refuses to act on a reference whose stamp reads CN1_REF_TOUCHED and marks the referent instead, which is right for this cycle: a mutator may be holding it. But the ageing loop walks cn1RefDiscovered, and a reference reaching that branch is there precisely BECAUSE it could not be recorded in that list -- so nothing ever resets the stamp. It stays TOUCHED for the life of the process, every later emergency cycle refuses to clear the same referent however long ago it was last read, and if that retained memory is what is blocking the allocation then codenameOneGcMalloc's retry loop never makes progress. The emergency was raised by an allocation failure; this is the failure mode it was added to break. Consuming the stamp after marking closes it. The referent has just been retained, which is the whole of what the stamp was protecting, so the next cycle is free to clear it if nothing reads it again -- and if something does, that get() stamps it afresh. Compare-exchange rather than a plain store so a get() landing between the read and the reset is not silently discarded. Gates: run-gc-verify.sh green with all three self-tests firing, run-gauntlet.sh green with all eleven tortures bit-identical and both GC stop modes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…er at the cap Two accepted review findings. The first is a hole in this branch's own plan that every gate here was structurally unable to see. SOFTREFERENCE WAS IMPLEMENTED AND UNUSABLE. It existed only in vm/JavaAPI, but maven/java-runtime builds the SUPPORTED API SURFACE from Ports/CLDC11/src and BytecodeComplianceMojo indexes that artifact as the set of types an application may touch. Application code would therefore resolve SoftReference against the host JDK and then be rejected as forbidden API -- a confusing way to discover that a shipped feature was never exported. The plan for this work said to add the CLDC11 stub and it was never done; nothing caught it because every gate here compiles vm/JavaAPI directly, which is exactly the path that bypasses the compliance surface. RETENTION TRACED WITH THE BARRIER DOWN AT THE REOPEN CAP. The drop-recovery path was corrected for this a commit ago and the identical defect was left at CN1_SATB_MAX_REOPENS: cn1GcRetainAllReferences marks referents that were white and gcMarkDrain then traces them, so with gcSatbActive already lowered those objects are grey and unwatched, and a mutator moving an old child out of one logs nothing on either side. Retention now runs BEFORE the barrier is lowered, so everything known at that point is traced under it. What remains after the final take -- retaining references that last drain discovered -- does trace with the barrier down, and that is said plainly at the call rather than glossed: it is the same weaker invariant the cap already relies on for the gcMarkDrain immediately above it, not a new exposure, on a path whose own comment measures 0-4 reopens against a cap of 32. Gates: run-gc-verify.sh green with all three self-tests firing, run-gauntlet.sh green with all eleven tortures bit-identical and both GC stop modes; the CLDC11 java.lang.ref package compiles. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… going stale Two accepted review findings, both introduced by this branch in the last few commits. A NATIVE CRASH ON THE EMERGENCY PATH. Deciding whether an unrecorded referent was already live read its mark word directly, without the cn1ConservativeResolve guard the clear pass carries. Under conservative roots a dead Reference kept alive by a stale native-stack word can hold a referent swept in an earlier cycle, whose memory is now unmapped -- and gcMarkObject's own comment says reading even the mark word of such a pointer faults. So a collection under memory pressure, which is the only situation that reaches this path, could take the process down. The referent is now resolved before any dereference, and an unresolvable one is treated as neither live nor clearable: nothing can validate it, and it is either garbage or an object allocated after this cycle's extent snapshot that the grace rule keeps anyway. SELF-TEST3 COULD PASS ON A STALE BINARY. It built RefPolicy-verify only when the file was absent, while every driver above it rebuilds unconditionally -- so a regression in the reference-verifier hook could still produce a green self-test by running an old binary. That is the same "a gate that cannot fail" problem this self-test was added to solve, reintroduced in the way the self-test itself is built. It now rebuilds every invocation. Gates: run-gc-verify.sh green with all three self-tests firing, run-gauntlet.sh green with all eleven tortures bit-identical and both GC stop modes; CN1_GC_CONFORM and CN1_DISABLE_BIBOP both build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…l loudly Two accepted review findings, and both are the same mistake: a fix applied to one copy of something and not to its duplicate. THE EARLY DROP RECOVERY WAS A SECOND COPY of the retain-to-fixpoint walk. The shared helper was taught that discovery advances TWO counters -- the emergency path clears a referent into cn1RefEmergencyCleared without touching cn1RefDiscoveredTop, because it exists precisely for when that list cannot grow -- and this copy was left comparing the list length alone. It therefore read "nothing new" over a freshly written recovery slot and returned without marking what that slot held, so a racing get() whose enqueue had also failed could be handed an object the sweep then freed. The copy is deleted and the helper called; patching it would have left a third place to drift. THE SELF-TEST REBUILD STILL ACCEPTED STALE CODE. Rebuilding unconditionally was one of three things needed and the only one done. translate-and-build.sh replaces its output only after the final compiler run succeeds, so a FAILED rebuild leaves the previous binary in place; and `|| true` discarded the status, so the -x test below accepted that binary. The self-test could then run old code and report green -- the "gate that cannot fail" problem it exists to prevent, for the second time in how it is built. It now removes the output first, checks the build status, and fails the gate. Gates: run-gc-verify.sh green with all three self-tests firing, run-gauntlet.sh green with all eleven tortures bit-identical and both GC stop modes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e last copy Two accepted review findings. DROP RECOVERY DEADLOCKED THE ALLOCATOR. Retaining everything on an SATB loss is the safe reflex and it is wrong under the emergency budget: that budget is raised by an allocation FAILURE, sustained exhaustion is exactly what keeps the SATB stack from growing, and if soft-referenced data is what exhausted memory then every retry cycle loses a batch, retains the same data, and codenameOneGcMalloc spins on collections that free nothing. The emergency exists to break that deadlock and this recovery was reinstating it. The distinction that resolves it: the emergency decision never depended on the log. "Drop every soft referent" is a policy choice taken from the memory budget at cycle start, not an inference from liveness, so a lost log entry does not invalidate it. What the log would have protected is a referent a mutator is mid-read of -- and the touch stamp records that independently and allocation-free, which is the signal sub-pass A already trusts. So cn1GcRecoverAfterDrop retains touched referents, still clears condemned soft ones, and retains everything else. The residual, stated at the code, is a get() that loaded a soft referent and was descheduled before stamping: the window the emergency path already accepts, against an allocator that otherwise cannot progress. A THIRD COPY OF THE FIXPOINT. The previous commit deleted one hand-written copy of the retain walk and said a third would drift again; there already was one, in the post-clear recovery, and it was not looked for. It had drifted the same way -- watching cn1RefDiscoveredTop while the emergency path advances cn1RefEmergencyTop -- so it read "nothing new" over a freshly written recovery slot. There is now one implementation and no copies. Gates: run-gc-verify.sh green with all three self-tests firing, run-gauntlet.sh green with all eleven tortures bit-identical and both GC stop modes; the emergency still reclaims under injected allocation failure (retained=0 at softBudget=-1). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…referents Two accepted review findings, both on the recovery helper added one commit ago. THE TOUCH STAMP WAS READ TOO EARLY. cn1SatbDrops becomes visible when an enqueue FAILS, and that happens before the accessor reaches its stamp -- so entering recovery on that signal and reading touchAgeField immediately can see "not touched" for a getter that is mid-load and about to stamp. Clearing on that reading hands the sweep an object the getter is being given, and this path deliberately does not record clearedReferent, so nothing downstream could recover it. The helper now quiesces first. Every getter registers across its whole load, the stamp included, so an in-flight count of zero means every getter that overlapped has finished and published -- which is what makes the stamp evidence rather than a race. A STRONGLY REACHABLE REFERENT COULD BE CLEARED. The other emergency path consults the mark word before clearing, because a SoftReference may only be cleared when its referent is SOFTLY reachable; this helper omitted that test and would clear an application's cache entry for an object it also holds in an ordinary field. It now applies the same current-epoch-or-fresh check. Both are the same shape: new code on one path missing a guard its sibling already carries. Gates: run-gc-verify.sh green with all three self-tests firing, run-gauntlet.sh green with all eleven tortures bit-identical and both GC stop modes; the emergency still reclaims under injected allocation failure (retained=0 at softBudget=-1). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two accepted review findings, both on the helper this branch added three commits ago, and the first is the previous fix creating the next defect. THE MARK-WORD READ HAD NO RESOLVE GUARD. It was added one commit back to stop the recovery clearing strongly reachable referents -- and it dereferences the referent without cn1ConservativeResolve, which the clear pass and the unrecorded emergency path both apply before the same access. A Reference kept alive by a stale native-stack word can hold a referent swept in an earlier cycle whose memory is unmapped, and reading even its mark word faults. So a fix for a correctness gap introduced a native crash, in code whose two siblings show the right pattern. THE QUIESCE CANNOT HOLD. It drains the getters in flight when recovery starts and cannot stop a new one registering immediately afterwards, while gcSatbActive and gcSatbTerminating are both still raised. That getter can load the referent, have its enqueue fail, and be descheduled before stamping, so this loop reads the old stamp and clears a field whose referent is being handed out -- with nothing saved, no later pass could mark it. The clear is now recorded. The post-clear recovery therefore marks it whenever a drop is visible, which gives the emergency's reclaim back in exactly the case where safety is uncertain, and keeps it in the common case where no further drop occurs. That is the right way round: reclaim is the goal, not being right. This is the fourth consecutive round on this one helper, each fix producing the next finding. The oscillation is between two poles this path genuinely sits between -- reclaiming under memory pressure, and staying safe against an unreliable log -- which is an argument about the shape of the design rather than about any of the individual fixes. Gates: run-gc-verify.sh green with all three self-tests firing, run-gauntlet.sh green with all eleven tortures bit-identical and both GC stop modes. The first gauntlet run was killed at IbpTest and re-run from the start; a partial gauntlet is not a result. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Localises and fixes an intermittent CI regression, and removes a cost every application was paying for a feature most of them never use. BibopPageFloorIntegrationTest began failing on the 4-marker arm64 job -- five times against master's five clean samples -- reporting its pages as released while phys_footprint stayed at ~231MB. The workload contains no Reference at all, which is why it took so long to believe: nothing in this feature can run there. Except one thing did. cn1GcProcessReferences ran on every outer termination pass regardless, and once the drop re-check landed it called cn1SatbBulkQuiesce() unconditionally -- which is not free. It spins in usleep(50) while any BULK ARRAY COPY is in flight, and that app copies object arrays, so the collector could stall inside the termination loop on behalf of a feature the program does not use. It fits the shape that never made sense otherwise: intermittent, only under the configuration where four markers and bulk copies actually overlap, and reporting release while the footprint does not move. Dispatching the workflow on wip/refbisect-base -- master plus only the first commit, weak references with none of this machinery -- passes the same job, which is what localised it to the later commits rather than to the feature. The fix is what should have been there from the start: with nothing discovered and nothing in the emergency array, the pass returns immediately and touches nothing. An earlier hypothesis for this failure was WRONG and is recorded so it is not retried: cn1RefBeginCycle's headroom probe was suspected of adding a per-cycle footprint syscall, but the test sets only CN1_LOG_PAGE_RELEASE, never CN1_SIMULATE_PROC_MEMORY_LIMIT, and cn1SimulatedProcLimitBytes caches -- so on Linux that path returns -1 from a cached atomic read. Gates: run-gc-verify.sh green with all three self-tests firing, run-gauntlet.sh green with all eleven tortures bit-identical and both GC stop modes; RefPolicy's weak, alias and cache assertions unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cn1GcDiscoverReference now defers to gcMarkObject while a thread is inside its own nursery minor collection. cn1PromoteDrain runs the generated mark functions with nurseryPromoting raised, and gcMarkObject's promotion branch is what moves a referenced object out of the block being recycled -- routing the referent past it meant a surviving WeakReference could be promoted alone and left pointing into a reclaimed block. Nothing clears on that path, so the edge costs a promotion and nothing else. The nursery arm did not compile at all, here or on master: nativeMethods' bulk barrier calls cn1SatbBulkBegin unconditionally while the prototype sat in the #else of the CN1_NURSERY split, which clang rejects as an implicit declaration under C99. The load barrier added a second instance. Declared beside the deletion barrier instead, where the callers are. Two gates that could not fail: - RefPolicy printed WEAK_LIVE_KEPT and exited 0 whatever it said. A referent cleared while still strongly reachable is heap-SAFE -- null dangles nothing -- so run-gc-verify cannot see it either, and the checksum reads live[] directly. Both advertised validation paths could stay green while WeakReference silently emptied every cache built on it. It is an assertion now. - ab-refs.sh ignored the return code. Metrics print before exit, so a VM that corrupted its heap and died in an atexit handler still emitted RESULT and the whole table, and the harness published checksum-matched medians from a crashed run. The cast-semantics baseline names anonymous classes as Outer$N, so #5746 adding one to AndroidImplementation renumbered onReceive from $46 to $47 and left the entry stale. master is red on the gate today; this is the same cast, not a new one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
d1904d3 to
1c8ab03
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c8ab03bf5
ℹ️ 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".
cn1GcRecoverAfterDrop is reached precisely when soft-referenced data is what is exhausting memory, and blanket retention there is what lets the allocator spin -- which is why it clears selectively instead of calling cn1GcRetainAllReferences. It then recorded each condemned referent in clearedReferent and marked that same referent one statement later in the same iteration, so the sweep kept every one of them and the reclaim never happened. Its own comment described the behaviour it did not have: "in the common case, where no further drop occurs, the clear stands and the reclaim happens". The recovery is owed to clears made by an EARLIER pass, which predate the drop that brought this one here; it is not owed to the clears this pass has just decided for itself, after its own quiesce. Entries therefore record which pass cleared them. A drop count cannot make that distinction, and the first version of this fix used one. The getter the record defends against fails its enqueue BEFORE the clear stamps anything, so the count at the clear already includes it, and "the count moved since" is false in exactly the case where the mark is owed -- it would have reintroduced the dangling read the record exists to prevent. The drop count is now read once per pass, after the quiesce that bounds the window, and a drop past it retains through the shared walk. Two harness gates that could not fail: - RefPolicy exits nonzero on ALIAS_SPLIT. This does NOT make the phase a detector for the violation, and the code says so: with -DCN1_REF_NO_ALIAS_ATOMICITY putting the single-loop bug back, three runs still reported 0/256, because catching it needs a get() inside a window microseconds wide. The ablation is what tests that path. - ab-refs.sh requires the weak phase to have cleared something -- and for noweak, nothing. The checksums are independent of retention policy by design, so they agree just as well when an arm keeps every referent strong, which is the regression that matters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
On a clean checkout, the documented ./run-gc-verify.sh GraceAudit invocation builds only GraceAudit-verify, but the unconditional second self-test later executes target/bin/LargeArrayLoad-verify; conversely selecting LargeArrayLoad leaves the first self-test without GraceAudit-verify. Thus every single-driver invocation either fails with exit 127 or silently depends on stale binaries from a previous run, rather than testing the requested driver; build both self-test prerequisites independently of DRIVERS or skip unrelated self-tests in this mode.
ℹ️ 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 review read the unmigrated createSoftWeakRef as the feature being undelivered: the base implementation still returns a WeakReference and the iOS override still pins every entry in a Hashtable until a memory warning replaces the map, so no framework cache constructs the new class. The facts are right and the conclusion is not. Migrating them changes the lifetime of every decoded image, gradient and resource cache in every app on every platform, which wants its own change and its own bisect point -- and each call site needs deciding rather than sweeping, because a lifetime tracker like JavascriptContext reads a null extract as proof of collection and breaks under a reference that outlives its referent. This change is the mechanism and the measurement that justifies it. Noted at both places a reader arrives from, since a PR thread is not somewhere anyone looks later. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
ParparVM's collector had no notion of a weak root.
java.lang.ref.WeakReferencekept its referent in an ordinary field, so the translator emitted agcMarkObjectfor it and the referent was strong — a "weak" reference pinned its referent for the life of the process, and every cache built onCodenameOneImplementation.createSoftWeakRef(theEncodedImagedecode cache,Image's scale and RGB caches,Border's round-rect cache, rasterised gradients) was unbounded. There was noSoftReferenceat all.What changed
The referent moves up into
Reference, and the translator no longer traces it (ByteCodeClass.isReferenceReferent). It emitscn1GcDiscoverReferenceinstead, which hands the collector the field addresses and decides soft retention on the spot. Clearing happens incn1GcProcessReferencesusing the sweep's own liveness test — both halves of the sweep free onmark != -1 && mark < currentGcMarkValue - 1, and nothing else may be cleared, because a dangling read here is a native crash no Javacatchcan see.Three placement decisions, none optional:
get()and hold it in a local the collector has already walked past; that referent is then neither marked nor fresh, which is the one case the sweep's "already marked or FRESH" invariant does not cover.get()carries a load barrier, so a racing read makes the trial clear ofgcSatbActivefind a non-empty log, which re-arms and re-runs the fixpoint and this pass with it.CN1_SATB_REF_LOAD). Logging every referent read is not a cost but a failure: unfiltered it put over 10,000 entries in the log per cycle and hitCN1_SATB_MAX_REOPENSon every cycle — the collector failing to converge. Skipping referents already marked this epoch or fresh (exactly those the clear pass would refuse to clear) tookpasses32 → 1 andrefMs0.06 → 0.005.get(), decided when the mark first reaches the reference, so the mark stays single-pass. Deciding afterwards means a second reachability closure over the retained set, on a mark that already spends most of its time in the grace pass.Measurement
vm/benchmarks/src/com/bench/RefPolicy.java+ab-refs.sh, five interleaved reps underCN1_SIMULATE_PROC_MEMORY_LIMIT, checksums identical across every arm:References themselves are unambiguous: 255/256 unreachable referents reclaimed against 0/256, for 1.8–4% of mark time,
vm/benchmarksgeomean 1.011 over 12 interleaved reps against master.The ranking is a cheap rider, not the payoff. Against never-clearing it is the same hit rate for about a megabyte. What it beats decisively is the pressure-triggered arm, which gives up 12 points of hit rate for zero footprint saving and is worse on both axes at 160MB — and that arm is the model of the iOS port's
didReceiveMemoryWarning -> flushSoftRefMap. Keeping the ranking is defensible because it costs a store, not because this shows it winning; a fixed retain budget would drop the age field and lose nothing this measurement can see.vm/CLAUDE.mdrecords the table plus three conclusions that were drawn from single runs and turned out to be wrong. The one that generalises: a pressure-triggered cache policy cannot work on this collector — the pacing loop defends the reserve by throttling the mutator, so headroom converges on any threshold placed there and never crosses it. A first attempt never fired at all; a second, written as multiples of the reserve wherereserve * 4is the whole budget, fired always.Not measured: nothing here separates ranking by recency from "trims at all", since there is no random-eviction arm at a matched rate.
Scope
Confined to the VM. The porting layer, the iOS
softReferenceMapoverride and the ~20 core call sites are deliberately untouched and follow separately — they change behaviour in every iOS app and want their own bisect point. Worth flagging for that follow-up: the shared table is a live correctness bug today, since one iOS memory warning makesJavascriptContext.cleanup()release every JS object under a still-reachable Java wrapper.Gates
run-gc-verify.shgreen —RefPolicyclean over 47 verify passes, both fault-injection self-tests firingrun-gauntlet.shgreen — all tortures bit-identical to the host JVMvm/testsfast leg — 552 tests, 0 failuresvm/testsbenchmark leg — 10/10, includingGcSteadyStateIntegrationTestandProcessBudgetPacingIntegrationTest-DCN1_NO_WEAK_REFSrestores the old strong behaviour as an A/B arm, and reports0/256so the driver cannot go inert🤖 Generated with Claude Code