ParparVM: make the translator self-hosting, and fix what that exposed - #5766
Open
shai-almog wants to merge 67 commits into
Open
ParparVM: make the translator self-hosting, and fix what that exposed#5766shai-almog wants to merge 67 commits into
shai-almog wants to merge 67 commits into
Conversation
javac lowers a primitive class literal to a read of the boxed type's own TYPE
field, so `TYPE = int.class` inside Integer's initializer compiles to
`getstatic TYPE; putstatic TYPE` -- it reads the field it is initializing and
leaves it null. Integer, Long, Byte, Character and Double all declared TYPE that
way and all had a null one; Short, Boolean and Float had no TYPE at all; and
Void.TYPE was java.lang.Void, the wrapper, rather than void.
Nothing threw. Measured on a translated binary before this change:
TYPE Integer null=true TYPE Double null=true TYPE Void name=java.lang.Void
map size 2 of 6 m.get(Integer.TYPE) -> "JAVA_DOUBLE"
A Map keyed on them collapses onto the single null key, so every lookup answers
with whatever was stored last. The translator's own Util.ctypeMap/sigTypeMap are
exactly that shape, keyed on all nine, which is how this surfaced: it would have
typed every primitive alike and emitted syntactically valid C with every
primitive type wrong.
The JDK declares a native for this for the same reason, and so does this:
- nine scalar `struct clazz` objects in cn1_globals.m, with designated rather
than positional initializers so a future field added to struct clazz cannot
silently shift every value the way it would in the generated ones beside them
- java_lang_Class_getPrimitiveClass, taking an int code rather than the JDK's
String name -- this runs inside the wrapper class initializers, which are
among the earliest code in the process, and decoding a Java String here would
drag String.getBytes and the charset machinery into Integer's own clinit
- isAssignableFrom and isInstance now test primitiveType before calling
instanceofFunction, which indexes tables by classId; a primitive class
carries a sentinel classId that no table has a row for
__codenameOneParentClsReference has to be set to class__java_lang_Class as the
generated clazz objects do. CN1_CLASS_OF reads it to find the vtable when a clazz
is used as an ordinary object, which is what happens the moment one becomes a Map
key -- leaving it zero segfaults on the first hashCode(), well away from anything
that names it.
PrimitiveTypeIntegrationTest compares a translated run against a real JVM rather
than a hard-coded expectation, because the failure was self-consistent and silent:
only an independent reference catches it. Confirmed to fail when TYPE = int.class
is put back.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ByteCodeTranslator's own bytecode, plus ASM's, translates to C and compiles into
a native binary that performs real translations. vm/selfhost/build-selfhost.sh
builds it and verify-selfhost.sh compares its output against the JVM-hosted
translator's.
The point is validation. The translator is a ~37k-line program that exercises
collections, strings, file I/O, exceptions and the GC at scale, so running both
builds over the same input and diffing the emitted C is an end-to-end conformance
test of the whole VM -- one whose corpus grows on its own as the translator does.
It has already earned that: three defects fell out of it, each invisible to every
existing test because each was self-consistent on HotSpot.
- C label names came from identity hash codes. ASM's Label.toString() is
"L" + System.identityHashCode(this), so the emitted C was irreproducible; and
on ParparVM, whose identity hash is the object pointer narrowed to int and so
often negative, it emitted label_L-180306432001, which C reads as a
subtraction. Every method with a try/catch failed to compile. Labels are now
numbered per method in bytecode order.
- C local-variable declarations were emitted in HashSet iteration order, so the
same input produced different C. debugVarEntries had already had to learn this
for the debug side-table; the declarations had the same defect and now share
its comparator.
- Class.getResourceAsStream returned a hard-coded null on every ParparVM target.
It now consults resources linked into the executable through a weakly-defined
cn1FindResource -- which the generated resource table overrides on targets that
embed them -- and then a search path from CN1_RESOURCE_PATH.
JavaAPI grows only where ASM's bytecode forces it, because ASM is a jar we cannot
edit: Integer.rotateLeft, Double.doubleToRawLongBits, Float.floatToRawIntBits,
the three-argument Class.forName, and TypeNotPresentException. Everything the
translator's own source needed was removed from the translator instead:
- String.split/replaceAll are gone from it entirely (Util.splitLiteral,
splitWhitespace, collapseWhitespace, rewriteLocalObjectRefs). Declaring them in
JavaAPI would have collided with BytecodeComplianceMojo, which rewrites those
calls onto com.codename1.util.regex precisely because JavaAPI lacks them.
UtilStringHelperTest holds each replacement against the JDK original over ~6000
fuzzed inputs; it caught one real divergence, that String.split returns { s }
when the pattern never matches instead of dropping the trailing empty.
- The 24 two-argument System.getProperty calls go through Util.getProperty,
which uses the one-argument form JavaAPI does have and falls back to getenv.
That also makes the knobs work in a native build, which no -D can.
- The ~110 java.nio.file calls are plain java.io again. Parser and
ConcatenatingFileOutputStream already used those constructors under the
zero-findings SpotBugs gate, so both idioms already coexisted.
- java.util.zip is confined to ArchiveClassScanner and DebugSymbolCompressor,
and NativeSignatureVerifier's command-line half moved to
NativeSignatureVerifierCli. JavaAPI cannot gain java.util.zip: it is mirrored
by Ports/CLDC11, where the package does not belong. Splitting out the CLI also
removes the second main() that made ByteCodeClass refuse the translation with
"Multiple main classes".
vm/selfhost/stubs holds no-op replacements used only by the native build, for the
JavaScript target and those two zip users.
Gate D (the native translator against itself) passes. Gate A (JVM against native)
is at 245 of 247 files byte-identical, and binaries built from the two trees
produce identical output. The remaining two files are java_util_HashMap.c/.h,
where the native pass culls seven more methods and emits them as stubs; both
trees compile, link and run correctly, but the two runtimes should not disagree.
Not nondeterminism (gate D passes on both) and not identity-hash order (tested by
re-running the JVM under -XX:hashCode=2, byte-identical output). Written up in
vm/selfhost/README.md.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… memory
bench-selfhost.sh runs both translators over the same corpus, interleaved, taking
the minimum wall clock and the peak phys_footprint (never ps rss). It refuses to
print ratios unless the two emitted identical C.
On the self-hosting corpus -- ASM plus the translator's own classes, ~570 classes
-- in the documented release shape (-O3 -flto=thin), against JDK 8:
wall clock (min of 3) parpar 7.06s jdk8 1.17s jdk8 6.0x faster
peak phys_footprint parpar 1434MB jdk8 509MB jdk8 2.8x smaller
That is the opposite of the expectation on both axes, so it is worth being clear
that it is a real measurement rather than a mistake. /usr/bin/time -l independently
reports 1328 MB and 501 MB, agreeing with the sampled vmmap figures; building at
-O1 rather than -O3 -flto=thin changes nothing measurable, so code quality is not
the bottleneck; and the corpus is large enough that JVM startup is not carrying the
result.
The user-versus-real split locates most of the gap:
parpar 6.29 real 7.31 user -> ~1.2x parallelism
jdk8 1.13 real 6.09 user -> ~5.4x parallelism
The two burn comparable CPU. HotSpot spends it across cores on JIT compiler
threads and parallel GC, while the translated program is single-threaded, so the
6x is mostly concurrency ParparVM does not have rather than per-instruction code
quality.
Fixing an early error in the harness, since it is the kind that reads as a result:
the memory sampler took $! from a subshell wrapper and reported the wrapper's
~1.3 MB footprint for both arms. It now execs the translator so the pid is the
process being measured.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…entical
The self-hosted translator emitted seven java.util.HashMap methods as empty stubs
that the JVM-hosted one emitted in full. The cause was not the VM: it was the
blanket "Javascript*" exclusion in build-selfhost.sh, which stubbed
JavascriptNativeRegistry along with the two classes that genuinely cannot compile
against JavaAPI.
That class compiles fine, and -- as the comment at its call site in Parser says --
it is consulted on EVERY target, not just JavaScript, because the C natives use
some of the same methods as fallbacks. Its RUNTIME_DELEGATE_TARGETS lists
java_util_HashMap's getImpl, putImpl, removeImpl, containsKeyImpl and clearImpl;
answering false for them let the dead-code pass cull all five, and with them
cn1PutSlot and cn1MaybeGrow, which nothing else calls.
Found by instrumenting the cull decision and diffing the two runs: the five showed
up as "examined jvm=0x parpar=1x" -- the JVM never even reached the cull check for
them, because isRuntimeDelegateTarget had already made it `continue`.
The stub list is now driven by what is actually in stubs/ rather than by a name
pattern, so only the sources that cannot compile are replaced.
Gate A (JavaAPI corpus, 247 files) byte-identical
Gate A (self-hosting corpus, 797 files / 21.6 MB) byte-identical
Gate D (native against itself), both corpora pass
The second of those is the bootstrap gate, and it is a stronger statement than
GCC's three-stage comparison: there is no foreign compiler in the loop, so the
program really is identical and only the runtime executing it changed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ot the GC
The wall-clock gap to JDK 8 is almost entirely the allocator's backpressure
throttle. `sample` on a default run puts 64% of the process's samples in a single
stack, and the mutator is not marking or sweeping -- it is asleep:
Ldc.getValueAsString -> cn1BibopAlloc -> cn1BibopMaybeGc
-> cn1PacingPark (3491 of 5476 samples)
-> usleep -> nanosleep -> __semwait_signal (3475)
CN1_LOG_PACING_PARKS reports only TWO park events for the whole run, so each one
is seconds long.
Isolated by A/B, translating ~570 classes on a 64GB / 16-core host, release shape:
as shipped 6.7-8.7s 6 cycles 2 parks 1434MB
CN1_GC_TRIGGER_MB=32768 (no GC) 1.42s 3 cycles 0 parks
CN1_GC_PACING_CAP_MB=4096 1.45s 4 cycles 0 parks
growth clamp disarmed 1.39-1.52s 4 cycles 0 parks 1467MB
Collection itself is nearly free: with the clamp disarmed the collector still runs
its four cycles and the time matches disabling GC outright. Against JDK 8 that is
1.19x, ordinary AOT-versus-warmed-JIT territory, instead of 6x.
The mechanism, from cn1BibopPacingCap: it computes fm/8 -- 4GB on this host -- and
then clamps to `trigger * CN1_BIBOP_GC_MAX_CAP_MULTIPLIER` once
cn1PacingPastGrowthFloor() is true, which is a process footprint over
CN1_PACING_GROWTH_FLOOR_BYTES (512MB). Early in the run the trigger is still at its
own 24MB floor, so the ceiling is 24 * 8 = 192MB, matching the observed
minCapKb=196608 exactly. A program whose live set is ~1.4GB cannot stay inside a
192MB allocation window, so it parks against a collector that can never get under
it. That bound is calibrated for phone-sized heaps and has no scaling for a 64GB
host: it costs 5x throughput to save 2% of peak footprint here. Left alone, since
what it should scale with is a policy call for the VM owners; the reproduction is
one -DCN1_PACING_GROWTH_FLOOR_BYTES, documented in vm/selfhost/README.md.
One real defect found alongside it IS fixed: cn1RefreshFreeMemCache() had exactly
one caller, inside the mark cycle, so cn1CachedFreeMem stayed 0 until the first
collection and the cap sat at its 72MB floor through the window with the least
reason to throttle anything. Primed in cn1BibopDoInit now.
ProcessBudgetPacingIntegrationTest's control arm reports minCapKb=4194304 with the
fix and the 72MB floor without it, and its budget-bounded arm still engages
backpressure (legacyParks=52, boundedChecks=771), so the ceiling that bound exists
to enforce is untouched.
Gates A and D still pass byte-identical on both corpora after the change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…pool
Two fixes take the self-hosted translator from 6.0x slower than JDK 8 to 1.18x
slower than JDK 25 -- and 1.24x FASTER than JDK 8 -- on the self-hosting corpus.
Benchmarks now use JDK 25 as the reference; JDK 8 is kept only because it is what
the builders fork.
parpar 1.84s / 1443MB jdk25 1.56s / 516MB jdk8 2.27s / 502MB
1. The mutator slept instead of allocating, and would on any machine.
cn1BibopPacingCap computes a cap from available memory (fm/8, 4GB here) and then
clamps it to trigger * 8 once cn1PacingPastGrowthFloor() is true. That floor was a
flat 512MB. Early in a run the trigger is still at its own 24MB floor, so the
ceiling was 192MB -- confirmed by minCapKb=196608 -- and a program with a ~1.4GB
live set cannot stay inside a 192MB allocation window. It parked against a
collector that could never get under it: `sample` put 64% of samples in
cn1PacingPark -> usleep, from just two park events, each seconds long.
A fixed 512MB says the process has grown; it does not say the machine is under
pressure, and the bound exists for pressure. The floor is now
max(512MB, availableMemory/4). Where cn1_available_memory is the flat 100MB
placeholder (Linux, Windows, non-Apple fallback) the absolute floor still wins and
behaviour is bit-for-bit unchanged, and the floor can only rise, never fall, so no
constrained host becomes more permissive than it was.
This is the no-per-process-ceiling path only. Where a ceiling exists -- iOS's
dirty-memory limit, or an explicit budget -- cn1PacingPark takes the bounded branch
and never reaches cn1BibopPacingCap. ProcessBudgetPacingIntegrationTest covers both
halves and still passes: control arm minCapKb=4194304 with zero parks, bounded arm
holding a 120MB limit at a 60MB peak across 427 parks.
cn1RefreshFreeMemCache() also had exactly one caller, inside the mark cycle, so
cn1CachedFreeMem was 0 until the first collection and both the cap and this floor
sat at their absolute minimums during the window with the least reason to throttle.
Primed in cn1BibopDoInit.
2. Parser.addToConstantPool was O(n^2).
With pacing out of the way the main thread's profile was dominated by
constantPool.indexOf(s) -- a String.equals against every string already interned,
and the pool holds ~200k of them on a self-hosting translation. It was 32% of
main-thread samples across String.equals (11.2%), the list iterator (10.3%),
indexOf (6.2%) and ArrayList.get (5.1%). A HashMap side index answers the same
question; the list stays the source of truth so the emitted indices are unchanged.
Gates A and D still pass byte-identical on both corpora after both changes.
Still open: peak footprint is 2.8x JDK 25's. It is retained data, not garbage --
sweeping the trigger from 8MB to 256MB moves peak less than 15% -- and it scales
with the object graph rather than being a fixed cost (2.37x on a hello-world
corpus, 2.73x on the full one). The 16-byte object header and BiBOP size-class
rounding do not account for it, and compact strings are not the explanation either
since JDK 8 has none and still fits in ~500MB.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cn1HeapAccounting and cn1AllocCensus were written but called from nowhere, so
nothing in the tree could answer "what is the footprint actually made of". They
now run post-sweep (the same world-consistent point the GC verifier uses) and once
at exit, under -DCN1_ALLOC_CENSUS plus CN1_HEAP_REPORT at run time, so an ordinary
build is untouched. A batch program usually ends between collections, hence the
atexit report as well as the per-cycle ones.
The forward declarations sit outside the CN1_GC_VERIFY block: putting them next to
cn1GcVerifyHeap looked natural and compiled to nothing in a census build, since
that block is off.
build-selfhost.sh: -O3 now implies -flto=thin. That IS the documented release
shape, and measured over five interleaved rounds it is the only rung that beats
-O1 -- 1.45s against 1.61s for -O1, 1.70s for -O2 and 1.73s for bare -O3.
Benchmarking a plain -O3 binary and calling it the release build understates it,
which is too easy to do when the flag is left to the caller to remember.
First results on the self-hosting corpus (~570 classes), at exit:
bibop pages=12029 reserved=751.81MB live=749.62MB slack=2.19MB
legacy objects=729174 bytes=110.85MB
JAVA TOTAL live=860.47MB process peak phys_footprint=1467MB
Two things fall out immediately. Page-pool slack is 2.19MB, so fragmentation is
not the memory story. And the Java heap is 860MB of a 1467MB process, so roughly
600MB is not the Java heap at all and needs its own answer.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two defects found by the per-class allocation census, on a self-hosting
translation of the ParparVM translator.
IdentityHashMap allocated an Entry on EVERY next(), including key and value
iteration, where the entry existed only to have one field read back out of it and
then dropped: 1,366,140 of them, 43.7MB, all garbage. java.util.HashMap already
had separate key/value/entry iterators for exactly this reason -- its key iterator
reads the flat table directly -- and this map had been left on the older shape.
It now has the same split, with the generic MapEntry.Type callback used only for
entrySet, which is the one view where a caller can observe an Entry at all.
ArrayList's no-arg constructor called this(10), so every list allocated a
128-byte slot up front, including one that is never added to. It now shares a
zero-length array until the first growth. That growth allocates exactly ten, not
the twelve the general growth path picks: ten keeps a one-to-ten element list in
the size class it already occupied, and growing to twelve would have traded a win
on empty lists for a loss on the common case.
Measured together on the self-hosting corpus:
allocations 10,160,401 objects / 991MB -> 8,706,929 / 940MB
legacy-heap objects 729,174 -> 444,783
Java live heap 860MB -> 770MB
process peak 1467MB -> 1324MB
CollectionSemanticsIntegrationTest holds both against a real JDK rather than a
hand-written expectation, since these fail at the edges and are invisible when
they work: empty-list operations, all three growth paths, identity semantics, null
keys and values through each of the three views, iterator removal, and a rehash.
Confirmed to fail when the key iterator stops mapping the table's NULL_OBJECT
sentinel back to null.
HashMap was investigated and deliberately left alone. It eagerly allocates three
arrays at capacity 16 and looks like the same defect, but the maps in this
workload are populated rather than empty, so the table is not waste. Rebuilding
with a default capacity of 1 -- the cheapest probe for how much of it is wasted --
made everything worse, because the maps then regrow repeatedly:
default capacity 16 Object[] 1,324,987 int[] 213,725 live 770MB
default capacity 1 Object[] 1,802,249 int[] 452,356 live 882MB
Its growth is also post-insert by design, so the shared-empty-table trick that
works for ArrayList would leave the put path writing into the shared table.
Also recorded in vm/selfhost/README.md: String's `long nsString` field, which
backs the Apple targets' direct NSString mapping, costs nothing anywhere else.
sizeof(obj__java_lang_String) is 48 with the field at offset 40, and the fields
before it end at 36, so half of those eight bytes were padding already. Without it
the struct is 40 bytes, and BiBOP's size classes are 32, 48, 64 -- both land in the
same 48-byte slot. Removing it would save zero bytes per String and cost the Apple
targets a side table and a lookup.
Full vm/tests suite: 36 classes, no failures.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cn1AllocCensus answers churn -- what was allocated, which is what costs CPU. There
was nothing that answered retention -- what is still here, which is what costs
memory -- and those are different questions: an iterator allocated a million times
retains nothing, a cache allocated once retains everything.
cn1LiveCensus walks the BiBOP pages and the legacy heap after a sweep and reports
the heap by class: occupied bytes, object count, bytes each, and how many of them
the last mark proved reachable. Objects are charged what they OCCUPY -- a whole
size-class slot, a whole malloc block -- so the rows add up to the footprint and
rounding waste lands on the class that causes it. Classes are collected in a local
pointer-keyed table rather than read from cn1ClazzSet, which only exists under
CN1_CONSERVATIVE_GC_ROOTS.
Occupied and reachable are reported separately on purpose: "a million live
iterators" and "a million dead iterators still holding slots" call for opposite
fixes. Reachability is only meaningful in the post-sweep report -- it means
"carries the current mark", so at exit, long after the last cycle, almost
everything reads as unreachable whether it is or not. That trap is real: the exit
report says 6% reachable and the last post-sweep says 75%.
What it says about a self-hosting translation, all of it recorded in
vm/selfhost/README.md:
- The heap is genuinely live, not uncollected garbage: 295MB occupied and 75%
reachable at the last sweep. The run then ends at 769MB because only three or
four cycles complete in 1.4s while the mark thread sits at 97% CPU.
- Collecting harder does not fix it. -DCN1_GC_MARK_THREADS=4 more than doubles
the cycles (4 -> 9) and is slightly faster, but peak moves 1256MB -> 1243MB.
- vmmap puts essentially all of the process in malloc'd heap: MALLOC_LARGE
435.8MB dirty (the BiBOP arenas), MALLOC_SMALL 111.7MB, and 50.2MB of
MALLOC_LARGE (empty) -- freed but not returned. An earlier claim in the README
that ~600MB was "not the Java heap" was wrong; it compared an exit-time census
against the whole-run peak.
Two leads it opens without settling: per-object width against the JDK (String 73B
here against ~32B there), and 295,907 SimpleListIterator objects reading 72%
reachable at a fresh sweep, which a stack-local iterator should never be --
conservative stack scanning is on unconditionally and would explain it, but that
is a hypothesis and not yet measured.
Compile-gated on CN1_ALLOC_CENSUS and run-gated on CN1_HEAP_REPORT, so an ordinary
build is untouched. Gates A and D still byte-identical.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…riments
The live census reported "reachable", which conflated two different things and got
the answer backwards. Read post-sweep, an object stamped live by the sweep's grace
rule is indistinguishable from one the mark actually traced -- so a heap full of
fresh garbage read as a heap full of live data.
The census now runs PRE-sweep, the only point where the four reasons a slot is
still occupied are still distinguishable, and reports all four per class: traced
(the mark reached it), fresh (allocated since the mark, kept by grace), aging
(known dead, kept one more cycle) and dead (this sweep returns it).
On a self-hosting translation, at the last cycle:
occupied 4,441,347 objects 349MB
traced 47% fresh 30% aging 14% dead 9%
Only 47% of the occupied heap is traced live; the rest is held by collector policy
rather than by the program. Per class it is sharper -- char[] is 5% traced and 76%
fresh, almost pure churn caught between cycles.
experiments/PinProbe establishes the mechanism directly: three arms allocate and
drop 200,000 objects each -- shallow, under a 400-deep recursion, and with the
stack scrubbed -- and a dead object needs THREE cycles to have its slot returned
(grace while fresh, then aging, then reclamation). A translation completes three or
four cycles in 1.4s, so most of what it allocates is never eligible to be freed and
the heap grows towards total allocation volume: 940MB allocated, 1.3GB peak,
150-300MB genuinely live.
The same probe rules OUT the hypothesis it was built to test. Conservative stack
scanning does not pin dead objects: the marks are precise and all three arms behave
identically, so depth and stale stack words make no difference. Fragmentation is
ruled out too, at ~2MB of page-pool slack in 715MB.
Collecting faster helps but does not change the ratio, since the grace rule keeps
everything allocated since the last mark whatever the rate:
1 mark thread 3 cycles peak 1320MB
-DCN1_GC_MARK_THREADS=4 8 cycles peak 1172MB
4 threads + CN1_GC_TRIGGER_MB=24 7 cycles peak 1259MB
So the dominant lever is allocation churn, and cutting one allocation removes about
three cycles of occupancy rather than one object. The [ALLOC] census names where it
is: char[] 368MB, Object[] 196MB, String 77MB, SimpleListIterator 40MB.
Gates A and D still byte-identical; the census is compile-gated on CN1_ALLOC_CENSUS
and run-gated on CN1_HEAP_REPORT, so an ordinary build is untouched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A dead object needs three GC cycles to have its slot returned, because the sweep keeps it twice: once as fresh (never marked) and once as aging (mark == V-1, not traced this cycle). The first is load-bearing. The second is not obviously anything, and the history says what it is -- November 2014, 31528ec: - if(o->__codenameOneGcMark != currentGcMarkValue) { + if(o->__codenameOneGcMark < currentGcMarkValue - 1) { with the message "Delayed GCing of elements to prevent them from being collected due to a race condition with the GC thread". That collector had NO SATB barrier -- zero matches for satb or snapshot in the file at that commit -- so a mutator could hide a reference from the mark, and keeping an extra generation made the resulting lost-object race improbable rather than impossible. It has been inherited ever since, including by the BiBOP sweep, without a rationale anywhere in the tree. The cases that would need it today have their own guards. A page missing from the page index for one cycle is covered by the fresh grace rule, since its objects are mark == -1; the repeated miss that aging could not save either is exactly what cn1GcPageIndexStale skips the whole reclaim for, and that comment says so. CN1_GC_NO_AGING compiles the second cycle out. Evidence: - run-gc-verify.sh GREEN, and its three self-tests still detect their injected faults -- including the injected EARLY-FREE fault, which is precisely the failure this change could cause, so the gate is not vacuous for it - run-gauntlet.sh GREEN: 12 torture suites byte-identical to the host JVM, plus GC stress in cooperative and forced-signal thread-stop modes - self-hosting gates A and D byte-identical over 793 files - peak footprint 1334 -> 1322 MB and 1349 -> 1302 MB, about 2-3% Deliberately NOT the default. The win here is small because aging is only 14-16% of the occupied heap while fresh is 26-36%, so removing the second cycle moves those objects one cycle earlier in a run that only has three or four; a long-running application whose heap reaches a steady state would see closer to the full 15%. And vm/CLAUDE.md is explicit that a green verifier is necessary rather than sufficient around the SATB window: it could not open the residual window even with the barrier deliberately compiled out. Also recorded, since dropping it for the non-GUI Apple targets is an obvious thing to try: String's `long nsString` costs nothing to keep. sizeof(obj__java_lang_String) is 48 with it and really does fall to 40 without, but BiBOP's size classes are 32/48/64 so both land in the same 48-byte slot. Three runs each way put peak at 1240/1379/1340 MB with the field and 1290/1345/1341 MB without -- ranges that overlap completely. There is no effect to find, and removing it would cost the Apple targets a side table and a lookup for nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rth 2-3% Backs out d12892d entirely -- both the CN1_GC_NO_AGING switch and the change behind it. Keeping a compile switch for this was the wrong shape regardless: it is not debug code, so it would just be a second collector policy nobody runs. The history in that commit still stands: the second grace cycle is a 2014 pre-SATB workaround (31528ec, "Delayed GCing of elements to prevent them from being collected due to a race condition with the GC thread") and nothing in the tree records a reason for it. What the switch missed is that four later mechanisms have since been built ON the rule: - two java.lang.ref clearing sites that must use EXACTLY the sweep's liveness test; their comment spells out the failure as "FAILING to clear one the sweep frees hands get() a dangling pointer" - the fast-sweep page shortcut, whose gcGraceEpoch < V-1 bound is derived from the per-slot rule, and whose comment records issue 5425 when the two disagreed: "testing != V let it drop whole pages holding V-1 slots... 26,924 slots in one run. That is what left kept objects pointing into reclaimed memory" - the legacy and BiBOP sweeps ageing in step, so a matured Hashtable.Entry at V-1 is never kept while its page-resident payload at V-1 has already been freed The measurement that made it look safe was itself inconsistent: it changed the sweep and left the ref-clearing sites on the old rule, which IS the dangling-get() bug, and run-gc-verify.sh still came back green. So the verifier does not cover this coupling and a green result there was never sufficient evidence. Removing the rule properly means changing all four together and re-deriving the fast-sweep bound, for a measured 2-3% of peak. Not worth it; the allocation churn is where the memory is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n it Companion to the revert. The rule is vestigial in origin -- a 2014 pre-SATB workaround -- but the java.lang.ref clearing sites, the fast-sweep page bound and the legacy/BiBOP pairing have all been built on it since, and issue 5425 is what happened when two of them disagreed. Measured upside for removing it was 2-3% of peak. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Translating ByteCodeTranslator with itself turns a 37.6k-line real program into
a VM conformance test: the emitted C is a byte-exact expected value that costs
nothing to maintain, because it is whatever the JVM produced from the same
inputs. A defect that changes behaviour rather than crashing shows up as a diff
instead of passing silently. `.github/workflows/parparvm-selfhost.yml` runs it
nightly, on demand, and on a PR carrying the `selfhost` label.
The gates found a real VM bug and the profiler found several API implementations
that were slow for reasons that had nothing to do with the VM.
CORRECTNESS
Every generated __STATIC_INITIALIZER_X was textbook-broken double-checked
locking: __X_LOADED__ read with a plain load, written with a plain store OUTSIDE
the monitor, and class__X.initialized read the same way by inline guards that
skip the initialiser entirely and so never take the monitor. On arm64 a second
thread can observe the flag set while the stores that filled the vtable and the
classToInterfaceMap rows are still invisible, and then dispatch through a NULL
row. Observed as three identical SIGSEGVs at
classToInterfaceMap_java_util_NavigableMap[classId] + 0x8, reached from
TreeSet.clear; the translator is single-threaded in its own code but shares the
process with the GC thread, which also runs Java and so also runs initialisers.
Both flags are now release-stored and acquire-loaded, across all 391 classes.
The interface maps are calloc'd rather than malloc'd so a class id with no row
reads NULL instead of whatever the allocator last left there.
THROUGHPUT (5782-class corpus, min of 3 interleaved reps, phys_footprint)
GC run-ahead ceiling in cn1BibopPacingCap. The cap was a fraction of AVAILABLE
MACHINE RAM, and the trigger-derived clamp sat at 192MB (24MB x 8) for most of
a run, so the mutator parked on a cycle it could not help finish. Bounded from
both ends near 1GB, where the benefit saturates: 62.8s -> 27.4s, kernel time
33.4s -> 9.9s. Proportionate, so a phone or container is unaffected.
Class-init checks were unconditional CALLS at 74% of 2116 sites; the callee's
own first line already returns when the flag is set. Inline-guarded now that
the flag has acquire/release: 7.21% -> 0.12% of mutator self-time.
javac's `a + b` StringBuilder idiom is lowered to String.cn1ConcatN, the fused
path invokedynamic concat already used. Two allocations and no byte<->char
conversion against the builder's four plus two conversions. Only JDK 9+ emits
the indy form, so everything built at source 8 -- the core, every port, every
cn1lib -- reached none of it. 898 chains fused, StringBuilder allocation sites
3519 -> 2041.
API IMPLEMENTATION
AbstractList.SimpleListIterator.next had a try/catch per element to turn one
exception into another. ParparVM has no zero-cost exception tables, so that is
a setjmp per element in the hottest loop in the program, on top of virtual
size() and get() calls and an index recomputed as size() - numLeft.
ArrayList now has a direct-array iterator: iteration path 25.5% -> 12.4% of
mutator self-time, ArrayList.get 7.42% -> 0.55%, _setjmp to zero.
IdentityHashMap's iterator reached checkConcurrentMod() and hasNext() through
two more non-inlined calls per element, making four with the interface
dispatches. Inlined: checkConcurrentMod 2.69% -> 0.00%.
StringBuilder grew by 1.5x (inherited from Harmony) where OpenJDK doubles.
Growing to N chars costs N*r/(r-1) in abandoned arrays: 3N against 2N.
String.equals and String.compareTo had their fast path INVERTED -- memcmp only
when BOTH strings were UTF-16, the rare case, while two compact ASCII strings
took a per-character loop calling a helper that re-derived the base pointer and
re-tested the backing array's class every character. Corrected. Measured no
improvement: the cost there is call overhead, not the comparison. Kept because
the old structure was backwards, not because it is a win.
ByteCodeClass.generateCCode built each file in a fresh StringBuilder, and 95
copies of x.replace('/','_').replace('$','_') re-mangled the same owner per
emitted instruction. Reused buffer and a memo: char[] 1530MB -> 837MB.
ALSO
BytecodeInstructionIntegrationTest reflected on readFileAsStringBuilder, which
no longer exists: replaceInFile works on a String since the translator had to
compile against ParparVM's own JavaAPI, whose StringBuilder has no
indexOf/replace. Pointed at readFileAsString. 45/45 green.
VERIFIED
Gate D, Gate A and the negative control pass on the 797-file self-hosting
corpus, byte-identical. GC heap verifier clean over ~1e9 references.
check-cast-semantics and check-native-signatures clean.
KNOWN, NOT FIXED
A rare (~1/14) crash remains in interface dispatch, now a clean NULL-row fault
rather than silent garbage because of the calloc above. It is a bad class id
reaching a registered-row lookup, it predates this branch as far as the
evidence goes, and it is not reproducible under a debugger. Tracked separately.
The remaining iterator cost is two interface dispatches per element, each a
four-load pointer chase. Removing those means not making the calls -- lowering
for-each to an indexed loop in the translator -- which is a control-flow
rewrite and is deliberately left for its own change.
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: 118044dc12
ℹ️ 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
|
…, keep the pacing bound off placeholder hosts
Three defects, two of them found by review.
CLASS-INIT GUARDS TESTED THE WRONG FLAG (P1, review)
class__X.initialized is stored BEFORE __CLINIT__ runs, because it doubles as the
recursion guard for a <clinit> that touches its own statics. A guard on it can
therefore skip the initializer while <clinit> is still executing and hand back a
default for a static field that has not been assigned yet -- and the guards were
added to the static accessors, which is exactly where that is observable.
__X_LOADED__ is stored after __CLINIT__ returns and is the only flag meaning
"finished". Every guard emitted from ByteCodeClass and BytecodeMethod now tests
it. It is file-local, so generateCCode forward-declares it above the accessors;
the initializer block later in the same file is the definition.
The two guards emitted from TypeInstruction and FusedConstructor still test
initialized: they name a DIFFERENT class, whose flag is not visible from the
emitting translation unit. That is pre-existing, it is now written down where the
guard is emitted, and closing it needs a globally visible completion flag on
struct clazz.
THE CONCAT MATCHER TRUSTED THE OWNER, NOT THE STACK (P1, review)
It recognised appends by owner rather than by tracking which object was on the
stack, so it accepted
new StringBuilder(); POP; return existing.append(a).append(b).toString();
-- valid bytecode -- and took the appends on `existing` for appends on the
builder it had just allocated. Removing the allocation and the appends would then
leave the POP: an operand-stack underflow and a concat of the wrong operands.
The whole DUP/POP/SWAP family now ends the chain. Refused rather than reasoned
about, because a missed fusion is slower and a wrong one is memory corruption.
Cost: one site out of 898.
THE RUN-AHEAD BOUND SCALED OFF A NUMBER THAT IS NOT A MEASUREMENT (CI)
cn1_available_memory answers a flat 100MB wherever it cannot measure -- Linux,
Windows, the non-Apple fallback. cn1PacingGrowthFloorBytes only ever RAISES its
floor from that value, so a placeholder host is bit-for-bit unchanged. The new
run-ahead bound only ever LOWERS the cap, so scaling it by the placeholder
tightened pacing on precisely the hosts we know nothing about.
BibopPageFloorIntegrationTest went red on arm64 Linux, where fm/8 is 12.5MB,
while the same code passed on macOS where fm is real.
The bound now applies only where fm is a genuine reading, and answers 0 --
"leave the cap alone" -- elsewhere. macOS keeps the measured win: 25.1s.
ALSO
ArrayList.java joins the copyright exclusions as Apache Harmony source retaining
its Apache-2.0 notice, beside the other Harmony files. Swapping in the Codename
One header, which is what the gate was asking for, would have relicensed
third-party code. The three genuinely new files got the real header.
VERIFIED
Gate D, Gate A and the negative control pass byte-identical on 797 files.
BibopPageFloor and GcHeapIntegrity green. Copyright, control-character and
ASCII gates clean.
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Contributor
Cloudflare Preview
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01643b78c9
ℹ️ 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 181 screenshots: 181 matched. |
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 160 screenshots: 160 matched. Benchmark Results
Detailed Performance Metrics
|
…s native handleAppleOutput copied the runtime native to srcRoot/java_io_File.m and then called Parser.writeOutput, which on an Apple target emits one <class>.m per surviving class. A retained java.io.File therefore lands on exactly the name the copy had just used and CLOBBERS the native. What is left declares exists() calling existsImpl and has no definition of it, so the link fails: Undefined symbols: _java_io_File_existsImpl___java_lang_String_R_boolean, referenced from _java_io_File_exists___R_boolean in java_io_File.o on build-ios, build-ios-tv and build-ios-metal. build-macos passed in the same run, which is what identified the mechanism: MacOSNativeBuilder sets -DconcatenateFiles=true, routing class output into one buffer so the colliding name is never written. IPhoneBuilder sets it only under ios.superfastBuild, so the collision is live by default there. The comment in MacOSNativeBuilder has described this hazard for as long as that flag has been passed; the flag hides it rather than fixing it. The clean target already writes the same resource as java_io_File_runtime.c precisely so the two can coexist. This does the same on the Apple path. The generated class keeps java_io_File.m; the native becomes java_io_File_runtime.m; both are compiled, and the symbols resolve. Nothing else needed changing. The Xcode project collects sources by extension rather than from a fixed list, so the renamed file is picked up. And NativeSignatureVerifier reads the RESOURCE "/java_io_File.m" off the classpath, not this output path, so its scan is unaffected -- deliberately, per the comment on bundledRuntimeSources. VERIFIED Translating for the ios target now emits BOTH java_io_File.m (the generated class, referencing existsImpl) and java_io_File_runtime.m (the native, defining it); before, only the former survived. Gate D, Gate A and the negative control still pass byte-identical on the 797-file self-hosting corpus.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a57b3ecdbc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The guards tested __X_LOADED__, which is the only flag that means "<clinit> finished" -- that part was right, and the review comment that prompted it stands. What is wrong is what happens when a class never reaches the store. The initializer returns early, WITHOUT setting __X_LOADED__, whenever it finds class__X.initialized already true: the re-entrant case, and the case where another thread is mid-<clinit>. It also never reaches the store if __CLINIT__ throws. Any class left in that state has __X_LOADED__ == 0 permanently, and with the guards in place EVERY subsequent static access and every allocation calls the initializer, takes the class monitor, finds initialized true and returns. Not a hang -- a monitor acquire on a path that used to be a predicted-not-taken load. MEASURED: the hello screenshot suite stops after 145 of 166 screenshots on Linux, in three runs across x64 and musl, at 699s and 1052s elapsed, with no crash, no OOM and no bad_alloc in the log. A passing master run reports CN1_HELLO_SUITE_PNGS=166. The stop lands in a different test each time but always at the same count, which is what a uniform slowdown looks like rather than a hang at one place. I had previously attributed those failures to the pre-existing flake in that workflow. That was wrong: the flake is real and does hit other branches, but it stops at a different count (82 on master), and matching on the symptom string hid a regression of my own. So the guards come out. What stays is the fix they were built on top of, which is independent and still wanted: __X_LOADED__ and class__X.initialized are release-stored and acquire-loaded in all 391 classes, and the interface maps are calloc'd. Those close the double-checked-initialization race that produced the SIGSEGV at classToInterfaceMap_java_util_NavigableMap[classId] + 0x8. The 7.2% of mutator self-time the guards were worth needs a design that cannot leave the flag unset -- a third state, or setting it on the already-initialized path once "another thread finished" can be told apart from "this thread is re-entrant". That belongs in its own change, with the suite as its gate. VERIFIED 0 inline guards emitted; 391 acquire fast paths and 391 release stores retained; 39 interface maps still calloc'd. Gate D, Gate A and the negative control pass byte-identical on the 797-file self-hosting corpus.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b236ee39c2
ℹ️ 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 independent fixes in the allocation-pacing and collection paths.
cn1PacingPark tested threadBlockedByGC, then ran a mutator assist, then
looped with a bare `continue`. The assist can take a long time, and a GC
that requests a stop while it is running found the thread still marked
active with no safepoint ahead of it on that path -- so the collector
waited out its 250ms timeout and force-stopped the thread instead. That
is what the iOS packaging leg was reporting: the screenshot suite ran to
completion and then could not emit SUITE:FINISHED, with
[GC] force-stopped thread 3 after 250000us at a safepoint it never
reached (2 so far) ... (16 so far)
and the child finally killed with SIGTERM. Re-check the flag once the
assist returns and park properly if it is set.
Separately, ArrayListIterator was declared private. A private inner class
whose constructor is reached from the outer class makes javac synthesise
an access bridge and an ArrayList$1 marker type, so every iterator() paid
an extra class plus an aconst_null for the bridge argument. Package
private is invisible outside java.util either way. The self-hosting
corpus drops from 797 emitted files to 795 -- the .c/.h pair for the
synthetic that no longer exists.
Gates D and A stay byte-identical over the 795-file corpus, with the
negative control still detecting an injected corruption.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ".." guard added in the previous commit split on '/' only, which leaves the
traversal open on the one platform whose separator is the other one: nothing
stops a caller passing "..\..\etc\passwd", and File("root", that) escapes on
Windows exactly as the '/' spelling does. Resource names are '/'-separated by
specification, but a guard that trusts the specification is not a guard.
Checked against ten shapes, including the two that must NOT be refused --
"a/..b/c", where ".." is a prefix of a real segment rather than a segment, and
"..." -- since an over-eager contains("..") would reject both.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25159f2aa9
ℹ️ 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".
…forces GC suite (1 marker, arm64) failed on the previous push: GcOverflowSpiralIntegration Test peaked at 2159916KB against a 2GB limit, with a live set of a few hundred bytes. The test and the workflow are both master's, unmodified by this branch, and master passes them, so the regression is this branch's. This branch had changed the footprint at which pacing starts applying from a flat 512MB to max(512MB, fm/4). The test pins the free-memory reading at 32GB, so the floor became 8GB: the growth test never fired, pacing never engaged, and the peak was bounded only by the 1GB run-ahead allowance. The shape of the failure is the tell -- the FOUR-marker arms passed on both arches and only the one-marker arm failed, which is what a bound that holds only while the collector is fast looks like. A slow collector is the case the bound exists for. Only the arming point moves back. The capCeiling raise is the other half of the same idea and STAYS, because it is measured to help where it matters: on a 5782-class translation, master's 192MB clamp peaked HIGHER than the 1GB one (9736MB against 8325MB) and took twice as long (46.3s against 23.8s). Withdrawing that as well would have made the workload this branch exists to speed up both slower and larger. I was about to, and the measurement table above the clamp is what stopped me. Not re-tuned to sit just under the threshold: the margin would be a few percent on a shared runner, which is a flake rather than a fix. The scaling wants its own change, with the measurement that justifies it and a decision about what the enforced bound should be. HONESTY ABOUT THE VERIFICATION, because a number here is easy to misread: an A/B of this function on an uncontended arm64 Mac measured 107904KB with the constant against 109792KB with the scaling -- identical. Neither arm reaches even the 512MB floor, so the clamp is never armed in EITHER and the value under test does not participate. I first read the single "with fix" figure as confirmation; it was not, and the A/B is what caught it. This change is reasoned from the CI failure and is a revert to master's own constant. The CI leg is the only thing that can actually test it, and the code now says so. Gates D and A pass, 800 files byte-identical, with the negative control. Local vm suite: 567 tests, 0 failures.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df53b707d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The previous commit withdrew half of this branch's pacing work and CI answered
with the other half of the problem. GcOverflowSpiral went green -- peakKb=456216
against the 2159916KB that failed -- and BibopPageFloorIntegrationTest went red in
the same job: after dropping a 261492KB live set the footprint only fell to
225396KB against a 143820KB budget, so the pages were not handed back. That test
passed on the push before, with the scaling in.
So the two halves pull in opposite directions and each reds a test master passes:
scaling in GcOverflowSpiral 2159916KB against a 2GB limit (clamp never armed,
floor was fm/4 = 8GB against the pinned 32GB reading)
scaling out BibopPageFloor does not release (capCeiling raised 192MB -> 1GB,
so the mutator runs far enough ahead that pages are not returned)
Two red tests traded back and forth is the signal to stop tuning, so the whole
optimization is out rather than the half I could argue for. cn1BibopPacingCap now
diffs IDENTICAL to master, and cn1PacingRunAheadBound and its two defines are
gone. cn1PacingGrowthFloorBytes stays as a function returning master's constant,
because the record of what was tried belongs somewhere a future attempt will find
it.
I argued in the last commit for KEEPING the capCeiling raise, on a measurement
that is still true: on a 5782-class translation the 192MB clamp peaked higher
(9736MB against 8325MB) and took twice as long (46.3s against 23.8s). It is still
not worth a test master passes. That speedup is one of the two things this branch
set out to buy and it is now deferred, which is a real cost and not a tidy
outcome -- but it needs an environment that reproduces BOTH failures, and this
machine reproduces neither: an A/B measured 107904KB against 109792KB, identical,
because neither arm reaches even the 512MB floor.
Verified here: gates D and A pass, 800 files byte-identical with the negative
control, and both GC tests pass locally (peakKb=111392; the floor probe releases).
Local passes are weak evidence for the pacing question specifically -- see the
comment -- but they do rule out a build or link break.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66d247a969
ℹ️ 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".
…p rendering
Three screenshot legs -- Windows, the Windows cross-compile, and the third
comparison -- reported ValidatorLightweightPicker changed. Decoding both PNGs, the
content is IDENTICAL and shifted up about four pixels: everything above the field
stack and everything below it is byte-identical, and every differing row matches
an actual row at a negative offset. A layout shift, not a rendering fault. A
caller that had been falling back on a built-in default now had a file.
The cause is a claim I made in this method's own javadoc and did not check:
"That keeps every existing target unchanged -- getResourceAsStream returned a
hard-coded null before this existed, so nothing can regress, only start
working."
On master this method is `return null` on EVERY ParparVM target -- the whole
resource machinery is this branch's -- so no application has ever received
anything from it and every caller has always taken its not-found path. "Only start
working" assumed each of those paths was strictly worse than having the resource.
Three ports disagreed. A method that previously always failed cannot be given
results without moving behaviour, and that is the opposite of "nothing can
regress".
What self-hosting actually needs is the FILESYSTEM tier, and that tier is opt-in:
it answers only when CN1_RESOURCE_PATH names a search root, which the self-hosted
translator sets and no application does. So only the embedded tier could return
non-null to an app, and only the embedded tier is withdrawn. An application now
sees exactly what master gives it, and the translator still finds the C runtime it
copies into its output.
Removed as a chain rather than left inert, since a native with no Java caller is
what NativeSignatureVerifier reports as an ORPHAN: the cn1EmbeddedResource native
and its declaration, the weak cn1FindResource, and the strong overrides the
Windows and Linux embedders generated. The id table is still built and linked, and
a note where each override used to be says that wiring those two halves together
is the whole of the future change. Letting applications read their own embedded
resources is a good feature; it wants a change where the screenshot baselines it
moves are the point rather than the fallout.
Also restores .github/workflows/linux-build-run.yml to master EXACTLY. I had
reverted it against the pre-merge merge-base, which silently deleted content
master added in #5750 (CN1_LINUX_FULL_DEBUG and the gdb install) -- reverting to a
base that has since moved, the same shape as the rerere hazard.
Gates D and A pass, 798 files byte-identical with the negative control. The file
count drops from 800 because ByteArrayInputStream is no longer reachable and the
cull drops it, which is the correct consequence.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d2a8f0922
ℹ️ 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 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
…eenshot The previous commit said the embedded-resource tier caused the Validator LightweightPicker screenshot difference. It did not. The difference persists on 8d2a8f0 with that tier removed, so the claim in that message and in the comment it left behind was wrong, and the cause is still open. The tier stays withdrawn on its own merits rather than that one: handing every caller a resource where master hands it null is a behaviour change for every shipping application, and it is a separate feature from self-hosting, which needs only the opt-in filesystem tier. But that is a scope argument, not evidence, and the comment now says so. Ruled out so far, each by looking rather than reasoning: the Windows resource table generated byte-identical to master's apart from an unused #include; the .rc file is identical IdentityHashMap only MigLayout and SensorSession use it, and this form uses neither translator output diffed master's translator against this branch's over an identical corpus: every difference is an intentional fix -- deterministic labels in place of identity-hash ones, deterministic local-variable declaration order, and the class-init guard moving to __X_LOADED__ with acquire/release. Re-entrant class init is still correctly guarded, because the initializer re-checks .initialized inside a reentrant monitor. Still open: the ArrayList specialised iterator, and the iterator lowering's devirtualization. Both are reachable from layout code and neither is ruled out. Worth recording that the master-versus-branch translator comparison is a gate this branch never had: verify-output-neutral.sh only ever compared the branch against ITSELF, so it could not see a difference from master. That comparison is what ruled the translator out here, and it should exist as a script rather than as something reconstructed by hand each time.
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
…t guard Two separate things, one push, because CI is the only instrument that reaches either. THE FIX, which stays: build-ios failed the native warning census with exactly one kind out of baseline -- runtime|java_io_File_runtime.m|-Wshorten-64-to-32 -- [files count] is NSUInteger, 64-bit, handed to allocArray's JAVA_INT length and then re-sent as the loop bound on every iteration. It is narrowed once and explicitly now, and the loop runs on the narrowed value so bound and index share a type. Verified with clang -Wshorten-64-to-32 -Wall: zero warnings. Worth saying why this appeared on THIS branch rather than master: the file was never compiled on the Apple targets before, because the translated java_io_File.c overwrote it -- the collision this branch fixes. Making a dead file live exposes its warnings, which is a real consequence of a real fix and not noise. THE PROBE, which comes back out either way: the inline class-init guard goes back to master's `class__X.initialized` at both emission sites, leaving the rest of the branch alone. Four native screenshot legs report ValidatorLightweightPicker as a ~4px layout shift with identical content, and the JAVASCRIPT screenshots PASS. That split is the reason for this particular probe: JavaScript shares this branch's JavaAPI and its whole front-end optimizer -- the iterator lowering included -- and differs only in the emitter and the runtime. So the JavaAPI suspects and the lowering are both weakened, and a C-emitter change is the place to look. Of those, this is the one with semantics rather than formatting. Master's guard lets a thread proceed once .initialized is set, which happens BEFORE the clinit body runs; this branch waits for __X_LOADED__, set after it completes. A static holding a computed metric read mid-initialization would differ by exactly the kind of small constant this shift looks like -- and note which way that cuts: if this probe goes green, the GOLDEN encodes a read of a partially-initialized class, and the question becomes which rendering is correct rather than how to restore the old one. Ruled out before spending a cycle on it, each by looking: the Windows resource table (generated byte-identical to master's bar an unused include, .rc identical), IdentityHashMap (only MigLayout and SensorSession use it), the java_io_File copy on the clean/Linux/Windows path (identical to master -- only the Apple variant is this branch's), and every other translator output difference (deterministic labels and local-variable order, which cannot move layout).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3550d2e9c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The bisect probe came back green on two legs, so the inline class-init guard is
what moved ValidatorLightweightPicker by four pixels on all four native ports.
This keeps the fix and drops the part that caused it, which turns out to be a part
that was never needed.
The emitted initializer orders its flags like this:
class__X.vtable = malloc(...); __INIT_VTABLE_X(...);
class__X.initialized = JAVA_TRUE; <- BEFORE the Java <clinit>
java_lang_X___CLINIT____(...);
__X_LOADED__ = 1; <- AFTER
The defect was VISIBILITY, not the flag's position: a plain load of .initialized
let a thread see it set while the vtable and classToInterfaceMap rows it describes
were still invisible, which is the SIGSEGV this branch chased. Moving the guard to
__X_LOADED__ fixed that -- and, as a side effect nobody asked for, also moved the
gate past the whole Java <clinit>, so other threads are released at a strictly
later point than master releases them. That second effect is what the screenshots
saw.
So the guard goes back to .initialized and gains only the ordering it was missing:
master .initialized, plain load vtable race broken, timing baseline
this branch __X_LOADED__, acquire race fixed, timing MOVED
now .initialized, ACQUIRE race fixed, timing baseline
It pairs with the release store already emitted beside the vtable setup, so the
publication is correct in both directions. Verified in generated C: 34 inline
sites acquire-load .initialized, against the matching
__atomic_store_n(..., JAVA_TRUE, __ATOMIC_RELEASE).
__X_LOADED__ keeps its acquire load at the TOP of the initializer, where it is a
fast path and not a gate: reaching it means the clinit has completed, so returning
early is correct and costs no monitor.
This is why the choice I was about to put up -- reseed twelve per-port goldens, or
drop a fix that took the Windows cross leg from 173/11 to 184/4 -- was a false one.
Both horns came from my own fix being broader than the bug.
Gates D and A pass, 798 files byte-identical with the negative control.
… LOADED extern
verify-output-neutral.sh could not have caught the codegen change that cost this
branch a bisect, and the reason is structural: its two modes run the SAME
translator twice, so a change present on the branch is present on both sides. It
reported neutral because it WAS neutral -- against itself. Four native screenshot
legs found what it could not.
The new `vs-master` mode runs this branch's translator and master's over one
corpus compiled against MASTER's JavaAPI, so JavaAPI is held constant and the
translator is the only variable. Deterministic label names and local-variable
declaration order are normalised out; everything else is a real codegen change and
has to be a deliberate one.
Running it immediately earned its keep. It reported 230 differing files, and one
delta in them was no longer needed: __X_LOADED__ was made non-static with an
extern in every class header purely so the inline guards could reach it across
translation units. Those guards test .initialized again, and nothing outside a
class's own .c reads the symbol -- verified by scanning the emitted tree for any
file referencing another class's LOADED flag, which finds only a comment. So it
goes back to internal linkage and the header extern goes away: one exported symbol
per class removed from every generated project.
That halves the delta to 115 files, and what remains is exactly three annotations:
- if(__X_LOADED__) return;
+ if(__atomic_load_n(&__X_LOADED__, __ATOMIC_ACQUIRE)) return;
- class__X.initialized = JAVA_TRUE;
+ __atomic_store_n(&class__X.initialized, JAVA_TRUE, __ATOMIC_RELEASE);
- __X_LOADED__=1;
+ __atomic_store_n(&__X_LOADED__, 1, __ATOMIC_RELEASE);
Memory ordering and nothing else -- no semantic or timing difference from master.
The comment in emitClassInitializer still described the withdrawn design, saying
the inline guards test __X_LOADED__. It now says what they actually do and why
guarding on the completion flag was more than the visibility defect required.
Verified: gates D and A pass, 798 files byte-identical with the negative control,
and the vm suite is 567 tests with no failures. CI has already confirmed
screenshot-comment (x64) green on the narrowed guard.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f20c895eb8
ℹ️ 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".
Seven findings, all against code written today, and three of them against
verify-output-neutral.sh itself -- the script added hours earlier precisely to
catch this class of problem. Each was real.
vs-master could not fail, in three independent ways:
* it walked MASTER's file listing, so a generated file the branch emits and
master does not was never visited and n stayed 0. It now walks the union of
both trees and reports ONLY IN BRANCH / ONLY IN MASTER.
* it replaced every label_L<n> with one token, which makes a jump RETARGETED
from one existing label to another compare EQUAL -- exactly the regression it
exists for. Demonstrated on a fixture: the old normalisation calls the
retargeted case identical; the new bijective one, renumbering by first
appearance, calls it different and still absorbs the identity-hash-to-
sequential rename.
* it printed its findings and exited 0, so every caller read a real mismatch as
a pass. It exits nonzero now.
That last one is this project's own rule -- a check satisfiable by nothing
happening is no check -- broken inside a checker written to enforce it.
Fixing the labels exposed a fourth problem no reviewer raised: the label number
also leaks into catch_L<n>, restoreToL<n> and tryBlockOffsetL<n>, so every
try/catch-bearing file would have reported as changed forever and buried any real
difference in permanent noise. All four spellings now share one numbering, and the
remaining delta is exactly the three intended memory-ordering annotations per
class.
The other four:
Class.cn1FileResource ABANDONED the search when an earlier root held an
unusable candidate. exists() is true for a directory, and the IOException path
returned null instead of trying the remaining roots, so a valid resource behind
a bad one was unreachable -- which defeats the point of a search path. Now
isFile() and continue.
build-selfhost.sh watched only '*.java' for staleness while the translator
carries its C runtime as CLASSPATH RESOURCES. Verified: a cn1_globals.m edit was
missed entirely by the old guard, so the self-hosted binary embedded an obsolete
runtime while the JVM side used the new one -- a Gate A divergence that points
at the VM and is not one. The guard now covers every file, and the staging
copies every non-Java resource instead of a hand-listed four that had already
drifted.
bench-selfhost.sh sampled memory ONCE regardless of ROUNDS while its header
promised the maximum of N samples, so one noisy run could decide the reported
ratio. It now samples every round and reduces with max.
bench-selfhost.sh also hard-coded /Users/shai/.../azul-25/bin/java, so anyone
else running the documented command died under set -e while BUILDING the arm
list, before a single measurement. JDK 25 is resolved from JDK_25_HOME or PATH.
Verified: gates D and A pass, 798 files byte-identical with the negative control;
vs-master exits 1 with 119 files, all of them the intended annotations; the
staleness guard flips on a .m edit; the label canonicaliser distinguishes a
retarget from a rename; vm suite 567 tests, 0 failures.
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c67b53ef8
ℹ️ 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".
… answer Three findings are correct about the facts and still should not be built. Leaving them as open threads makes that look like an oversight; the reasoning belongs where the next reader hits the code, so it is written there and the threads are closed. Class.forName's initialize flag. Honouring it needs a way to reach a class's static initializer from its Class object, and struct clazz carries no such pointer -- only newInstanceFp and enumValueOfFp. Adding one is a field on EVERY class in EVERY application, for a flag whose only in-tree caller is ASM, which passes false. Cost paid by every app, benefit claimed by none. Emit the pointer if a real caller ever appears. NativeSignatureVerifier's missing main. Nothing names it as an entry point -- check-native-signatures.sh invokes the Cli and no document spells the old command -- and adding a delegating main recreates the edge the split exists to remove: the verifier would reference the Cli, and the Cli reaches java.util.zip, which JavaAPI cannot gain while Ports/CLDC11 mirrors it. A second main also brings back ByteCodeClass's "Multiple main classes" refusal. Relative resources on a NESTED class. Fixing it at the call site means guessing where the package ends, and every guess is wrong for some real input: a package may be named like a class, and a class name may contain '_'. A guess turns today's harmless miss -- a directory named after a class does not exist, so the lookup returns null exactly as it did before this method was implemented -- into a confident wrong answer. The defect is that getName() is lossy, because ParparVM builds the runtime name from the MANGLED form; it is fixed there or not at all.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26d6b04657
ℹ️ 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".
…xit census The final resolve pass closed FIVE threads, not the three I had triaged -- two arrived between the query and the mutation and I closed them unread. That is the bulk-resolve mistake, and both turned out to be real, so the fixes land here and the threads are now resolved against code that actually exists. bench-selfhost.sh gave two arms the same name, and this one is MY regression from an hour earlier. Discovering JDK 25 from JDK_25_HOME or PATH instead of hard-coding it means the fallback and JDK_8_HOME can both be Java 8, so both arms label as jdk8; every tree, log and diff is filed under the name, so the second arm's `mv` lands INSIDE the first arm's directory and the correctness check then compares a tree with its own nested copy and reports a divergence that is an artefact of naming. Names are deduplicated (jdk8, jdk8#2) and each arm now prints the executable it resolved to, so a suffix is never a mystery. Checked against three same-version arms. cn1BibopExitReport walked the heap under a live collector. atexit runs with the collector still going, and cn1HeapAccounting/cn1LiveCensus/cn1AllocCensus read allObjectsInHeap, object headers and non-atomic page fields -- exactly what a sweep clears, reuses and frees. So the diagnostic could report corrupted totals or dereference a reclaimed object in the batch-program exit case it was added to measure, which is the one case where its numbers would be believed. It now waits for gcCurrentlyRunning to clear, following the shape already used for the pending-table stall. Two properties on purpose: the wait is BOUNDED, because a diagnostic must not turn a hung collector into a hung exit; and on expiry the census is SKIPPED with a message rather than run anyway, because a report read off a heap being swept is worse than no report -- it looks like data. My first attempt called cn1GcRequestStopAndJoin(), which does not exist. Caught by grepping for it rather than trusting that a plausible name was a real one. The replacement is compiled WITH -DCN1_ALLOC_CENSUS against a cleared baseline (zero errors both ways), because the normal build never compiles that block and a green build would have said nothing about it. Gates D and A pass, 798 files byte-identical with the negative control; vm suite 567 tests, 0 failures.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a59f2fa12
ℹ️ 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".
… label diffs Two findings, both mine, and the first is the more serious. CN1_NATIVE_VERIFY never reached the translator. parparvm-selfhost.yml sets it to strict at workflow level, and every Gate D/A process is launched through `env -i`, which starts from an EMPTY environment -- so NativeSignatureVerifier.mode() saw nothing and defaulted to OFF. The gate has been reporting a configuration it was not running in. Same shape as the gdb install that never ran and the -D handed to a CMakeLists that did not declare the variable: armed-looking and inert. Forwarded explicitly at all three env -i sites, and FORWARDED rather than hard-coded, so a local run with nothing set behaves exactly as before. Checked directly: the variable now arrives as [strict] where it previously arrived as []. The label canonicaliser generated FALSE POSITIVES. The branch numbers labels with a method-local counter, so L0 and L1 recur in every method, while master derives them from ASM identities that are distinct across a whole file. A file-wide map folds the branch's second L0 onto the first method's token and master's onto a fresh one, so identical output reports as a code-generation difference. It resets per generated function now, which is the scope the names are actually minted in. Demonstrated in both directions on a fixture: file scope calls identical code DIFFERENT, per-function calls it equal, and per-function still catches a jump retargeted from one existing label to another. That is the third distinct defect in this one script -- the first version could not fail at all, the second hid retargets, the third invented differences -- and all three were found by review rather than by the script's own use. Worth remembering before trusting the next checker written here. Gates D and A pass, 798 files byte-identical with the negative control; vs-master exits 1 on its 119 intended annotation files; vm suite 567 tests, 0 failures.
The RUNTIME pattern reached grep -E as \\. before each dot, which in an extended regular expression means "a literal backslash followed by any character" -- so none of cn1_globals.[ch], nativeMethods.c, cn1_intrinsics.h, java_io_File_runtime.c or cn1-source-manifest.txt was ever excluded, and the four copied runtime files this branch edits ON PURPOSE were counted as code-generation differences. A single backslash is what the shell value needs; the doubling came from the python heredoc that wrote the line. This is the gap I had already seen and not chased. The hand classification counted 115 differing generated files and vs-master reported 119, and I recorded the difference as uninteresting instead of asking what the extra four were. They were exactly these. Confirmed by the fix: the count drops 119 -> 115 and no runtime file is listed. Fourth defect in this script. The pattern across all four is the same -- each one made the gate quieter or noisier than the truth, and none was found by running it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d669466929
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+5591
to
+5595
| while(gcCurrentlyRunning && waitMs < 2000) { | ||
| usleep(1000); | ||
| waitMs++; | ||
| } | ||
| if(gcCurrentlyRunning) { |
There was a problem hiding this comment.
Fresh evidence in the updated code is that the new “quiesce” step only waits until gcCurrentlyRunning is momentarily false; it does not stop or join the GC thread. In a CN1_ALLOC_CENSUS run with CN1_HEAP_REPORT, System's GC loop unconditionally calls gcMarkSweep() again after each idle period, so it can set the flag again immediately after this check or while the three heap walks execute, recreating the race with reclaimed objects and mutable page fields. Disable and join the collector, or hold synchronization that prevents another cycle from starting, before running the census.
Useful? React with 👍 / 👎.
…e census Two fixes, one of them undoing my own mistake. The gdb install. This run hit a SIGSEGV (exitValue=139, 82 of 100 screenshots, stopped in SheetScreenshotTest) and produced NO backtrace, because the step runs under `working-directory: vm` and invokes `bash scripts/ci/apt-get-install.sh gdb` -- which resolves to vm/scripts/ci/apt-get-install.sh, does not exist, and is swallowed by the trailing `|| echo`. gdb has never been installed on that leg, so every post-mortem it has ever run printed nothing while uploading a core nobody could read. 4333174 on this branch fixed that. I then reverted the whole workflow to master to drop my crash-hunt scaffolding and took the fix with it -- over-reverting, the same shape as reverting against a stale base earlier. Only the absolute path comes back; none of the scaffolding does. The exit census. The previous "quiesce" was check-then-act and did not close the race the review named: System's GC thread runs `while(gcShouldLoop) { gcMarkSweep(); wait(idle); }`, so waiting for gcCurrentlyRunning to fall leaves it free to raise the flag again in the gap before the walks start, or while they run. gcShouldLoop is cleared FIRST, so no new cycle can begin, and only then is waiting out the in-flight one sufficient. Set directly rather than through System.stopGC(): this runs from atexit, where calling back into Java is a larger promise than a diagnostic should make. The symbol is the generated set_static_java_lang_System_gcShouldLoop, checked against the emitted header rather than guessed -- I invented a plausible-looking cn1GcRequestStopAndJoin() earlier today and it did not exist. Compiles clean with and without -DCN1_ALLOC_CENSUS against a cleared baseline, since the normal build never compiles that block.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8947134b51
ℹ️ 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".
| // should make. The GC thread observes it on its next loop test -- immediately if | ||
| // it is idling, after the current cycle if it is collecting -- and exits, which | ||
| // is exactly the ordering needed here. | ||
| set_static_java_lang_System_gcShouldLoop(JAVA_FALSE); |
There was a problem hiding this comment.
When a CN1_ALLOC_CENSUS batch exits with CN1_HEAP_REPORT, storing false here does not guarantee that the collector has stopped: the GC thread can already have evaluated the while (gcShouldLoop) condition in System.java:104 and be preempted before gcMarkSweep() sets gcCurrentlyRunning, or its initial-delay path can subsequently overwrite the flag with true at System.java:88. This handler then observes gcCurrentlyRunning == false and starts walking the heap while that pending cycle resumes and sweeps it. Fresh evidence beyond the prior report is that the final revision added this stop store but still has no termination acknowledgement or join; wait for the GC thread to exit (or synchronize against cycle startup) before running the three census walks.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Rationale is in the commit message and in code comments; this body is a pointer only.
What this is. ByteCodeTranslator now translates itself, and
.github/workflows/parparvm-selfhost.ymlcompares the C it emits against the Cthe JVM-hosted translator emits from the same inputs. A 37.6k-line real program
becomes a VM conformance test whose expected value costs nothing to maintain.
Nightly,
workflow_dispatch, and opt-in on a PR via theselfhostlabel -- afull run builds the translator twice and translates the corpus several times, so
it does not belong on every PR.
A real VM bug fell out of it. Every generated
__STATIC_INITIALIZER_Xwasbroken double-checked locking --
__X_LOADED__plain-loaded and plain-storedoutside the monitor, and
class__X.initializedread the same way by inlineguards that skip the initialiser and so never take the monitor. On arm64 a
second thread can see the flag set while the vtable and
classToInterfaceMaprows are still invisible. Observed as three identical SIGSEGVs at
classToInterfaceMap_java_util_NavigableMap[classId] + 0x8fromTreeSet.clear.Acquire/release on both flags across all 391 classes; interface maps
calloc'd.Measured (5782-class corpus, min of 3 interleaved reps,
phys_footprint):62.8s -> 27.4s wall, kernel time 33.4s -> 9.9s. Class-init checks 7.21% -> 0.12%
of mutator self-time; iteration path 25.5% -> 12.4%;
checkConcurrentMod2.69% -> 0.00%;
char[]1530MB -> 837MB.Verified. Gate D, Gate A and the negative control pass byte-identical on the
797-file corpus.
vm/testsgreen.check-cast-semanticsandcheck-native-signaturesclean. GC heap verifier clean over ~1e9 references.Known, not fixed. A rare (~1/14) crash remains in interface dispatch -- a bad
class id reaching a registered-row lookup. The
callocabove makes it a cleanNULL-row fault instead of silent garbage. It predates this branch as far as the
evidence goes, and it is not reproducible under a debugger.
Deliberately not here. Lowering for-each to an indexed loop in the
translator. The remaining iterator cost is two interface dispatches per element,
each a four-load pointer chase; removing those means not making the calls, which
is a control-flow rewrite and belongs in its own change.
Not rebased on #5741. Both touch
cn1_globals.mGC policy in differentfunctions (that PR trims the SATB log's retained buffers; this one bounds
run-ahead in
cn1BibopPacingCap).