Uh oh!
There was an error while loading. Please reload this page.
ParparVM: collector and codegen cleanup - #5658
Open
shai-almog wants to merge 39 commits into
Open
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: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".
Uh oh!
There was an error while loading. Please reload this page.
CollaboratorAuthor
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
CollaboratorAuthor
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
|
CollaboratorAuthor
Compared 166 screenshots: 166 matched. |
CollaboratorAuthor
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
CollaboratorAuthor
Compared 181 screenshots: 181 matched. |
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
CollaboratorAuthor
Compared 217 screenshots: 217 matched. |
CollaboratorAuthor
Compared 144 screenshots: 144 matched. |
CollaboratorAuthor
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
CollaboratorAuthor
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
CollaboratorAuthor
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>
CollaboratorAuthor
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-almogforce-pushed
the
vm-performance-and-gc-cleanup
branch
from
September 1, 2026 19:17
851d60d to
fe581d5CompareThere 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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
CollaboratorAuthor
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:eaec4ed8e0
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…s UTF-8 The Windows clean-target leg failed the test added with the UTF-8 decoder, and it was right to: "cafe-acute-euro" arrived as 99,97,102,65533,65533 -- c, a, f, and two replacement characters. The CRT hands main() and getenv() the wide command line and environment already converted down to the ACTIVE CODE PAGE, so decoding those bytes as UTF-8 finds invalid sequences and substitutes U+FFFD for every non-ASCII character. That failure was predicted by a comment I had written in this very function -- which then shipped alongside a test asserting the behaviour the comment said did not exist. MultiByteToWideChar with CP_ACP is the conversion Windows actually needs, and it yields UTF-16 code units directly, so nothing decodes afterwards. RENAMED from newStringFromUtf8 to newStringFromNative for the same reason: a function named FromUtf8 that deliberately does not decode UTF-8 on one of its platforms is a trap for whoever reads it next. The name now says what it does -- convert text that came from the OS, in whatever encoding the OS used. WIN32_LEAN_AND_MEAN before windows.h, which is the same winsock timeval collision that broke java_io_File.m; and the byte-length local moved onto the POSIX arm, which is the only one that uses it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The JLS orders these: NullPointerException, then ArrayIndexOutOfBoundsException, then ArrayStoreException. Under -Dcn1.checkedCasts the emitted covariance check ran BEFORE the setter that reports the first two, so a store with both a bad index and an incompatible value reported the value -- hiding the exception the program should have seen. (The null case was worse and is already fixed: the check dereferenced the array to reach its class.) The store check is now guarded by the same access validation the setter performs, so the first two exceptions are thrown first and in the right order. The setter re-checks, which on the in-bounds fast path costs one comparison. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e collector This backs out my own fix from earlier in this branch. Marking the attached ThreadLocalData threadActive around the context switch reads as obviously correct and is a REGRESSION, worse than what it fixed. A virtual thread's state has no pthread of its own -- deliberately, it may run on a different carrier next time. The collector's wait for a lightweight thread is `while(t->threadActive) usleep(500)` with no bound, and the forced-stop escalation that exists to break exactly that wait is gated on gcPthreadValid, which is permanently false here. So the flag converts a POSSIBLE race on the state's object stack into a CERTAIN hang for any virtual thread that computes without reaching a safepoint: the collector waits for a flag only that thread can clear, and cannot stop it. What the same report asked for has two halves, and the other one stands. The C stack is covered: cn1GcScanParkedVirtualThreads scans every registered virtual thread whether or not it is running, so no virtual stack goes unscanned during the windows where `running` is set but the carrier has not switched yet. That fix is independent of this revert and stays. The half that remains open -- a collection walking the state's object stack and pending-allocation table while the virtual thread mutates them -- is documented at cn1SpawnVirtualThread along with why the obvious fix is worse and what the real one is: carrier association. A running virtual thread executes ON a carrier that does have a stoppable pthread, so the collector should satisfy the wait by stopping the carrier. That needs the stop handshake to stop being per-TLD (the signal handler records into the TLD of the thread it runs on, which is the carrier's), i.e. a change to the collector's stop protocol rather than to the spawn path -- not something to improvise in an API that has no callers yet. 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:2b9d12aec3
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
With -Dcn1.checkedCasts the covariance check broke correct programs, which is the worst direction for a check to fail in. A generated array class records arrayType as the BASE element class rather than the immediate component: String[][] has dimensions 2 and arrayType String, not String[]. So `values[0] = new String[1]` asked whether a String[] is an instance of String, got no, and threw ArrayStoreException on a store the language requires to succeed. Restricted to dimensions == 1, where arrayType genuinely IS the component type. Multidimensional stores lose a diagnostic that did not exist before this feature was added; the alternative was breaking working code. Covering them properly needs the immediate component type, either emitted per array class or reconstructed from dimensions at runtime, and the macro says so. Also fixes a timeout in VirtualThreadRuntimeTest that could never fire. It read the child's output inline and then called waitFor: the read blocks until the child closes stdout, so a binary that hangs -- exactly what a context-switch regression produces -- never reached the timeout, and the Maven job would sit until CI killed it instead of the test failing. Output now drains on its own thread, with a bounded join so a wedged reader cannot reintroduce the hang the change removes. 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:48e84bb243
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This corrects my own change earlier in this branch, and the reasoning behind it was the defect. "A thread it cannot stop is one it does not scan either way" is true only while the thread genuinely cannot be stopped. Failures are often TRANSIENT -- a stop signal briefly masked is enough -- and the thread recovers. Skipping it then meant cn1GcScanThreadNativeStack returned without scanning a RESPONSIVE thread, for roughly the next sixty collections, so references held only in frameless C locals or registers went unmarked and could be reclaimed while still in use. A GC correctness bug, traded for a performance win. The two things I had conflated: the cost was never the SIGNAL, it was the WAIT. One unresponsive thread consumed the entire 2,000,000-spin budget -- 267ms of a 280ms mark. So a thread with a failure history is now probed with a 20,000-spin budget rather than skipped. Healthy threads answer within about 200 spins, which is a hundredfold margin for one that is merely slow, at one percent of what a hang used to cost; and a thread that recovers is picked up on the very next cycle instead of up to 64 later. Verified across the GC suites, including GcUncooperativeThreadIntegrationTest -- the issue #5537 scenario this logic exists to serve: 6/6. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Boolean shares its kind with boolean and Character with char, so the direct writer treated both as primitives. Only the boxed form can be null, and both handled it wrongly in opposite ways: a null Boolean was unboxed by a ternary and threw NullPointerException, and a null Character went through String.valueOf(Object), which returns the four characters "null", and was then QUOTED -- so an unset field serialised as the string "null". The map path stores the value and lets JSONWriter see the null, emitting JSON null for both. Told apart by binaryName, which does distinguish them, with a temporary in each so a getter is not evaluated twice, and charValue() so String.valueOf resolves to the char overload rather than the Object one. The parity test carries both fields now, and they discriminate by construction: against the old code the Boolean case throws (a test error) and the Character case produces a quoted "null" against the map path's null (an assertion mismatch). 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:bb98371968
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…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".
Uh oh!
There was an error while loading. Please reload this page.
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".
Uh oh!
There was an error while loading. Please reload this page.
…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".
Uh oh!
There was an error while loading. Please reload this page.
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".
Uh oh!
There was an error while loading. Please reload this page.
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".
Uh oh!
There was an error while loading. Please reload this page.
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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…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".
| if(f == NULL) { | ||
| return -1; | ||
| } | ||
| return fflush(f) == 0 ? 0 : -1; |
There was a problem hiding this comment.
When the output targets a FIFO, device, or slow network filesystem, fflush() can block just like the bracketed fwrite() above, but this call leaves the VM thread active. On Windows, where CN1_GC_CAN_FORCE_STOP is disabled, a collection then waits indefinitely for that thread to reach a safepoint while the peer may never drain the stream. Wrap this flush—and the flushing fclose() path below—with the same CN1_YIELD_THREAD/CN1_RESUME_THREAD treatment used for reads and writes.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto 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