Repository navigation
Conversation
…s gone A name that is a property of the global object is read with op_get_from_scope and the resolve type GlobalProperty. When no property has the name, the read is a ReferenceError if its mode is ThrowIfNotFound. That is the mode of every read except `typeof name`. LLInt, Baseline and code compiled ahead of time throw it from their slow paths. The DFG bytecode parser loads the property directly when it can prove where the property is. When it cannot, it emitted a GetByIdFlush of the global object. That is a plain get by id: it gives undefined when it finds no property, and its inline cache caches that answer. So once a function was compiled by the DFG, a read of a global whose property was deleted gave undefined, and a call of it gave "x is not a function". The parser takes this path when the lower tiers have no structure to check: an accessor, a property with a getter in C++, a property that the global object inherits, a global object that is an uncacheable dictionary. Since 318918@main (webkit.org/b/320802) it also takes it when the structure that the lower tiers cached no longer matches and the site exited once. That is the case of a plain value that was deleted. Add the inline cache access type GetByIdThrowIfNotFound. Its two slow path operations throw the ReferenceError when they find no property, and never get as far as caching it. tryCacheGetBy builds no miss case for it, and the access type is not in the list that allows a megamorphic cache, so every way the access can end without a property is one of these operations. A hit is what it is for GetById: the same cases, the same handlers, the same machine code at the site. GetByIdData carries the ResolveMode of the access, and has to be given one. The parser passes the mode of the bytecode for the read of a global, and the DFG and FTL back ends pick the access type from it. The base of such a read is a scope, so the fixup phase gives it a cell edge, and there is no operation for a base that is not a cell. The write is not changed. A strict mode write to such a name is a PutDynamicVar since #757, and global-property-strict-write-not-found.js is the test for it. Without this change that test fails only where it reads. Upstream main has the same GetByIdFlush. * JSTests/microbenchmarks/global-accessor-property-read.js: Added. * JSTests/microbenchmarks/global-inherited-property-read.js: Added. * JSTests/stress/global-property-read-not-found.js: Added. * JSTests/stress/global-property-strict-write-not-found.js: Added. * Source/JavaScriptCore/bytecode/InlineCacheCompiler.cpp: * Source/JavaScriptCore/bytecode/PropertyInlineCache.cpp: * Source/JavaScriptCore/bytecode/PropertyInlineCache.h: * Source/JavaScriptCore/bytecode/Repatch.cpp: * Source/JavaScriptCore/bytecode/Repatch.h: * Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp: * Source/JavaScriptCore/dfg/DFGFixupPhase.cpp: * Source/JavaScriptCore/dfg/DFGGraph.cpp: * Source/JavaScriptCore/dfg/DFGNode.cpp: * Source/JavaScriptCore/dfg/DFGNode.h: * Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp: * Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp: * Source/JavaScriptCore/jit/ICStats.h: * Source/JavaScriptCore/jit/JITOperations.cpp: * Source/JavaScriptCore/jit/JITOperations.h:
|
Status. The PR is open and waits for review: #908 How it was reproduced. With the script from the description, in the
In Bun 1.4.3 the same script (with |
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Retain resolution mode in DFG Source/JavaScriptCore/dfg/DFGNode.h, DFGByteCodeParser.cpp, DFGFixupPhase.cpp, DFGSpeculativeJIT64.cpp, FTLLowerDFGToB3.cpp, DFGNode.cpp, DFGGraph.cpp |
DFG get-by-identifier data now stores a resolve mode. Global scope reads pass that mode through parsing and JIT lowering. Other read paths specify non-throwing modes. |
Throwing inline-cache path Source/JavaScriptCore/bytecode/PropertyInlineCache.h, Repatch.h, Repatch.cpp, PropertyInlineCache.cpp, InlineCacheCompiler.cpp, Source/JavaScriptCore/jit/JITOperations.h, JITOperations.cpp, ICStats.h |
The inline-cache path adds a get-by-identifier access type for missing-property errors. JIT operations handle lookup and missing properties, and repatching handles eligible caches. |
Global read tests and benchmarks JSTests/stress/global-property-read-not-found.js, JSTests/microbenchmarks/global-accessor-property-read.js, global-inherited-property-read.js |
Stress tests cover reads across property deletion and restoration, accessors, inherited properties, bindings, and realms. Benchmarks check repeated reads from accessor and inherited properties. |
Strict global write tests JSTests/stress/global-property-strict-write-not-found.js |
The stress test checks strict writes to missing, present, accessor, and inherited properties. It also includes property changes, garbage collection, and a sloppy-mode control case. |
Priority:
Merge Risk: 
Minimal · up to edd74
-
No concrete merge-blocking risk is established in the reviewed change.
Pre-merge checks: 
3 · 
1
-
Failed checks: 1 warningCheck name Status Explanation Resolution Description check The description follows the WebKit-style intent of the template: it names the bug (DFG/FTL read undefinedfor a deleted global where LLInt, Baseline and AOT throwReferenceError), explains the fix…Add the WebKit Bugzilla bug link in the form https://bugs.webkit.org/show_bug.cgi?id=##### and use a bug title as the first line. Add a Reviewed byline (the template expects NOBODY (OOPS!) as a placeholder until review). Replace the free… Passed checks: 3 passedFull details: Description check
-
Explanation
The description follows the WebKit-style intent of the template: it names the bug (DFG/FTL read
undefinedfor a deleted global where LLInt, Baseline and AOT throwReferenceError), explains the fix (a newGetByIdThrowIfNotFoundaccess type selected fromResolveMode, with a second commit that resets the cache when it gives up), and gives thorough verification. It is not a fit for the template's required structure, though. It has no bug title, no Bugzilla link (it says upstream bug is not filed, and gives a PR-style reference instead), noReviewed byline, and no changelog-style file list with function names. The instructions say a description that is mostly complete should pass, and this one has the substance. The gaps in format are real but non-critical to the content. The description also lacks a clear statement of the bug tracker link that the template requires for every change, which is a required item in the template.Resolution
Add the WebKit Bugzilla bug link in the form https://bugs.webkit.org/show_bug.cgi?id=##### and use a bug title as the first line. Add a
Reviewed byline (the template expects NOBODY (OOPS!) as a placeholder until review). Replace the free-form notes with a short explanation of the fix in the template's style. Add a file list at the end (for example* Source/JavaScriptCore/bytecode/PropertyInlineCache.h:with the changed functions) so the change is traceable. Keep the detailed benchmark and test tables, but move them below the template sections so the core fields come first.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, two further concerns were examined and ruled out: an FTL/DFG GetById with ThrowIfNotFound reaching a non-cell base (and thus the missing Generic operation) — FixupPhase pins the edge to CellUse and both backends assert on it; and the missing DFGSpeculativeJIT32_64 arm — that backend does not exist in this checkout, so the 64-bit and FTL updates cover every compiled tier.
Extended reasoning...
The change adds a new inline-cache access type for DFG/FTL global reads across the IC compiler, Repatch, JIT operations, and the DFG parser/fixup/backends, plus four JS tests; it touches no auth, crypto, or data-exposure surface. Two confirmed findings (a stale inherited-property cache after a later var declaration, and the lack of megamorphic folding for the new access type) are posted inline, and the change is a non-trivial JIT correctness fix, so human review is warranted.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
Source/JavaScriptCore/bytecode/InlineCacheCompiler.cpp— Hot DFG/FTL reads of a global name whose global object changed structure 8 times now walk 8 dead handlers and then call C++ on every read. tryFoldToMegamorphic at InlineCacheCompiler.cpp:4697 only folds AccessType::GetById and GetByIdWithThis; a GetByIdThrowIfNotFound cache falls to the default, returns nullptr and goes to GiveUpOnCache with its full handler list kept. Fix: when the list reaches maxAccessVariantListSize for GetByIdThrowIfNotFound, drop the dead handlers (or add a throw-aware megamorphic case) so the slow path call is reached directly, matching what base GetById does for the same site.Why this was flagged
A ThrowIfNotFound GetByIdFlush is emitted at DFGByteCodeParser.cpp:10837 for every unproven global name read (accessor, inherited, dictionary global). Each new structure of the global object seen by that site adds a Load/Getter case; after Options::maxAccessVariantListSize() cases, InlineCacheCompiler.cpp:4692-4735 returns nullptr for the new access type, so the cache gives up and repatches to operationGetByIdThrowIfNotFoundGaveUp (JITOperations.cpp) while all 8 handlers stay chained before the call. Base GetById folds to one LoadMegamorphic case at InlineCacheCompiler.cpp:4734 and, because JSGlobalObject has OverridesGetOwnPropertySlot (JSGlobalObject.h:778), getByIdMegamorphic at JITOperations.cpp:379 repatches to a slow path call behind one handler. The author sizes the delta at +38 instructions per read (DFG) and +7 (FTL) on every read for the lifetime of the compiled code; global-object churn (polyfills, test harnesses, delete/re-add of globalThis properties) reaches it in production. Remedy: replace the handler list with a direct slow path call for this access type once it gives up.
Verification: Trigger: a hot DFG/FTL read of an unproven global name after 8 global object structure changes.
tryFoldToMegamorphic(Source/JavaScriptCore/bytecode/InlineCacheCompiler.cpp:4692-4698) only foldsAccessType::GetById/GetByIdWithThis;GetByIdThrowIfNotFoundreturns nullptr, so every read lands in the GaveUp operation. On the base branch the same site folds toLoadMegamorphic.
|
Preview build of edd7475: |
The base of such an access is one global object. A global object is a dictionary from its start, so no transition gives it a structure that it had before. When the cache gives up, its cases are for structures that are gone. The one exception is the last case of a cache that gives up because it is full: that case is for the structure of the moment. The cases stayed in front of the slow path call. A site that saw more than 8 structures of the global object then cost 38 instructions per read more in the DFG, and 7 in the FTL, than the same site with GetById, whose megamorphic case does not take a global object and resets the site. repatchGetBy now resets the cache when it gives up for this kind, with repatchGetBySlowPathCall, as the megamorphic operations do. Such a site now costs what it costs with GetById: 563 instructions per read of a getter global in the DFG (565 with GetById), 529 in the FTL (531). * Source/JavaScriptCore/bytecode/Repatch.cpp: (JSC::repatchGetBy):
|
About the 8 dead handlers of the review above: the second commit (edd7475) takes the suggestion. A site that saw more than 8 structures of the global object, instructions per read of a getter global (FTL / DFG only):
The reset also drops the last case of a cache that gives up because it is full, as it does on |
Problem
undefinedfor a global name after its property is deleted. LLInt, Baseline and AOT code throwReferenceError: x is not defined.GetByIdFlushwhen it cannot prove where the property is (dfg/DFGByteCodeParser.cpp:10835). Its inline cache caches the miss.Fix
GetByIdThrowIfNotFound, picked from the bytecode'sResolveMode: its slow path throws the ReferenceError and caches no miss. A hit runs the code ofGetById.GetById.JSTests/stress/global-property-read-not-found.jsfails in 17 of 18 modes onmainand passes here, also with assertions. Bun's new test passes with this change on Bun's pin.$vmfunction), an upstream bug report, a named owner.Background
ThrowIfNotFoundis the mode of every read of a global name excepttypeof x.GetDynamicVar(11 lines), as [JSC] Ahead-of-time compilation #757 has for the write. With no inline cache a getter global, such as Bun'sBuffer, costs 458 instructions per read, not 62.5.Downsides
jsctext +2416 bytes, 8 bytes per VM.var.Notes
Repro in the
jscshell.jsc --useConcurrentJIT=0 repro.jsprints 399 onmain(f994ea3) and on 680ef47, the commit Bun pins. It prints -1 here and with--useDFGJIT=0.How the parser gets to the fallback. From graph dumps (
--dumpGraphAfterParsing=1,--verboseOSR=1):op_get_from_scope. After the delete their structure check fails, and their slow path throws.CheckStructureof that stale structure. It exits withBadCache. The next compile sees the exit site, andGetByStatus::computeForanswersLikelyTakesSlowPath, so the parser emitsGetByIdFlush.GetByIdFlushat once, and the code that is already there answers wrong at the first call after the delete. That is the case for an accessor, a property with a C++ getter, a property that the global object inherits, and a global object that is an uncacheable dictionary (129 deletes in a row make it one).Where it comes from. No user reported this. It was found in a test of reads of browser globals in Bun.
hasBadCacheExitSitetest to both parser arms, "so we'll emit a generic IC instead of speculating and failing repeatedly". Before it, the DFG kept itsCheckStructure, and a deleted value threw.mainhas the sameGetByIdFlush, and no test for a name that is gone. No upstream bug is filed for it.[[HasProperty]]again. That's also true without a Proxy, for exampleObject.prototype.xread in a hot loop and then deleted." This PR removes the part without a Proxy.global-bindings/programs/delete-then-read-throwsinaot/tests/AOTGlobalBindingProgramTests.cpp), and fixed the write arm of this parser fallback.What is wrong where. The same scripts on three builds, with
--useConcurrentJIT=0:mainTypeError: f is not a functionat call 396var x = 9;undefinedundefinedtypeofof a deleted name, sloppy writeThe write is not in this PR. An access type for the strict mode write was built too (75 instructions per write through a global setter, where
PutDynamicVarcosts 843). It is left out. A cached setter stays in use after a script declaresvar x;with that name, whichPutDynamicVargets right (see #738 below), and at a site that does not cache it costs 41 to 58 instructions more thanPutDynamicVar.Why no miss can be cached. Every way a
GetByIdThrowIfNotFoundaccess can end without a property:Optimize,GaveUp) throw whengetPropertySlotfinds nothing, beforerepatchGetBy.tryCacheGetBybuildsAccessCase::Missonly for an unset slot. It returns first for this kind.tryFoldToMegamorphiclists the access types that get a megamorphic cache. The new one is not in it.GetByIdFlushonly whenGetByStatus::computeFor(globalObject, structures, ...)isSimple, and that needs the property on the structure.Genericoperation.Tests.
run-jsc-stress-testson the two new files, 18 modes each (the 17 default ones and--forceICFailure=true, which sends every access to theGaveUpoperation):global-property-read-not-found.jsglobal-property-strict-write-not-found.jsmainENABLE_ASSERTS=ONThe mode that passes everywhere is
lockdown, which has no JIT.The write test is the first one in
JSTestsfor the statement of [JSC] Ahead-of-time compilation #757. It has a strict write through an inherited setter that a later script shadows withvar. A build with an inline cache for the write fails there.The C++ custom getters are on the prototype chain of a second global object (
$vm.createGlobalObject). The shell has no custom getter directly on a global object. Bun has, and that case ran there (see Bun below).The build has
-Wall, and no-Wswitchdiagnostic names a changed file. No tested CI lane has assertions on, so the assertions build above was made by hand.A build with
GetDynamicVarpasses both tests too. Nothing in CI compares speed. The two files inJSTests/microbenchmarksare an input forrun-jsc-benchmarks, no more.The related tests (804 files of
JSTests/stresswhose names haveglobal,scope,get-by-id,getter,lengthand the like, 12447 runs) pass on the assertions build.All of
JSTests/stresswith--memory-limitedon the Release build, 91132 runs: 20 fail, inbigint-inc-dec-in-place(17),big-int-out-of-memory-tests,bigint-terminate-multiplyandftl-osr-exit-stack-overflow-phantom-array. Each ends withCrashing because current footprint: ... exceeds limit: 629145600, the limit that the option sets, and each fails the same way onmain(the last one in 5 of 6 runs on both).Instructions per read (
perf_event_open, user space, x64 Linux, ASLR off,(count(4M) - count(2M)) / 2M, Release-O3 -march=nehalem, same toolchain for all builds). FTL / DFG only:mainGetDynamicVarMath,Array,Objecton such a global objecttypeofof a getter globalmainand here. So the column on the right is about optimized code only.Bufferis such a global: a function that readsBuffergetsGetByIdFlush(... cachable-id {uid:(Buffer)} ...)from the DFG of Bun 1.4.3.A site that does not cache. Every read is a call of the
GaveUpoperation. FTL / DFG only:mainGetDynamicVar--forceICFailure=true--forceICFailure=truemainand here.GlobalRegistrator.unregister()of happy-dom 17.0.3 deletes 344 globals: in Bun 1.4.3 a read of a global that it did not touch costs 1 to 2 ns before the secondregister()andunregister(), and 11 to 16 ns after. That is an older limit of the inline caches, and not changed here.mainthe eighth case folds into a megamorphic cache, which does not take a global object and resets the site. This access type has no megamorphic case, sorepatchGetByresets the site when its cache gives up (the second commit). Without that the 8 handlers stay in front of the call, which costs 38 instructions more per read in the DFG and 7 in the FTL (603 and 538 in the last rows). The numbers of the last two rows include the call of the function that reads.Slow path calls (gdb breakpoint counts, N = 20000 and 40000 reads):
operationGetByIdThrowIfNotFoundOptimize, so none per read.main: 2 and 2 ofoperationGetByIdOptimize. A C++ custom getter: 3 and 3.GaveUpoperation and of the four plainGetByIdoperations.A name that stays missing. 200000 reads after the delete: 0 wrong (199220 wrong on
main). 3526 instructions per read with the throw and the catch, 4049 with the DFG off. The function is compiled 4 times by the DFG at 200000 and at 400000 reads, so there is no recompile loop.BadCacheexits in a script that defines and deletes the name 40 times: 216, and 216 onmain.A global object that keeps changing shape.
GaveUpcalls, 33837 onmain.GaveUpcalls, 76997 onmain. A miss takes no place in the cache now.main. In the getter benchmark: 1 and 1.Size.
jsctext: 49228940 to 49231356 bytes, +2416. The two new operations are 1819 bytes of it.sizeof(SharedJITStubSet): 264 to 272, one slow path handler slot per access type and VM.DFG::GetByIdDatastays 16 bytes,HandlerPropertyInlineCache80,RepatchingPropertyInlineCache144. The startup of the shell calls neither new operation. Its instruction count is 5017185 onmainand 1955 more here, but builds of this change differ by 4400 among themselves (a hash table keyed by the addresses of functions), so that number says nothing.Other open PRs.
var x = 33;adds the variable with no new Structure. This PR makes one of its shapes right, the read of a name that was found once and is gone: nothing is cached for it, so the read finds the variable. A name that the global object inherits and that a script then declares still reads the inherited value in optimized code, as onmain.get_from_scopeslow paths throughCommonSlowPaths::getPropertySlotForGetFromScope, for a Proxy in the prototype chain of a global object.getByIdThrowIfNotFoundinjit/JITOperations.cppis one more place that does this lookup. The one that lands second has to send it through that function too.GlobalPropertyfallbacks need theResolveModetoo.GetByIdDatahas no default for it, so a rebase fails to compile at that statement.Upstream. The source files have no fork guard. 14 of the 15 apply to WebKit/WebKit
main(801209c) as they are. In the parser one hunk of 8 does not: aGetByIdDatainhandleIteratorNextthat only the fork has. The tests expect this fork's message,x is not defined. Upstream's isCan't find variable: x.Bun. Bun 1.4.3 pins 680ef47. With
console.logforprint, the script above prints 399 underBUN_JSC_useConcurrentJIT=0, a few thousand without it, and -1 underBUN_JSC_useDFGJIT=0. Node v26.3.0 prints -1.Bun cannot link a build of this branch before its own upgrade past #757. So the branch
robobun/0767d4fc/read-not-found-on-680ef47cechas the two commits of this PR and the write statement of #757 on top of 680ef47. Its lanes areautobuild-aef2ee0608c6c30e03ce8eee4122ce6c10b37ee4, from a run of the workflow by hand. That branch is for this test only. The table is from two Release builds of Bunmain(179664ea89,--lto=off, Linux x64): one with 680ef47, one with--webkit-version=aef2ee0608c6c30e03ce8eee4122ce6c10b37ee4.BUN_JSC_useConcurrentJIT=0register(), 100 uses ofdocument,unregister(): cycles where a read ofdocumentis not a ReferenceError after ittest/js/bun/jsc,test/js/node/vmand three more files (34 files)test/js/web,test/js/bun/util,test/regression/issue, four directories oftest/js/node), one process for each fileBuffer.isBuffer(b)selfBuffer.from(arrayBuffer).lengthBuffer.isBuffer(b)bunBUN_JSC_useDFGJIT=0give 0 in the happy-dom row too.GetByIdFlushof a global hasThrowIfNotFoundon the second one.-debug-asanlane of that autobuild (JavaScriptCore with assertions), passes the new test and the happy-dom loop. On it, a read of each of 15 of Bun's own globals after its delete (self,onerror,Buffer,process,fetch,URL,setTimeoutand more) is a ReferenceError at every call, also underBUN_JSC_validateExceptionChecks=1. On 680ef47 these readundefinedfrom call 0 or call 601.selfhas a getter in C++ directly on the global object, which thejscshell cannot test.streams-leak.test.tsthat takes about a minute on this machine, which was the timeout of the run. Run six more times, it timed out 2 times on the first build and 5 times on the second. Its loop executes no more instructions on the second build (242 and 251 million for 1000 rounds, against 259 and 274), so that is its timing.robobun/0767d4fc/global-variable-not-defined-testof oven-sh/bun (test/js/bun/jsc/global-variable-not-defined-after-tier-up.test.ts). It goes in with the WebKit upgrade that has this change. Only Linux x64 was run. No run of Bun's CI was made with this build.