ParparVM: collector and codegen cleanup - #5658
Merged
Merged
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. |
![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: 851d60d60d
ℹ️ 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
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Contributor
Cloudflare Preview
|
Collaborator
Author
|
Compared 166 screenshots: 166 matched. |
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 160 screenshots: 160 matched. Benchmark Results
Detailed Performance Metrics
|
The clean (non-Objective-C) target could translate a Java main() and run it, but
not much more: main(String[]) was handed JAVA_NULL, so a translated program could
not read its own command line, and there was no way to read the environment, open
a file or read stdin. Every knob had to be a compile-time macro, which is why the
GC benchmarks are parameterised the way they are.
- argv reaches main(String[]) via cn1MainArgs, skipping argv[0] the way Java does
- System.getenv(String)
- java.io.FileInputStream / FileOutputStream over C stdio, so the same code
serves the Windows target, which has no unistd.h
- java.io.StandardInputStream behind System.in. Not a FileInputStream: stdin is
not seekable, so skip and available cannot be answered by seeking
Separately, CHECKCAST. BC_CHECKCAST expanded to nothing, so a failed cast handed
the wrong object to the next instruction and the target type's fields were read
out of it -- a native crash no Java catch can see (issue #5531). Implementing the
macro alone would have changed nothing: BytecodeMethod DELETES the CHECKCAST
instruction before codegen ("gets in the way of other optimizations"), so nothing
ever reached TypeInstruction. Array stores had the companion hole -- AASTORE was
bounds-checked but never covariance-checked, and the macro's own comment claimed
otherwise.
Both are now enforced under -Dcn1.checkedCasts=true, which also drives retention
of ClassCastException and ArrayStoreException so the emission and the classes can
never disagree and leave an unresolved symbol. Opt-in, because turning it on
changes the outcome of app builds that succeed today; a server-side build parsing
untrusted input should always enable it.
Verified against vm/tests: 80 tests, no regressions.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The next stage is a standalone server rather than a Lambda, and the first
question it asks is whether a connection can have a thread. That needed a number,
so ThreadCost parks N threads and holds them while RSS is read from outside.
Measured with 512 parked threads:
musl/arm64 (the deployment target) 243 KB/thread
macOS/arm64 118 KB/thread
Attribution on Linux, by ablation:
callStack arrays (1024 -> 128) -50 KB
pendingHeapAllocations (4096 -> 256) -27 KB
try blocks (500 -> 32) -15 KB
shadow stack (16536 -> 2048) 0 KB
thread stack (16MB -> 256KB) 0 KB
Two of those are worth recording because they are the opposite of what the
macOS numbers suggested. The shadow stack, the biggest single allocation at
258KB, costs nothing resident on Linux -- shrinking it changes the number not at
all, though on macOS it looked like the dominant cost. And the pinned 16MB thread
stack is free: it is reserved, never committed.
The five sizes are now #ifndef-guarded so an A/B can override them with -D. They
were unconditional #defines, so a -D was silently ignored -- the redefinition
warning is suppressed by the generated code's -w, which is how the first round of
ablations produced three identical numbers and no conclusion.
The shadow stack is now mapped rather than malloc'd and memset in full. That is a
spawn-path win (258KB of stores per thread creation), not a footprint win; the
comment says so rather than implying the measurement it did not produce.
The conclusion for the server design: at 155-243 KB even with every buffer
shrunk, ten thousand connections is 1.5-2.4GB of threads. A connection cannot have
one. The design is a reactor with a bounded worker pool, where a few dozen threads
cost a few megabytes and the connection is just an fd.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
throwException walked the try-block stack looking for a handler and, when it found none, RETURNED. The generated code then carried on with the statement after the throw, with the method's locals in whatever state the failed operation left them. On an app target something upstream nearly always catches -- the EDT's own try -- so this stayed invisible; a server binary has nothing above main. What it looked like in practice: a database client whose TLS handshake was rejected threw, Database.open "returned" a null, and the program segfaulted two statements later on the null. The message that would have named the real cause was never printed, and a program that threw out of main exited with status 0. The clean target now prints the exception, its message and a stack trace, and exits 1. Every other target keeps today's behaviour: making this fatal everywhere would change what apps that ship today do, so the generated main() opts in and nothing else does. Two details the fix needed. The message is fetched separately because the pre-rendered stack string carries only the type, and on a server the message is the actionable half. And the try depth is reset to zero before rendering: the search leaves it at -1, and a Java method that saves and restores a negative depth corrupts what it restores into, which turned the reporter itself into a SIGBUS. Also here, because the same audit found it: java.lang.System.in is a static field, so every translated program reaches StandardInputStream's natives, and the JavaScript backend had no category for them -- which turned the core-slice completeness gate red for code that never touches stdin. They are marked unsupported there, as java.io.File already is: a browser has no process stdin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both are one-line consequences of the same C rule, found by building the same program two ways. ATOMIC_VAR_INIT on an atomic POINTER is rejected by clang 14 -- which is what Debian bookworm ships, and therefore what the glibc backend builder image uses -- as "initializer element is not a compile-time constant". The generator emits it for every `volatile` static reference field, so any such field in ordinary user code failed to build there. A static object is zero-initialized by the language, so the initializer is dropped; the macro is deprecated in C17 and gone in C23 regardless. CN1_RESUME_THREAD referenced gcParkCaptured unconditionally, but that field only exists when conservative roots are compiled in. So -DCN1_DISABLE_CONSERVATIVE_GC_ROOTS -- the A/B arm vm/CLAUDE.md documents -- did not build at all, and the one measurement that isolates the conservative scan's cost could not be taken. It is now behind a macro that compiles away with the field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A virtual thread runs Java on a stack of its own, so parking one is a stack
switch of a couple of nanoseconds rather than a blocked OS thread. Measured
round trip on arm64: 2.1ns.
The runtime is three files -- cn1_virtual_thread.{h,c} and the context switch,
which has to be assembly because glibc aborts a cross-stack longjmp under
_FORTIFY_SOURCE and musl has no makecontext. aarch64 and x86_64 are implemented;
anywhere else the header's stubs answer "there is no virtual thread here", which
is the truth, and every caller folds away at compile time.
The collector had to learn about them, because a virtual thread breaks two of its
assumptions silently:
- A carrier RUNNING a virtual thread has its stack pointer inside that virtual
stack, so the [sp, base) bounds test rejected it and skipped every
conservative root the thread held.
- A PARKED virtual thread is referenced by nothing the collector walks, while
its stack still holds Java references in C temporaries.
Both are served from a registry snapshot taken once per cycle before any thread
is stopped: walking the live registry would take its mutex, and a thread frozen
by the stop signal may be the one holding it.
Also here, because they are what made the above work: the translator emits the
runtime into every generated project, and CN1_RESUME_THREAD yields a virtual
thread rather than sleeping the carrier it runs on -- a carrier hosts many
virtual threads, so sleeping it freezes all of them.
Carried along in the same change: LinkedHashMap runs its eviction hook only on a
real insertion, as java.util does, which also drops an allocation per insertion;
a generated mapper can serialise straight to JSON instead of filling a map and
walking it back, measured 2.05x/1.51x/2.81x on a four-property object with output
asserted byte-identical; and a repeated CHECKCAST is dropped when it immediately
follows the identical one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CN1_RESUME_THREAD waited out a collection with usleep(1000). Two things make that expensive on the backend and neither is visible at the call site. It sleeps the CARRIER, and a carrier hosts many virtual threads: hostCount is min(workers, cores), so on a two-core pin sixty four connections share two carriers. One carrier sleeping a millisecond freezes about thirty two connections that were ready to run, which is the shape of a server whose median is healthy and whose tail is not. And it is a sleep-poll, so the wait is quantised to the sleep interval however briefly the flag was actually held. The measured worst case was 1923us: two iterations of a 1ms sleep waiting for something that had long since cleared. The pacing park already yielded here; this site did not, and it is the hottest of the four -- once per syscall return, 204105 times in a twenty second run against 9 for the handshake. Platform threads still sleep, having nothing to yield to, and off the backend the stub answers "not virtual" so the macro folds back to exactly the old loop. This shortens the wait; it does not remove it. The thread is still held until the collector has drained the whole worklist reachable from its roots rather than merely captured them, which is a separate question and a larger one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cn1SpawnVirtualThread and cn1CreateThreadLocalData were declared inside #ifdef CN1_CONSERVATIVE_GC_ROOTS. Neither has anything to do with how the collector finds its roots, and burying them there broke -DCN1_DISABLE_CONSERVATIVE_GC_ROOTS -- the precise threadObjectStack arm that vm/CLAUDE.md documents -- with an undeclared cn1SpawnVirtualThread in the backend's native sources. C being what it is, the implicit declaration then also produced an int-to-pointer conversion, so the failure named the wrong thing. Found while measuring that arm rather than by building it, which is the point: nothing builds it. The default build is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CN1_RESUME_THREAD is a safepoint: it can park the thread on a timed wait while a collection runs, and that overwrites errno. Reading errno after it recorded the WAIT's outcome rather than the read's, so lastError handed Java an error belonging to something else entirely. Captured at the syscall instead. The do/while EINTR retry idiom elsewhere is already safe -- it reads errno before the resume. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
The mark phase signals every thread and spins until it answers, so it can scan the thread's native stack conservatively. A thread that never answers is not scanned either way -- the caller returns 0 and reads nothing -- so the wait buys literally nothing, and one such thread cost 267ms of a 280ms mark, every cycle. Count consecutive timeouts per thread and skip a thread that has failed three of them, re-probing every 64th attempt so one that becomes responsive is picked back up, and clearing the count the moment it answers. The forced-stop escalation (issue #5537) must NOT be throttled this way, so the implementation takes a maySkip flag and the escalation passes 0. It retries every CN1_GC_SAFEPOINT_WAIT_MAX_US precisely to ride out a transient or descheduled handler; skipping those retries would leave the collector waiting on threadActive for tens of seconds, turning a recoverable timeout into exactly the whole-VM pause the escalation exists to prevent. Measured on the server workload: stackMs 269 -> 0.20. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sembly Two halves of one bug. Virtual threads were gated on a build flag that only the server build set, and the flag was justified by an Xcode misfiling it was working around: Xcode has no mapping for the .S extension, so an unrecognised one becomes `lastKnownFileType = file` and lands the file in the RESOURCES phase, where it is copied into the bundle and never assembled. The iOS target then failed to link naming _cn1VirtualThreadSwitch, whose source was sitting right there in the project. Gating the feature off made the misfiled resource inert, so the phone target linked and the misfiling stayed hidden. Fix the misfiling instead: .S maps to sourcecode.asm.asm (preprocessed, which the capability gate in the file needs) and .s to sourcecode.asm, and both route into the Sources phase rather than Resources. Every future assembly file gets this too. That removes the reason for the flag, so the gate becomes a capability test: on anywhere the switch is written for -- aarch64 and x86_64, excluding Windows, whose calling convention needs its own prologue -- virtual threads are on. There is no separate "server build" of the VM; a flag would only mean the feature is off in every build nobody remembered to set it in. Elsewhere the header's no-op stubs answer "there is no virtual thread here", which is true, so the collector needs no #ifdefs and every call folds away. CN1_DISABLE_VIRTUAL_THREADS forces that path. The predicate is repeated verbatim in the .S, which is preprocessed assembly and cannot include the header -- the two must stay identical or the link breaks on the switch symbol. Also excludes LinkedHashMap from the copyright gate: it is Apache Harmony source and keeps its Apache-2.0 notice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Turning virtual threads on by capability rather than by a flag nobody set made
three latent bugs reachable at once, all the same shape: the context switch was
copied into the generated project and never assembled, so the C half linked
against a symbol whose source was sitting in the same directory.
- CMake globbed *.S only for the LINUX app type, and only when embedding
resources -- the condition belonged to the resource blob, which used to be
the only .S there is. Now any .S present drives both the ASM language and the
glob, on every cmake target.
- The WINDOWS app type is also cross-built with clang on a POSIX host, where
_WIN32 is undefined, the switch is live, and MSVC's inability to assemble GNU
syntax is irrelevant. That is a question about the compiler, and CMake can
only answer it after project() has enabled C, so it is asked there rather
than guessed from the app type. Under MSVC the variable stays unset and
expands to nothing.
- Xcode has no mapping for .S at all, so it became `lastKnownFileType = file`
and landed in the RESOURCES phase, shipped into the bundle and never built.
sourcecode.asm is the identifier for both spellings: Xcode's own
StandardFileTypes.xcspec lists it as `Extensions = (s)` with
`GccDialectName = assembler-with-cpp`, which is the preprocessing the file's
capability gate needs. The neighbouring sourcecode.asm.asm is for .asm.
Tests. BackendUncaughtExceptionTest needed a support class that does not exist
here, and only ever reached the fix through a server binary; replaced by
UncaughtExceptionIntegrationTest, which builds a clean-target program directly
and asserts the whole contract -- message, stack frame, non-zero exit, and that
execution stops AT the throw rather than carrying on, which is the half the other
three can all pass without.
test_virtual_thread.c was built by nothing. A hand-written context switch with no
enforced coverage could break in any commit and stay green, so
VirtualThreadRuntimeTest drives it from the suite, compiled out of the SAME
staged classpath resources a generated project receives -- which also asserts
those three files are present and agree with each other.
The iOS project test now asserts the assembly is typed as assembly, IS in the
Sources phase and is NOT in Resources. All three: the type alone does not prove
the phase, and the phase alone does not prove it assembles.
The generator's own source set is what caught the last of it. Two copies of
replaceLibraryWithExecutableTarget matched the add_library line by its full
argument LIST -- the shared one in CleanTargetIntegrationTest and a private
duplicate at the bottom of FileClassIntegrationTest. Adding the assembly glob
made both stop matching, so those tests built a library and then failed running
an executable nothing had asked for. The shared one now matches the CALL and
asserts the substitution happened; the duplicate is gone, and FileClassIntegration
uses the shared one like the other twenty-two callers already did.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
force-pushed
the
vm-performance-and-gc-cleanup
branch
from
September 1, 2026 19:17
851d60d to
fe581d5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe581d5813
ℹ️ 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 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
CODENAME_ONE_ASSERT is plain assert(), which NDEBUG compiles out of every release build. So once all NUMBER_OF_SUPPORTED_THREADS slots were taken, threadOffset stayed -1, the assertion vanished, and the next statement executed allThreads[-1] = i -- writing over whatever precedes the table. A debug build aborted; a shipped one carried on with silent memory corruption, which is the worse of the two. Capacity exhaustion is a condition to report, not to assert. It returns 0 now, and cn1SpawnVirtualThread already checks for that. Pre-existing rather than new: every OS thread creation runs this path too. A virtual thread per request only makes reaching the limit realistic. The partially built state is unwound through cn1FreeThreadLocalDataFields, extracted from cn1ReleaseThreadLocalData rather than copied, because the release path also decrements nThreadsToKill and a state that never reached allThreads was never counted as living. Duplicating the frees would have drifted apart, and getting that counter wrong would have been a slow leak in the opposite direction. Verified across the GC suites including GcUncooperativeThread and GcHeapIntegrity: 6/6. (The translator build says nothing about this -- it compiles Java, and the C here is only compiled by those tests.) 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: 9f27f80b21
ℹ️ 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".
…reeing Two defects, and both are mine from earlier in this branch. THE HANG I REVERTED WAS STILL REACHABLE. Removing the threadActive assignment from cn1VirtualThreadResume did not close it, because CN1_RESUME_THREAD does the same thing and every bracketed native goes through that macro. getThreadLocalData() resolves to the VIRTUAL thread's state while one is running, so a virtual thread that read a file or a socket returned with its state marked active, and nothing lowers it again until the next yield. Same unbounded while(threadActive) wait, same forced-stop escalation gated on gcPthreadValid and therefore unavailable, same stall. I checked the call site I had edited and not the shared path through it. The guard states the invariant the code always needed: mark active only what the collector can STOP. gcPthreadValid is exactly that question. A real thread is unaffected; a virtual thread's state stays down, which is where it was before any of this. Roots do not depend on the flag -- cn1GcScanParkedVirtualThreads scans every registered virtual thread whether or not it is running. THE EXHAUSTION CHECK INTRODUCED A USE-AFTER-FREE. pthread_setspecific binds the new state to TLS above the capacity search, so the failure path I added freed a state the key still pointed at: every later getThreadLocalData() on that thread would return memory that had been given back. That is worse than the out-of-bounds write it replaced, because the thread keeps using the stale pointer rather than failing. Unbound before the free. Also: System.getenv(null) throws NullPointerException as the API requires, instead of returning null and making an invalid argument indistinguishable from an unset variable. Verified across the GC suites, 6/6, including GcUncooperativeThread and GcHeapIntegrity. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
emitFieldToMap stores `_v.toString()` when the declared type of a REFERENCE field has no registered mapper, so JSONWriter quotes it: an Object field holding an Integer serialises as "5". appendJsonUsing passed the raw instance to writeJson instead, which emits 5 -- a change of wire TYPE, not just of formatting, the day a mapper gains a direct writer. Mapper.Direct promises identical output. Mapping parity 6/6. 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: e1ea7199d2
ℹ️ 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".
InputStream.close() is a no-op, and this class did not override it, so a caller that closed System.in -- directly, or by closing a Reader wrapped around it -- kept reading and CONSUMING standard input instead of getting the IOException the contract promises. Reads after close now throw. The flag is volatile because a stream is usually closed from a different thread than the one blocked reading it. The file descriptor is deliberately NOT closed, which is a departure from what the report suggested and the reasoning is in the code. Descriptor 0 belongs to the PROCESS rather than to this object: the VM and any native library in it may still be using it, and once released the number is free for the next open() in the process to take -- so a later read would be answered by an unrelated file instead of failing. That is a worse outcome than the bug being fixed. Closing the stream stops this stream, which is what the caller asked for. The test drives a real clean-target binary, because the behaviour only exists once the native read is wired up, and it discriminates by construction: without the fix stdin is empty, the read returns -1, and the program prints CLOSE_NOT_HONOURED. 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: f13a53c611
ℹ️ 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".
…inert
Two surfaces in this branch are server-side work in progress rather than shipping
features, and review has been treating them as shipping features. Saying so in the
code is the answer to that, not another round of patches.
CHECKED CASTS STAY OFF, INCLUDING ON THE CLEAN TARGET. Review observed that
nothing sets -Dcn1.checkedCasts=true and proposed defaulting it on for clean
builds. Inert is the intent: the feature is unfinished, and enabling it would
change codegen for every clean-target build in the tree to exercise a path still
being designed. The flag stays the way in. The emitted checks are maintained under
it -- the null guard, the JLS ordering, the one-dimension restriction -- but their
presence is not a claim that the VM validates casts today, and CLAUDE.md's "never
rely on ClassCastException" remains the rule for every shipping target. A comment
that claimed builds pass the flag is corrected; none do.
cn1SpawnVirtualThread AND cn1RetireVirtualThread ARE EXPERIMENTAL. Nothing in this
repository calls them; they ship so the server work can build against them. Their
three known gaps are named at the definition -- a collection can walk the state's
object stack while the virtual thread mutates it, retiring one retires the
CARRIER's BiBOP pages, and the collector cannot stop a compute-only virtual thread
-- and all three wait on the same design decision: carrier association, which
means the stop handshake giving up being per-TLD. Findings there are noted, not
patched, because every patch so far traded one hole for another: a scanning race
became a collector hang, a bounds fix became a use-after-free.
The line is drawn explicitly in both notes. The COROUTINE runtime underneath --
cn1_virtual_thread.{h,c,S} and the collector's stack scanning -- is finished,
tested, exercised by VirtualThreadRuntimeTest, and is NOT experimental.
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: fbc8ca83aa
ℹ️ 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 virtual thread registered after the collector's once-per-cycle snapshot is invisible to the stack scan until the next cycle. Raised in review as a P1; it is real, and it belongs to the EXPERIMENTAL spawn API rather than to the scan. Inside the VM the only caller of cn1VirtualThreadCreate is cn1SpawnVirtualThread, which nothing in this repository calls -- the other callers are the standalone runtime test, which has no collector. Not widened here, and the reason is in the code: covering post-snapshot registrations from this pass means holding the registry lock during the scan, and avoiding exactly that is what the snapshot is FOR -- a thread frozen by the stop signal may be the one holding that lock. The suggested remedy trades an unreachable missed root for a reachable deadlock. Listed as the fourth known gap above cn1SpawnVirtualThread. All four resolve together through carrier association, when there is a caller to design against. 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: 9896b1f157
ℹ️ 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".
Same defect as the one already fixed in VirtualThreadRuntimeTest, in the other test this branch adds: output was read inline before waitFor, and that read blocks until the child closes stdout. A program that HANGS -- one of the regressions this test exists to catch -- therefore never reached the timeout, and the job would sit until CI killed it rather than failing here. A timeout that the guarded failure prevents from being evaluated is not a timeout. Swept for it rather than fixing the reported line alone, and the sweep narrowed the scope rather than widening it: 26 places in the suite read process output before waitFor, but 24 of them use the UNTIMED waitFor(), where a blocking read is equivalent and there is no timeout to defeat. Only the two tests added by this branch pass a timeout, and both are now drained on a separate thread with a bounded join. Nothing else needs changing. 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: de54965605
ℹ️ 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".
stringToUTF8 returns threadStateData->utf8Buffer -- one buffer per thread, reused -- so converting dest overwrote the source and rename(p, d) was rename(d, d). It reports success when the destination already exists and failure when it does not, and never moves the source. Not merely aliasing either: the helper frees and re-allocates when the second string is longer, so the first pointer can be dangling rather than stale. Two corrections to how this was reported. It is not Windows-specific -- the shared non-ObjC arm serves Linux and the clean target too -- and renameTo on the clean target has therefore been entirely non-functional rather than degraded. The source is copied out before the second conversion now. Swept before fixing: this is the ONLY function in java_io_File.m, nativeMethods.m or cn1_globals.m that converts two strings in one call, so the fix is local, and that is from a check rather than an assumption. It survived because renameTo had no test at all -- grep found zero references in the suite. The coverage added here asserts the source is gone, the destination exists, AND that the three bytes moved; content is the assertion that discriminates, since the aliased version reported success while moving nothing. The destination name is deliberately longer than the source, which is the case that makes the buffer reallocate and the pointer dangle rather than merely alias. Verified by reverting: 5/5 fail against the aliased version, 5/5 pass with the 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: 5694e1526b
ℹ️ 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".
Fixing the unmapped REFERENCE case earlier in this branch, I made appendJsonUsing quote instance.toString() when no mapper is found. That is right for a reference field, where emitFieldToMap stores _v.toString(). It is wrong for a list ELEMENT, where emitFieldToMap stores _e unchanged and the writer keeps its JSON type -- so a List<Object> holding 5 serialised as ["5"] instead of [5]. Two paths with different map-path semantics, one rule applied to both through a shared helper. The generated list code now splits the no-mapper case explicitly and keeps the declared-type lookup for the rest. Covered: the parity test carries a List<Object> of a number, a boolean and a string, and pins "mixed":[5,true,"s"]. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both stream classes are new in this branch and neither had a reclamation hook, so a stream that went unreachable unclosed held its FILE* until the process exited. On a desktop app that is untidy; on a long-running clean-target server it ends in EMFILE, and for output it also drops whatever was still buffered. finalize() is the established convention here rather than an invention -- java.lang.Thread already releases its native thread state the same way, and this VM runs finalizers for exactly this purpose. Deliberately silent: a finalizer has nobody to report to, and throwing from one is worse than the leak it is cleaning up. close() remains the way to learn that a close failed. 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: 30d7662b63
ℹ️ 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".
…t cap Three review findings, and they get three different answers. A SINGLE LEADING SEPARATOR IS NOT ABSOLUTE ON WINDOWS. "\logs\app.txt" is rooted but still drive-relative -- it means that path on whichever drive is current -- and only "\\server\share" is fully absolute. Reporting the first as absolute made getAbsolutePathImpl hand it back unqualified. It is now qualified with the current drive, rather than joined to the whole working directory, which would have produced "C:\cwd\logs\app.txt". CLOSING TWICE IS NO LONGER FATAL. Two threads could both read closed == false and pass the same FILE* to fclose, which is undefined and takes the process down rather than returning an error. volatile plus a synchronized close makes it idempotent, and the finalizer takes the same lock -- otherwise the finalizer IS the second closer. What that does NOT do, stated in the code so it is not mistaken for more: a read racing a close on the same stream can still reach the native call with a handle being closed. The JDK buys that with a lock on every operation, and these streams are not worth that on every read; like most java.io streams they are for one thread at a time. The guarantee is that closing twice or closing from another thread is safe, not that concurrent use is. THE SNAPSHOT CAP IS LISTED, NOT FIXED. Past 4096 registered virtual threads the collector's snapshot truncates and the overflow goes unscanned. Reaching that count requires cn1SpawnVirtualThread, which nothing calls -- so it joins the other known gaps above that function rather than turning into collector surgery for an unreachable case. It is the second P1 raised against code that only the EXPERIMENTAL API can reach. 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: bd2057f13a
ℹ️ 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".
…write
The report named fflush. Sweeping the file layer found five unparked blocking
calls rather than one, and the two I would not have thought of are the opens.
- fflush pushes the buffer at the peer and blocks exactly where the write does.
- Both fclose calls FLUSH before closing, so they block in the same place.
- Both fopen calls block on a FIFO: opening for read waits until a writer opens
the other end, opening for write waits for a reader, and there may never be
one. Opening reads as cheap, which is precisely why it was missed.
Each left the VM thread active while it blocked, so a collection waited for a
safepoint that could not arrive -- and on Windows, where CN1_GC_CAN_FORCE_STOP is
off, there is no escalation to break that wait.
The opens need no buffer keep-alive, unlike the reads and writes: `path` points
into the thread's utf8Buffer, which is C memory a collection cannot move or
reclaim, whereas those hold an interior pointer into a Java array the collector
could sweep.
Verified by re-running the same sweep afterwards: all eight java_io_* natives that
touch stdio now park.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
CodenameOne/vm/ByteCodeTranslator/src/nativeMethods.m
Lines 1405 to 1408 in 0c96b7b
Use the monotonic Windows clock for nanoTime
On Windows, any wall-clock correction from NTP, daylight/administrative changes, or a manual adjustment makes this value jump forward or backward, violating System.nanoTime()'s monotonic elapsed-time contract and causing deadline/timeout calculations to expire early or stall. The Windows compatibility layer already provides the QueryPerformanceCounter-backed cn1_monotonic_micros() in cn1_win_compat.c, so this branch should derive its result from that helper rather than gettimeofday().
ℹ️ 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".
Half of this report is right and half of it is not.
THE EMBEDDED NUL IS REAL. The native converts to a C string, where a NUL ends it,
so a lookup of "PATH" + NUL + "suffix" found PATH and returned that variable's
value. A silent answer about a DIFFERENT variable is worse than reporting the name
unset, so a name containing a NUL is now answered null.
The check is in Java because that is where the information is: one indexOf against
re-deriving the byte length in C and walking the string's backing representation,
which is the compact byte[] versus char[] distinction consolidated earlier in this
branch. That moved the null check up too, so the native is now the raw lookup.
ILLEGALARGUMENTEXCEPTION IS DECLINED. Neither this VM's contract for getenv ("or
null when it is not set") nor java.lang.System.getenv(String) declares it -- the
documented exceptions are NullPointerException and SecurityException. The
validation that throws IllegalArgumentException belongs to ProcessBuilder's
environment mutation, not to a lookup. An empty name, or one containing '=', names
nothing, and null is exactly what "not set" means. Adding the throw would make this
VM diverge from the platform in the name of matching it. Written at the method so
it is not re-raised.
The rename to getenvImpl is the dangerous part of this edit -- a wrong native name
compiles, links, and silently drops the method, leaving a green build and an inert
feature. check-native-signatures.sh reports 0 fatal with every native resolving on
both ports, and the UTF-8 environment test passes end to end, which it could not if
the symbol had been dropped.
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: 9f7d96b856
ℹ️ 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".
TWO THINGS, and the first is a correction to my own sweep. Last round I said every java_io_* native touching stdio now parks, and verified it mechanically -- against a pattern list I had built from the calls I had already fixed. It did not include ftell or fseek, so skipImpl and availableImpl were still unparked. A mechanical check is only as good as the pattern given to it. Seeking is not free everywhere: a remote mount or a FUSE filesystem services ftell/fseek over the wire, and the thread sits inside the CRT for the duration -- where a collection waits for a safepoint that cannot arrive, with no forced-stop escalation on Windows to break it. Both functions take ONE yield spanning their whole seek sequence rather than bracketing each call: the collector only needs the thread parked, and three yield/resume pairs would cost more than the seeks they guard. Re-swept with ftell/fseek included: zero unparked. THE THREAD BENCHMARK NEVER TERMINATED. ThreadCost spawns non-daemon threads parked on LOCK.wait() and nothing ever notified them, so returning from main ended only the main thread. The documented "/usr/bin/time -l /tmp/threadcost" invocation could not print its result without an external kill -- the measurement was taken and then discarded. It notifies after measuring; waking the workers cannot affect a number already recorded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
added a commit
that referenced
this pull request
Sep 6, 2026
check-copyright-headers fails any file a PR touches whose header is neither the Codename One nor the Oracle GPLv2 + Classpath text. HashMap, Hashtable and IdentityHashMap carry the Apache Software Foundation notice, because they are Apache Harmony derived -- byte-identical in provenance to TimeZone.java and LinkedHashMap.java, which the exclusions file already lists for exactly this reason (LinkedHashMap was added when #5658 last modified it). Excluding them is the correct fix and replacing the header is not: rewriting an Apache-2.0 notice as the Codename One GPL header would misstate the licence of third-party code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
added a commit
that referenced
this pull request
Sep 6, 2026
check-copyright-headers fails any file a PR touches whose header is neither the Codename One nor the Oracle GPLv2 + Classpath text. HashMap, Hashtable and IdentityHashMap carry the Apache Software Foundation notice, because they are Apache Harmony derived -- byte-identical in provenance to TimeZone.java and LinkedHashMap.java, which the exclusions file already lists for exactly this reason (LinkedHashMap was added when #5658 last modified it). Excluding them is the correct fix and replacing the header is not: rewriting an Apache-2.0 notice as the Codename One GPL header would misstate the licence of third-party code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
added a commit
that referenced
this pull request
Sep 7, 2026
…#5722) * ParparVM collections: fix an O(n) HashMap miss, and compact Hashtable HashMap here is open addressed with linear probing, and cn1Marker spreads the hash with the JDK's h ^= h >>> 16 -- a spread designed for a CHAINED map, where colliding keys share a bucket and clustering costs nothing. Integer.hashCode() is the value itself, so a dense key range (ids from zero, epoch seconds, counters, indices) lands at slot == value: one contiguous run of occupied slots with no gap. Linear probing then walked that entire run for any probe that entered it and did not find its key. Measured average probe length for a MISS: 2547 slots at 20k keys, 16742 at 100k, 222721 at 1M -- O(n) per unsuccessful lookup, reaching get returning null, containsKey returning false, and put of a key not adjacent to the run. In wall time, 3M containsKey calls against a 100k map took 32.7 SECONDS against 49.9ms on HotSpot. A remove/insert churn shape took 7.8 seconds against 12.5ms. Hits stayed at exactly one probe throughout, which is why hashMapChurn -- get and put on keys that are PRESENT -- reported a healthy 1.12x the whole time. The fix is the probe SEQUENCE, not the spread. Scrambling the hash was tried first and rejected on measurement: it fixes the miss but destroys the sequential placement, and dense-key build and scan shapes regressed 1.8x-2.2x (HotSpot's HashMap gets that same locality from that same weak spread). Keeping the first probe at marker & mask and perturbing only the steps after it -- CPython's dict recurrence -- keeps the locality and still leaves the run at once. missHeavy 32698ms -> 44.9ms (728x) tombstones 7781ms -> 16.5ms (471x) stringKeys 33.6 -> 25.5ms largeTable 26.6 -> 33.3ms (the cost) vm/benchmarks geomean 1.005, i.e. unmoved Hashtable gets the same compact layout it never received: no Entry object per mapping, a power-of-two mask instead of the % integer division it did on every operation, and the same perturbed probe. Build 1.74x. Lookup only 1.09x, and that is the useful part -- with identical probe code Hashtable is still 3x HashMap on the same workload, and the difference is synchronized. This VM keeps monitors in an address-keyed side table rather than an object header word, so an uncontended accessor costs more than the whole lookup. Further work there belongs on the monitor, not the map. IdentityHashMap indexed with % (length / 2) on an unscrambled identity hash. Copying java.util.IdentityHashMap's hash function made it SLOWER, and the reason is worth recording: HotSpot's identityHashCode is a scrambled per-object value, ours is a truncated object ADDRESS, measured 32-byte aligned. The JDK's (h << 1) - (h << 8) is h * -254, an EVEN multiplier, so it preserves those five zero bits and adds a sixth -- 2045 distinct home slots out of 65536 and 12.73 probes per lookup. The old modulo survived only because a non-power-of-two modulus folds high bits back in as a side effect. Folding explicitly (h ^= h >>> 16) reaches 50000/65536 slots and 1.00 probes; adding a multiply on top makes it worse again, because sequential allocation means sequential addresses and one fold is already near a perfect hash. 2.65x on lookup and build. Its rehash() overflow guard also turned an overflowed length into 1 -- an odd array length, which would have split every key from its value. Tests: MapBench adds the map shapes hashMapChurn cannot reach (miss-heavy, String-keyed, large-table, tombstone-heavy, grow-dominated, identity-keyed). It is deliberately outside CommonWorkloads, which port_status.py pins at exactly ten ids. HtTorture and IdmTorture join the gauntlet; HtTorture was written and verified against the CHAINED Hashtable before the rewrite, and IdmTorture was verified non-vacuous by injecting a relocation off-by-one that it caught. IdmProbe is a diagnostic that must run on the target, because the identity-hash distribution is a property of the allocator. Also removes three dead constants from parparvm_runtime.js, two of which named HashMap fields that the compact layout deleted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Record the three modified java.util maps as Apache Harmony sources check-copyright-headers fails any file a PR touches whose header is neither the Codename One nor the Oracle GPLv2 + Classpath text. HashMap, Hashtable and IdentityHashMap carry the Apache Software Foundation notice, because they are Apache Harmony derived -- byte-identical in provenance to TimeZone.java and LinkedHashMap.java, which the exclusions file already lists for exactly this reason (LinkedHashMap was added when #5658 last modified it). Excluding them is the correct fix and replacing the header is not: rewriting an Apache-2.0 notice as the Codename One GPL header would misstate the licence of third-party code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix the growth rule's hidden load-factor assumption, and containsKey's allocation Two review findings, both real. The growth rule. All three compact maps chose between doubling and rebuilding in place with `elementCount * 2 >= capacity` -- a capacity test standing in for a threshold test, which silently assumes a load factor of 0.5 or more. Below that, the threshold is reached while the table is still less than half full, so the rebuild keeps the same capacity, the rebuilt table is immediately at its threshold again, and every subsequent put rebuilds the whole table. Inserting 20000 entries at a load factor of 0.25 did 19999 rebuilds and rehashed 200 million entries: 22.3 SECONDS, against 16.4ms once fixed. The two-argument constructors take any positive load factor, so `new HashMap<>(16, 0.25f)` reached it from ordinary code. `elementCount >= threshold` is the rule -- grow when the LIVE count has reached the threshold, rebuild at the same size only when tombstones are what pushed occupancy there. At 0.75 and 0.5 the two rules agree rebuild for rebuild, which is why nothing existing moved, and why nothing existing could have caught it: `MapBench.lowLoadFactorBuild` and `HtTorture`'s sparse case exist for this alone, and the benchmark was verified to reproduce the 22.3s before the fix. Fixed in Hashtable, HashMap and LinkedHashMap. Only Hashtable's was new here -- the other two carried it from #5327 -- but it is one rule with one fix, and leaving two of the three would have been arbitrary. containsKey. The compact Hashtable delegated to getEntry, which built an Entry purely so the caller could compare it against null, turning every successful membership test into an allocation; the chained representation it replaced handed back the entry it already had. keySet().contains() routes here. It now answers from the probe index. The other getEntry caller, entrySet().contains(), genuinely needs the Entry for its equality test and is unchanged. GAUNTLET GREEN. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ParparVM: collector and codegen cleanup
Four independent changes, all on by default, none behind a flag.
Stop signalling a thread that never answers the collector
One thread in a typical process never answers the GC's stop signal, and the
collector spun the full 2,000,000-spin budget at it on every cycle before giving
up. That single wait was 267ms of a 280ms mark -- 97% of the collector's cost,
every cycle, forever.
The wait bought nothing: when
cn1GcSignalStopOnetimes out, the caller returnswithout reading the thread's stack, so the outcome is identical whether the
collector waits two million spins or does not signal at all. Only the waiting
differed.
Now: count consecutive failures per thread; after three, stop attempting it, and
re-probe every 64th cycle so a thread that becomes responsive is picked back up.
A thread that answers clears the counter.
stackMs-- the whole per-thread root phase, conservative scan and handshaketogether -- falls from 269ms to 0.20ms per cycle. Mark time is then dominated by
actual marking rather than by waiting.
How it was found, since the path was misleading: varying the conservative scan
volume 12x (262k to 3.2M words) moved
markMsnot at all, which ruled outscanning despite
stackMsandmarkMstracking each other almost exactly. Aper-phase breakdown then put 97% of mark in the stop wait, and a spin census
showed 5 stops per cycle sharing 2,003,073 spins -- four answering within ~200
spins each, and one consuming the entire budget.
Capture errno before the GC safepoint in the Linux socket read
CN1_RESUME_THREADis a safepoint: it can park the thread on a timed wait while acollection runs, which overwrites
errno. Readingerrnoafter it recorded thewait's outcome rather than the read's, so
lastErrorhanded Java an errorbelonging to something else entirely. Captured at the syscall instead. (The
do/whileEINTR retry idiom elsewhere was already safe -- it readserrnobeforethe resume.)
Run LinkedHashMap's eviction hook only on a real insertion
java.util.LinkedHashMapcallsafterNodeInsertion, and thereforeremoveEldestEntry, only whenputValadded a new node; overwriting anexisting key does not evict. This implementation called it after every put -- a
deviation from the specified behaviour and wasted work on the common path.
The
CompactEntryit passes exists solely to be handed toremoveEldestEntry.There are no node objects in this representation, so unlike the JDK -- which
passes a node it already has -- one has to be allocated. For a plain
LinkedHashMapit is built, passed to a method whose body isreturn false, anddropped: an allocation per insertion, on every caller, for nothing.
Drop a CHECKCAST that immediately repeats the one before it
Deliberately narrow. Only a
LineNumbermay sit between the two, because itcarries no semantics. A
LabelInstructionmay not: another path can jump therewith a different value on the stack, and then the second cast is the only thing
guarding it. Same reasoning for anything else in between -- if it can touch the
stack, the second cast is not redundant.
Testing
Full ParparVM suite: 540/541. The one failure is
GcSteadyStateIntegrationTest's 768MB-ceiling scenario, which fails identicallyon unmodified master on this machine (a core-count sensitivity documented in the
test itself) and passes in CI.
🤖 Generated with Claude Code