JavaScript port: resolve suspension by receiver type, not by bare signature - #5755
Open
shai-almog wants to merge 3 commits into
Open
JavaScript port: resolve suspension by receiver type, not by bare signature#5755shai-almog wants to merge 3 commits into
shai-almog wants to merge 3 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 026f05c6f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
Contributor
Cloudflare Preview
|
Collaborator
Author
|
Compared 166 screenshots: 166 matched. |
Collaborator
Author
|
Compared 166 screenshots: 166 matched. |
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
…nature The JavaScript backend compiles a method that can block into a JS generator and every call to it into ``yield*``. JavascriptSuspensionAnalysis decides which methods those are, and it decided by ``name + descriptor`` with no owner class -- so one blocking ``run()V`` anywhere in the program made every ``run()V`` call site in the program a suspension point, and every caller of those suspending in turn. Measured on hellocodenameone before this change: 21,467 live methods, 13,011 of them suspending, and 51% of those became suspending purely through that signature-wide propagation. The top entries were ``toString()`` (1,682 call sites), ``equals(Object)`` (1,192), ``getHeight()I`` (1,145), ``getWidth()I`` (1,126) and ``length()I`` (1,059) -- leaf methods whose typical receiver has a trivially synchronous implementation. JavascriptReachability already computed the receiver-type information needed to fix this and kept it private. It now publishes its instantiated set and a Model that resolves a call site against its subtype cone, and three separate over-approximations go away: * Suspension propagates through per-implementation edges resolved from the receiver cone instead of through the bare signature. Emitter and analysis share one DispatchModel, because a ``yield*`` emitted into a plain ``function`` is a JS SyntaxError rather than a subtle mistake. * A signature declared on a JSO bridge class is suspending only when the call site's cone can actually reach such a class. ``getWidth()I`` is declared on three JSO bridge interfaces, which was making every ``Component.getWidth()`` in the program a suspension point. * collectBridgeReferencedCn1Tokens scraped every ``"cn1_..."`` literal out of the bridge JS and seeded every matching method suspending. That is right for a name the bridge REPLACES (bindNative, or ``classDef.methods[id] = fn``) and wrong for one it merely LOOKS UP through resolveVirtual, whose result is driven by cn1_ivAdapt / adaptVirtualResult -- both of which tolerate a plain function. A token is now dropped only when EVERY occurrence of it is a resolveVirtual argument, so a name reached through a variable stays protected. That drops 10 of 705 tokens, including toString, equals, hashCode and run. Object.toString and Object.hashCode bind as plain ``function`` and Object.equals is not bound at all, so all three were suspending for no reason. Also fixed while in here: the cone resolver treated an instantiated receiver whose body the superclass walk could not find as contributing nothing, which under-approximates -- resolveVirtual also walks interfaces and then falls back to the global native table, and either can land on a generator the walk never saw. It now fails the whole query instead. And both published statics are reset per translation; Surefire reuses one JVM across fixtures, so a stale instantiated set would have resolved one app's call sites against another app's type graph. Measured, hellocodenameone: yield* sites 54,549 -> 40,741 (-25.3%), generators 13,068 -> 11,044 (-15.5%), suspending virtual dispatch 28,569 -> 19,073 (-33.2%), synchronous methods 8,456 -> 9,875 (+16.8%), bridge-referenced seeds 896 -> 236. The bundle barely moves (-1.2% raw, -0.25% gzip): generator machinery is nearly free in bytes and expensive only in time. That last point is why this change also brings a benchmark. Nothing in the tree could price a JavaScript backend change -- the bundle does not move, the screenshot suite is pass/fail and the lifecycle harness reports milestones rather than time, so a real reduction in generator density and a no-op looked identical. scripts/run-javascript-throughput-benchmark.sh translates vm/benchmarks/javascript/JsThroughputBench.java and runs it under Node in about 55 seconds. Interleaved against master, best of three: hashCodeHeavy -57.6%, toStringHeavy -18.9%, equalsHeavy -7.8%, mapChurn -6.6%, against a master-vs-master noise floor of 0.3-4.9% and two controls that the dispatch work cannot touch (-1.8% and 0.0%). Four properties of that benchmark were each paid for with a wrong answer, and the comments say so: every workload reports a checksum and the comparison refuses a workload whose checksum moved; there are two controls; each workload runs in its own process, because sharing one made iteratorWalk measure 8.5ms run first and 15ms run eighth, so speeding up an earlier workload reported a regression in byte-identical code; and the harness counts the times cn1_ivsDrive silently steps a generator that reached the sync dispatcher, which would otherwise show up as a speedup rather than as the bug it is. That counter reads 0 on every arm measured here. One result is unexplained and is not claimed as noise: iteratorWalk regressed 13.7%, twelve times its own noise floor, reproducibly. Its emitted body is byte-identical between arms, all nineteen iterator functions are byte-identical, an exhaustive function-level diff finds 42 differing functions and none in its execution path, both bundles define the same function set, and V8 deopt counts match. It survives process isolation and hoisting the allocation out of the timed region. vm/tests: 305 tests, 0 failures, 1 pre-existing skip. SpotBugs clean. The 181-screenshot suite has NOT been run locally -- Cn1ssScreenshotServer binds a hardcoded port 8765 and a concurrent checkout holds it -- which is the main thing CI needs to answer here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The screenshot suite caught a real regression: nine theme tests (CheckBoxRadioTheme x4, ShowcaseTheme x4, DesktopMode) differed, and master is green on this PR's base commit, so they were mine. collectBridgeReferencedCn1Tokens has SIX consumers, not one. Besides the suspension seed it feeds minifyGeneratedIdentifiers, mangleDispatchIds, mangleInstanceFieldProps and the devirtualization exclusions in JavascriptBundleWriter and JavascriptMethodGenerator -- four of them on by default. Narrowing that shared set to "names the bridge can replace" therefore did not just relax suspension, it let the minifier RENAME names the bridge only looks up. Verified in the emitted bundle: cn1_com_codename1_ui_plaf_UIManager_getLookAndFeel_R_..., cn1_com_codename1_ui_plaf_LookAndFeel_getDefaultFormTintColor_R_int and cn1_com_codename1_ui_MenuBar_initMenuBar_com_codename1_ui_Form occur 0 times in the broken bundle and 1 time each after this commit. port.js resolves all three by literal, so the theme lookup failed and the fallback rendered differently. Those are two different questions and this commit stops conflating them. "The bridge NAMES this method" decides whether the identifier may be renamed, whether its m: entry may be pruned and whether the call may be devirtualized -- a lookup needs all of that. "The bridge REPLACES this method" decides whether callers must yield*, and only a replacement can turn out to be a generator. So collectBridgeReferencedCn1Tokens goes back to the full scrape and a new collectBridgeReplacedCn1Tokens carries the narrowing, used by the suspension analysis alone. The optimization is unaffected: yield* sites 40,741, generators 11,044 and suspending dispatch 19,073 are identical to the broken build; only 96 more dispatches take the sync helper because three names are no longer devirtualized. Two more review findings, both real: * js_throughput_report.py ASSIGNED the BENCHPROBE count instead of summing it. The runner uses one process per workload and concatenates their output, so only the last workload's count survived: a workload that reached cn1_ivsDrive with a generator would have been reported as zero and its warning suppressed, which defeats the one probe that can tell an unsound classification from a speedup. Proven non-vacuous against a synthetic log -- 4 and a warning with the fix, 0 and silence without it. * The RTA reset lived inside JavascriptReachability.run, which Parser skips entirely under -Dparparvm.js.rta.off. A reused JVM that translated one application with RTA on and a second with it off would have handed the second application's call sites the first application's type graph -- an under-approximation, i.e. the direction that picks the synchronous dispatcher for a suspending override, and a property this translator's own tests already flip mid-JVM for the minifier. The reset is now an explicit resetExportedFacts() that Parser calls before deciding whether to run RTA. vm/tests: 305 tests, 0 failures, 1 pre-existing skip. SpotBugs 0 findings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
force-pushed
the
js-owner-aware-suspension
branch
from
September 9, 2026 17:22
026f05c to
5606f72
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5606f72125
ℹ️ 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
+570
to
+571
| for (Map.Entry<String, List<BytecodeMethod>> e : sigCallersOf.entrySet()) { | ||
| counts.put(e.getKey(), Integer.valueOf(e.getValue().size())); |
There was a problem hiding this comment.
When RTA resolves a virtual call successfully, the scan records it only in callersOf and then continues at line 516, so it never enters sigCallersOf. Building dispatchSiteCount solely from sigCallersOf therefore counts only fallback calls; under the default RTA path, receiver-resolved calls that actually emit yield* are reported as zero or omitted from the ranking, making Section 2's dispatch-site totals and optimization guidance inaccurate.
Useful? React with 👍 / 👎.
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
The suite went from nine theme mismatches to zero, and then failed on two test
FAILURES instead. Both are the named errors this work added on purpose, firing
exactly as intended rather than corrupting silently.
CryptoApiTest: "cn1_ivs: sync virtual dispatch reached a yielding method (CHA
unsound): cn1_s_aesEncrypt_...". collectSyncNativeTokens decides whether a
bindNative wrapper is a plain function (synchronous native) or a function*
(suspending) by searching FORWARD from the name array for the next occurrence
of "function". When the wrapper is built by a factory --
bindNative([...aesEncrypt...], cn1CryptoAesBinding("aesEncrypt"));
-- there is no function keyword in the call at all, so the search ran past the
end of it and matched the "function cn1CryptoRsaBinding(op)" DECLARATION
several lines below. aesEncrypt was classified a synchronous native; it is a
generator. The wrapper is now required to be the literal argument that follows
the array, and anything else -- a factory call, an identifier, an arrow -- is
left out of the sync set, i.e. stays suspending, which is the safe direction.
315 genuine sync natives still classify as such.
This bug predates the branch. It was unreachable because the signature-wide
bridge seed made every one of those dispatches suspending regardless; resolving
call sites against their receiver removed that cover.
BytecodeTranslatorRegressionTest: "yield* (intermediate value)(intermediate
value) is not iterable" -- the mirror image. A devirtualized call site names its
single implementation directly through _dv*/_dw*, but took its suspending flag
from isInvokeSuspending, which answers for the SIGNATURE. Those used to be the
same thing. Once a call site resolves against its receiver they are not: a
signature that suspends somewhere else in the program can devirtualize here to
a plain function, and _dv* then put yield* in front of it.
computeMonomorphicDispatch now records whether each devirtualized target is
itself suspending and both emission sites follow the target.
Verified in the emitted bundle: aesEncrypt was "function ...aesEncrypt..." with
a _wN call site and is now "function* ...aesEncrypt..." with a _vN one. The
optimization is intact -- yield* 40,753, generators 11,054, suspending dispatch
19,075, against master's 54,549 / 13,068 / 28,569.
vm/tests: 305 tests, 0 failures, 1 pre-existing skip. SpotBugs 0 findings. The
throughput benchmark's sync-dispatcher probe reads 0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
The JS backend compiles a method that can block into a JS generator and every call to it into
yield*.JavascriptSuspensionAnalysisdecides which methods those are — and it decided byname + descriptorwith no owner class. One blockingrun()Vanywhere made everyrun()Vcall site in the program a suspension point, and every caller of those suspending in turn.Measured on hellocodenameone before the change: 13,011 of 21,467 live methods suspending, 51% of them from that signature-wide propagation alone. Top entries were
toString()(1,682 call sites),equals(Object)(1,192),getHeight()I(1,145),getWidth()I(1,126),length()I(1,059).JavascriptReachabilityalready computed the receiver-type information needed to fix this and kept it private. It now publishes it, and three over-approximations go away — per-implementation propagation resolved from the receiver cone, cone-aware JSO protection, and a bridge-token rule that distinguishes a name the bridge replaces from one it merely looks up.Full reasoning is in the commit message and in the code comments.
Results (hellocodenameone)
yield*sitesThe bundle barely moves, and that is the honest read: generator machinery is nearly free in bytes (
yield*is seven characters) and expensive only in time.Why there is a benchmark in here
Nothing in the tree could price a JS backend change — the bundle does not move, the screenshot suite is pass/fail, the lifecycle harness reports milestones rather than time. A real reduction in generator density and a no-op looked identical.
scripts/run-javascript-throughput-benchmark.shtranslatesvm/benchmarks/javascript/JsThroughputBench.javaand runs it under Node in ~55s. Interleaved against master, best of three:hashCodeHeavy-57.6%,toStringHeavy-18.9%,equalsHeavy-7.8%,mapChurn-6.6% — against a master-vs-master noise floor of 0.3-4.9% and two controls the dispatch work cannot touch (-1.8%, 0.0%).It reports a checksum per workload and refuses a comparison whose checksum moved, runs one process per workload, and counts the times
cn1_ivsDrivesilently steps a generator that reached the sync dispatcher (0 on every arm here).What reviewers should look hardest at
collectBridgeReferencedCn1Tokens. It drops a token only when every occurrence is aresolveVirtualargument — 10 of 705 tokens, includingtoString,equals,hashCode,run. If there is a replacement path that reaches a method some other way, this is where it breaks.Model.resolveImplsvsresolveVirtual. The static walk must not be narrower than the runtime's. It now fails the whole query rather than skipping an unresolvable receiver, becauseresolveVirtualalso walks interfaces and then falls back to the global native table.DispatchModel; ayield*emitted into a plainfunctionis a SyntaxError, not a subtle bug.What CI is being asked to answer
The 181-screenshot suite has not been run locally —
Cn1ssScreenshotServerbinds a hardcoded port 8765 (the device runner hasws://HOST:8765compiled in) and a concurrent checkout on this machine holds it. That is the one gate that exercises the narrowed bridge seed against real bridge traffic.vm/**is in the workflow's paths filter, so it should fire here.Locally green:
vm/tests305 tests / 0 failures / 1 pre-existing skip (incl.JavascriptRuntimeSemanticsTest, 177 tests over 11 compiler configs), SpotBugs 0 findings, copyright + control-character + cast-semantics gates.Known open item
iteratorWalkregressed 13.7%, twelve times its own noise floor, reproducibly — and I could not explain it. Its emitted body is byte-identical between arms, all nineteen iterator functions are byte-identical, an exhaustive function-level diff finds 42 differing functions and none in its execution path, both bundles define the same function set, and V8 deopt counts match (44 vs 43). It survives process isolation and hoisting the allocation out of the timed region. Flagging rather than burying it.🤖 Generated with Claude Code