From 013a7127ab7759ce1b322ac0eefcf7c89999d8ba Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sun, 26 Apr 2026 00:44:09 +0000 Subject: [PATCH 1/3] perf(JSC): O(E+N) GetModuleNamespace; iterative InnerModuleLoading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - AbstractModuleRecord::getModuleNamespace: replace per-name star-graph walk (O(names × edges)) with a single BFS that records each name's unique Local/Namespace binding. A name with exactly one binding is Resolved by spec regardless of resolveSet, so only names with >1 binding or an Indirect entry fall back to resolveExportImpl. - AbstractModuleRecord::resolveExportImpl: read m_resolutionCache in Type::Query unconditionally. Only Resolved values are ever written (root or local), so a non-empty resolveSet can only turn paths to the cached binding into null, and merge(Resolved, null) = Resolved. - JSModuleLoader::innerModuleLoading: drain a worklist on the state instead of recursing through the FinishLoadingImportedModule -> ContinueModuleLoading re-entry. continueModuleLoading short-circuits already-visited modules without a function call. - ModuleGraphLoadingState: inline the trivial accessors; widen m_visitedSet to AbstractModuleRecord* so the per-edge contains check runs before dynamicDowncast. - JSModuleLoader::loadRequestedModules: memoize the GraphLoadingState capability per module (skip rejected so retry still re-walks). bench/module-loader (500 modules, 30k star edges): GetModuleNamespace + ResolveExport drop from ~48% of cycles to <1%. Roughly 2x faster end-to-end vs previous. --- .../runtime/AbstractModuleRecord.cpp | 133 +++++++++---- .../runtime/AbstractModuleRecord.h | 7 + .../JavaScriptCore/runtime/JSModuleLoader.cpp | 179 ++++++++++++------ .../runtime/ModuleGraphLoadingState.cpp | 35 ---- .../runtime/ModuleGraphLoadingState.h | 31 ++- 5 files changed, 241 insertions(+), 144 deletions(-) diff --git a/Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp b/Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp index e37881c92f52..a78b4f3fdee2 100644 --- a/Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp +++ b/Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp @@ -96,6 +96,7 @@ void AbstractModuleRecord::visitChildrenImpl(JSCell* cell, Visitor& visitor) visitor.append(thisObject->m_cycleRoot); visitor.append(thisObject->m_topLevelCapability); visitor.append(thisObject->m_asyncCapability); + visitor.append(thisObject->m_loadRequestedModulesPromise); Locker locker { thisObject->cellLock() }; visitor.append(thisObject->m_asyncParentModules.begin(), thisObject->m_asyncParentModules.end()); auto values = thisObject->m_dependencies.values(); @@ -688,13 +689,16 @@ auto AbstractModuleRecord::resolveExportImpl(JSGlobalObject* globalObject, const if (!moduleRecord->starExportEntries().isEmpty()) foundStarLinks = true; - // 4. Once we follow star links, we should not retrieve the result from the cache and should not cache the result. - if (!foundStarLinks) { - if (std::optional cachedResolution = moduleRecord->tryGetCachedResolution(query.exportName.get())) { - if (!mergeToCurrentTop(*cachedResolution)) - return Resolution::ambiguous(); - continue; - } + // The cache only stores Resolved entries. A Resolved cached value means + // ResolveExport(moduleRecord, name, []) found exactly one binding T; + // any non-empty resolveSet can only turn paths to T into null, never + // surface a different binding (that would have made the [] result + // ambiguous and therefore uncached). null vs T merges to T either way, + // so reading the cache here is sound regardless of foundStarLinks. + if (std::optional cachedResolution = moduleRecord->tryGetCachedResolution(query.exportName.get())) { + if (!mergeToCurrentTop(*cachedResolution)) + return Resolution::ambiguous(); + continue; } const std::optional optionalExportEntry = moduleRecord->tryGetExportEntry(query.exportName.get()); @@ -802,36 +806,6 @@ auto AbstractModuleRecord::resolveExport(JSGlobalObject* globalObject, const Ide return resolveExportImpl(globalObject, ResolveQuery(this, exportName.impl())); } -static void getExportedNames(JSGlobalObject* globalObject, AbstractModuleRecord* root, IdentifierSet& exportedNames) -{ - VM& vm = globalObject->vm(); - auto scope = DECLARE_THROW_SCOPE(vm); - - UncheckedKeyHashSet exportStarSet; - Vector pendingModules; - - pendingModules.append(root); - - while (!pendingModules.isEmpty()) { - AbstractModuleRecord* moduleRecord = pendingModules.takeLast(); - if (exportStarSet.contains(moduleRecord)) - continue; - exportStarSet.add(moduleRecord); - - for (const auto& pair : moduleRecord->exportEntries()) { - const AbstractModuleRecord::ExportEntry& exportEntry = pair.value; - if (moduleRecord == root || vm.propertyNames->defaultKeyword != exportEntry.exportName) - exportedNames.add(exportEntry.exportName.impl()); - } - - for (const auto& starModuleName : moduleRecord->starExportEntries()) { - AbstractModuleRecord* requestedModuleRecord = moduleRecord->hostResolveImportedModule(globalObject, Identifier::fromUid(vm, starModuleName.get())); - RETURN_IF_EXCEPTION(scope, void()); - pendingModules.append(requestedModuleRecord); - } - } -} - JSModuleNamespaceObject* AbstractModuleRecord::getModuleNamespace(JSGlobalObject* globalObject, bool shouldPreventExtensions) { VM& vm = globalObject->vm(); @@ -846,12 +820,84 @@ JSModuleNamespaceObject* AbstractModuleRecord::getModuleNamespace(JSGlobalObject if (m_moduleNamespaceObject) return m_moduleNamespaceObject.get(); - IdentifierSet exportedNames; - getExportedNames(globalObject, this, exportedNames); - RETURN_IF_EXCEPTION(scope, nullptr); + // GetModuleNamespace = GetExportedNames + ResolveExport per name. The naive + // implementation walks the star-export graph once per name. With N names and + // E star edges that is O(N * E), and large re-export trees (barrel files, + // `export * from`) make N and E both grow with the graph. + // + // Observation: ResolveExport(root, n, []) is fully determined by the set of + // explicit (Local / Namespace) bindings for n in the star-reachable graph. + // If exactly one module T binds n, the answer is always Resolved(T, ...). + // The spec's resolveSet only ever turns *extra* paths to T into null, and + // merge(Resolved(T), null) = Resolved(T). Only when two distinct bindings + // exist (potential shadowing / ambiguity) or an Indirect entry is involved + // does the path-sensitive walk matter. + // + // So: do a single traversal that (a) collects exported names and (b) records + // each name's unique binding. Names with one binding go straight into the + // namespace; the rest fall back to resolveExportImpl. This is O(E + N) for + // the common case. Vector> resolutions; - for (auto& name : exportedNames) { + IdentifierSet slowPathNames; + { + UncheckedKeyHashSet exportStarSet; + Vector pendingModules; + Resolutions uniqueBindings; + + exportStarSet.add(this); + pendingModules.append(this); + + while (!pendingModules.isEmpty()) { + AbstractModuleRecord* moduleRecord = pendingModules.takeLast(); + + for (const auto& pair : moduleRecord->exportEntries()) { + const ExportEntry& exportEntry = pair.value; + if (moduleRecord != this && vm.propertyNames->defaultKeyword == exportEntry.exportName) + continue; + RefPtr name = pair.key; + if (slowPathNames.contains(name)) + continue; + + Resolution candidate; + if (exportEntry.type == ExportEntry::Type::Local) + candidate = Resolution { Resolution::Type::Resolved, moduleRecord, exportEntry.localName }; + else if (exportEntry.type == ExportEntry::Type::Namespace) { + AbstractModuleRecord* importedModuleRecord = moduleRecord->hostResolveImportedModule(globalObject, exportEntry.moduleName); + RETURN_IF_EXCEPTION(scope, nullptr); + candidate = Resolution { Resolution::Type::Resolved, importedModuleRecord, vm.propertyNames->starNamespacePrivateName }; + } else { + slowPathNames.add(WTF::move(name)); + uniqueBindings.remove(pair.key); + continue; + } + + auto addResult = uniqueBindings.add(name, candidate); + if (!addResult.isNewEntry) { + const Resolution& existing = addResult.iterator->value; + if (existing.moduleRecord != candidate.moduleRecord || existing.localName != candidate.localName) { + slowPathNames.add(WTF::move(name)); + uniqueBindings.remove(pair.key); + } + } + } + + for (const auto& starModuleName : moduleRecord->starExportEntries()) { + AbstractModuleRecord* requestedModuleRecord = moduleRecord->hostResolveImportedModule(globalObject, Identifier::fromUid(vm, starModuleName.get())); + RETURN_IF_EXCEPTION(scope, nullptr); + if (exportStarSet.add(requestedModuleRecord).isNewEntry) + pendingModules.append(requestedModuleRecord); + } + } + + resolutions.reserveInitialCapacity(uniqueBindings.size() + slowPathNames.size()); + for (auto& pair : uniqueBindings) { + cacheResolution(pair.key.get(), pair.value); + resolutions.append({ Identifier::fromUid(vm, pair.key.get()), pair.value }); + } + } + + for (auto& name : slowPathNames) { Identifier ident = Identifier::fromUid(vm, name.get()); const Resolution resolution = resolveExport(globalObject, ident); RETURN_IF_EXCEPTION(scope, nullptr); @@ -902,6 +948,11 @@ void AbstractModuleRecord::asyncCapability(VM& vm, JSPromise* promise) m_asyncCapability.set(vm, this, promise); } +void AbstractModuleRecord::setLoadRequestedModulesPromise(VM& vm, JSPromise* promise) +{ + m_loadRequestedModulesPromise.set(vm, this, promise); +} + void AbstractModuleRecord::setModuleEnvironment(JSGlobalObject* globalObject, JSModuleEnvironment* moduleEnvironment) { VM& vm = globalObject->vm(); diff --git a/Source/JavaScriptCore/runtime/AbstractModuleRecord.h b/Source/JavaScriptCore/runtime/AbstractModuleRecord.h index fa9b74535710..86d8b5809af7 100644 --- a/Source/JavaScriptCore/runtime/AbstractModuleRecord.h +++ b/Source/JavaScriptCore/runtime/AbstractModuleRecord.h @@ -171,6 +171,8 @@ class AbstractModuleRecord : public JSInternalFieldObjectImpl<2> { AsyncEvaluationOrder asyncEvaluationOrder() const { return m_asyncEvaluationOrder; } std::optional pendingAsyncDependencies() const { return m_pendingAsyncDependencies; } bool hasTLA() const { return m_hasTLA; } + JSPromise* loadRequestedModulesPromise() const { return m_loadRequestedModulesPromise.get(); } + void setLoadRequestedModulesPromise(VM&, JSPromise*); JSPromise* topLevelCapability() const { return m_topLevelCapability.get(); } void cycleRoot(VM&, CyclicModuleRecord*); @@ -281,6 +283,11 @@ class AbstractModuleRecord : public JSInternalFieldObjectImpl<2> { WriteBarrier m_asyncCapability; + // Memoized result of LoadRequestedModules() — every concurrent caller waits + // on the first GraphLoadingState's capability instead of spawning its own + // and re-walking the same subgraph. + WriteBarrier m_loadRequestedModulesPromise; + // We assume that all the AbstractModuleRecord are retained by JSModuleLoader's registry. // So here, we don't visit each object for GC. The resolution cache map caches the once // looked up correctly resolved resolution, since (1) we rarely looked up the non-resolved one, diff --git a/Source/JavaScriptCore/runtime/JSModuleLoader.cpp b/Source/JavaScriptCore/runtime/JSModuleLoader.cpp index 425149a79244..0b66165afa50 100644 --- a/Source/JavaScriptCore/runtime/JSModuleLoader.cpp +++ b/Source/JavaScriptCore/runtime/JSModuleLoader.cpp @@ -792,74 +792,110 @@ JSPromise* JSModuleLoader::loadModule(JSGlobalObject* globalObject, const Module return resultPromise; } -static void checkSafeToRecurse(JSGlobalObject* globalObject, ThrowScope& scope) -{ - if (!globalObject->vm().isSafeToRecurse()) - throwRangeError(globalObject, scope, "Maximum call stack size exceeded"_s); -} - -void JSModuleLoader::innerModuleLoading(JSGlobalObject* globalObject, ModuleGraphLoadingState *state, AbstractModuleRecord* module) +void JSModuleLoader::innerModuleLoading(JSGlobalObject* globalObject, ModuleGraphLoadingState *state, AbstractModuleRecord* startModule) { // InnerModuleLoading(state, module) // https://tc39.es/ecma262/#sec-InnerModuleLoading + // + // The spec phrases this recursively, with HostLoadImportedModule re-entering + // via FinishLoadingImportedModule -> ContinueModuleLoading. That re-entry + // happens once per import edge, but only one-per-module actually enters the + // step-2 loop body — the rest hit the visited check, decrement the counter + // and return. With dense `export * from` graphs (E >> V) the per-edge + // function entry / throw-scope / dynamicDowncast dominates. We instead drain + // a worklist on the state: ContinueModuleLoading enqueues, and the outermost + // call drains. Same observable [[Visited]] / [[PendingModulesCount]]. VM& vm = globalObject->vm(); auto scope = DECLARE_THROW_SCOPE(vm); - // 1. Assert: state.[[IsLoading]] is true. - ASSERT(state->isLoading()); - // 2. If module is a Cyclic Module Record, module.[[Status]] is NEW, and state.[[Visited]] does not contain module, then - if (auto* cyclic = dynamicDowncast(module); cyclic && cyclic->status() == CyclicModuleRecord::Status::New && !state->containsVisited(cyclic)) { - // 2.a. Append module to state.[[Visited]]. - state->appendVisited(vm, cyclic); - // 2.b. Let requestedModulesCount be the number of elements in module.[[RequestedModules]]. - size_t requestedModulesCount = module->requestedModules().size(); - // 2.c. Set state.[[PendingModulesCount]] to state.[[PendingModulesCount]] + requestedModulesCount. - state->setPendingModulesCount(state->pendingModulesCount() + requestedModulesCount); - // 2.d. For each ModuleRequest Record request of module.[[RequestedModules]], do - for (const AbstractModuleRecord::ModuleRequest& request : module->requestedModules()) { - // 2.d.i. If AllImportAttributesSupported(request.[[Attributes]]) is false, then - // 2.d.i.1. Let error be ThrowCompletion(a newly created SyntaxError object). - // 2.d.i.2. Perform ContinueModuleLoading(state, error). - // (Not possible.) - // 2.d.ii. Else if module.[[LoadedModules]] contains a LoadedModuleRequest Record record such that ModuleRequestsEqual(record, request) is true, then - if (auto iter = module->loadedModules().find(ModuleMapKey { request.m_specifier.impl(), request.type() }); iter != module->loadedModules().end()) { - checkSafeToRecurse(globalObject, scope); - RETURN_IF_EXCEPTION(scope, void()); - // 2.d.ii.1. Perform InnerModuleLoading(state, record.[[Module]]). - innerModuleLoading(globalObject, state, iter->value.m_module.get()); - RETURN_IF_EXCEPTION(scope, void()); - // 2.d.iii. Else, - } else { - // 2.d.iii.1. Perform HostLoadImportedModule(module, request, state.[[HostDefined]], state). - JSPromise* promise = hostLoadImportedModule(globalObject, cyclic, request, ModuleLoaderPayload::create(vm, state), state->scriptFetcher(), true); - RETURN_IF_EXCEPTION(scope, void()); - promise->performPromiseThenWithInternalMicrotask(vm, globalObject, InternalMicrotask::ModuleGraphLoadingError, jsUndefined(), state); - // 2.d.iii.2. NOTE: HostLoadImportedModule will call FinishLoadingImportedModule, which re-enters the graph loading process through ContinueModuleLoading. + state->enqueueInnerLoad(startModule); + if (state->drainingInnerLoad()) + RELEASE_AND_RETURN(scope, void()); + state->setDrainingInnerLoad(true); + + ModuleLoaderPayload* payload = nullptr; + AbstractModuleRecord* module; + while ((module = state->takeInnerLoad())) { + // 1. Assert: state.[[IsLoading]] is true. + ASSERT(state->isLoading()); + // 2. If module is a Cyclic Module Record, module.[[Status]] is NEW, and state.[[Visited]] does not contain module, then + // (containsVisited first — it's the cheap pointer-hash check that fails + // fast for the common already-seen case before we pay for the dyncast.) + if (!state->containsVisited(module)) { + if (auto* cyclic = dynamicDowncast(module); cyclic && cyclic->status() == CyclicModuleRecord::Status::New) { + // 2.a. Append module to state.[[Visited]]. + state->appendVisited(vm, cyclic); + // 2.b. Let requestedModulesCount be the number of elements in module.[[RequestedModules]]. + size_t requestedModulesCount = module->requestedModules().size(); + // 2.c. Set state.[[PendingModulesCount]] to state.[[PendingModulesCount]] + requestedModulesCount. + state->setPendingModulesCount(state->pendingModulesCount() + requestedModulesCount); + bool loadedModulesEmpty = module->loadedModules().isEmpty(); + // 2.d. For each ModuleRequest Record request of module.[[RequestedModules]], do + for (const AbstractModuleRecord::ModuleRequest& request : module->requestedModules()) { + // 2.d.i. If AllImportAttributesSupported(request.[[Attributes]]) is false, then + // 2.d.i.1. Let error be ThrowCompletion(a newly created SyntaxError object). + // 2.d.i.2. Perform ContinueModuleLoading(state, error). + // (Not possible.) + // 2.d.ii. Else if module.[[LoadedModules]] contains a LoadedModuleRequest Record record such that ModuleRequestsEqual(record, request) is true, then + if (!loadedModulesEmpty) { + if (auto iter = module->loadedModules().find(ModuleMapKey { request.m_specifier.impl(), request.type() }); iter != module->loadedModules().end()) { + // 2.d.ii.1. Perform InnerModuleLoading(state, record.[[Module]]). + AbstractModuleRecord* loaded = iter->value.m_module.get(); + if (state->containsVisited(loaded)) + state->setPendingModulesCount(state->pendingModulesCount() - 1); + else + state->enqueueInnerLoad(loaded); + if (!state->isLoading()) + break; + continue; + } + } + // 2.d.iii. Else, + // 2.d.iii.1. Perform HostLoadImportedModule(module, request, state.[[HostDefined]], state). + // The payload only carries `state`; one instance is sufficient for every + // request originating from this drain. Async loads stash it in their + // ModuleLoadingContext, but they all want the same state back. + if (!payload) + payload = ModuleLoaderPayload::create(vm, state); + JSPromise* promise = hostLoadImportedModule(globalObject, cyclic, request, payload, state->scriptFetcher(), true); + if (scope.exception()) [[unlikely]] { + state->setDrainingInnerLoad(false); + return; + } + promise->performPromiseThenWithInternalMicrotask(vm, globalObject, InternalMicrotask::ModuleGraphLoadingError, jsUndefined(), state); + // 2.d.iii.2. NOTE: HostLoadImportedModule will call FinishLoadingImportedModule, which re-enters the graph loading process through ContinueModuleLoading. + // 2.d.iv. If state.[[IsLoading]] is false, return UNUSED. + if (!state->isLoading()) + break; + } + if (!state->isLoading()) { + state->setDrainingInnerLoad(false); + RELEASE_AND_RETURN(scope, void()); + } } - // 2.d.iv. If state.[[IsLoading]] is false, return UNUSED. - if (!state->isLoading()) - RELEASE_AND_RETURN(scope, void()); } - } - // 3. Assert: state.[[PendingModulesCount]] ≥ 1. - ASSERT(state->pendingModulesCount() >= 1); - // 4. Set state.[[PendingModulesCount]] to state.[[PendingModulesCount]] - 1. - state->setPendingModulesCount(state->pendingModulesCount() - 1); - // 5. If state.[[PendingModulesCount]] = 0, then - if (!state->pendingModulesCount()) { - // 5.a. Set state.[[IsLoading]] to false. - state->setIsLoading(false); - // 5.b. For each Cyclic Module Record loaded of state.[[Visited]], do - state->iterateVisited([](CyclicModuleRecord* loaded) { - // 5.b.i. If loaded.[[Status]] is NEW, set loaded.[[Status]] to UNLINKED. - if (loaded->status() == CyclicModuleRecord::Status::New) - loaded->status(CyclicModuleRecord::Status::Unlinked); - }); - // 5.c. Perform ! Call(state.[[PromiseCapability]].[[Resolve]], undefined, « undefined »). - state->promise()->fulfill(vm, globalObject, module); + // 3. Assert: state.[[PendingModulesCount]] ≥ 1. + ASSERT(state->pendingModulesCount() >= 1); + // 4. Set state.[[PendingModulesCount]] to state.[[PendingModulesCount]] - 1. + state->setPendingModulesCount(state->pendingModulesCount() - 1); + // 5. If state.[[PendingModulesCount]] = 0, then + if (!state->pendingModulesCount()) { + // 5.a. Set state.[[IsLoading]] to false. + state->setIsLoading(false); + // 5.b. For each Cyclic Module Record loaded of state.[[Visited]], do + state->iterateVisited([](CyclicModuleRecord* loaded) { + // 5.b.i. If loaded.[[Status]] is NEW, set loaded.[[Status]] to UNLINKED. + if (loaded->status() == CyclicModuleRecord::Status::New) + loaded->status(CyclicModuleRecord::Status::Unlinked); + }); + // 5.c. Perform ! Call(state.[[PromiseCapability]].[[Resolve]], undefined, « undefined »). + state->promise()->fulfill(vm, globalObject, module); + break; + } } // 6. Return UNUSED. + state->setDrainingInnerLoad(false); scope.release(); } @@ -932,8 +968,25 @@ void JSModuleLoader::continueModuleLoading(JSGlobalObject* globalObject, ModuleG // 2. If moduleCompletion is a normal completion, then if (auto* module = std::get_if(&moduleCompletion)) { // 2.a. Perform InnerModuleLoading(state, moduleCompletion.[[Value]]). - innerModuleLoading(globalObject, state, *module); - RETURN_IF_EXCEPTION(scope, void()); + // Per-edge fast path: most calls land here with a module that's already + // visited (E >> V); skip the dyncast+enqueue and just decrement. + if (state->containsVisited(*module)) { + ASSERT(state->pendingModulesCount() > 1 || !state->drainingInnerLoad()); + state->setPendingModulesCount(state->pendingModulesCount() - 1); + if (!state->pendingModulesCount()) { + state->setIsLoading(false); + state->iterateVisited([](CyclicModuleRecord* loaded) { + if (loaded->status() == CyclicModuleRecord::Status::New) + loaded->status(CyclicModuleRecord::Status::Unlinked); + }); + state->promise()->fulfill(vm, globalObject, *module); + } + } else if (state->drainingInnerLoad()) + state->enqueueInnerLoad(*module); + else { + innerModuleLoading(globalObject, state, *module); + RETURN_IF_EXCEPTION(scope, void()); + } // 3. Else, } else { // 3.a. Set state.[[IsLoading]] to false. @@ -982,8 +1035,16 @@ JSPromise* JSModuleLoader::loadRequestedModules(JSGlobalObject* globalObject, Ab // 1. If hostDefined is not present, let hostDefined be empty. // 2. Let pc be ! NewPromiseCapability(%Promise%). + // Step::Main calls this once per fetched module, so concurrent callers for + // the same module would each spawn their own GraphLoadingState and re-walk + // the same subgraph. Memoize the first capability; later callers wait on it. + // Don't memoize rejection — a fresh import() after a failed load must redo + // HostLoadImportedModule for the deps so transient errors aren't sticky. + if (JSPromise* existing = module->loadRequestedModulesPromise(); existing && existing->status() != JSPromise::Status::Rejected) + return existing; JSPromise* pc = JSPromise::create(vm, globalObject->promiseStructure()); // This will eventually be resolved with an AbstractModuleRecord*. pc->markAsHandled(); + module->setLoadRequestedModulesPromise(vm, pc); // 3. Let state be the GraphLoadingState Record { [[IsLoading]]: true, [[PendingModulesCount]]: 1, [[Visited]]: « », [[PromiseCapability]]: pc, [[HostDefined]]: hostDefined }. auto state = ModuleGraphLoadingState::create(vm, pc, WTF::move(scriptFetcher)); RETURN_IF_EXCEPTION(scope, nullptr); diff --git a/Source/JavaScriptCore/runtime/ModuleGraphLoadingState.cpp b/Source/JavaScriptCore/runtime/ModuleGraphLoadingState.cpp index 03ff4121f5dd..b5e4df01c795 100644 --- a/Source/JavaScriptCore/runtime/ModuleGraphLoadingState.cpp +++ b/Source/JavaScriptCore/runtime/ModuleGraphLoadingState.cpp @@ -73,36 +73,6 @@ ModuleGraphLoadingState* ModuleGraphLoadingState::create(VM& vm, JSPromise* prom return instance; } -JSPromise* ModuleGraphLoadingState::promise() const -{ - return m_promise.get(); -} - -unsigned ModuleGraphLoadingState::pendingModulesCount() const -{ - return m_pendingModulesCount; -} - -bool ModuleGraphLoadingState::isLoading() const -{ - return m_isLoading; -} - -ScriptFetcher* ModuleGraphLoadingState::scriptFetcher() const -{ - return m_scriptFetcher.get(); -} - -void ModuleGraphLoadingState::setPendingModulesCount(unsigned count) -{ - m_pendingModulesCount = count; -} - -void ModuleGraphLoadingState::setIsLoading(bool loading) -{ - m_isLoading = loading; -} - void ModuleGraphLoadingState::appendVisited(VM& vm, CyclicModuleRecord* cyclic) { Locker locker { cellLock() }; @@ -110,9 +80,4 @@ void ModuleGraphLoadingState::appendVisited(VM& vm, CyclicModuleRecord* cyclic) m_visitedSet.add(cyclic); } -bool ModuleGraphLoadingState::containsVisited(CyclicModuleRecord* cyclic) const -{ - return m_visitedSet.contains(cyclic); -} - } // namespace JSC diff --git a/Source/JavaScriptCore/runtime/ModuleGraphLoadingState.h b/Source/JavaScriptCore/runtime/ModuleGraphLoadingState.h index e7ca4f9ecd5e..89cc9c533860 100644 --- a/Source/JavaScriptCore/runtime/ModuleGraphLoadingState.h +++ b/Source/JavaScriptCore/runtime/ModuleGraphLoadingState.h @@ -57,22 +57,31 @@ class ModuleGraphLoadingState final : public JSCell { inline static Structure* createStructure(VM&, JSGlobalObject*, JSValue); static ModuleGraphLoadingState* create(VM&, JSPromise*, RefPtr); - JSPromise* promise() const; - unsigned pendingModulesCount() const; - bool isLoading() const; - ScriptFetcher* scriptFetcher() const; + JSPromise* promise() const { return m_promise.get(); } + unsigned pendingModulesCount() const { return m_pendingModulesCount; } + bool isLoading() const { return m_isLoading; } + ScriptFetcher* scriptFetcher() const { return m_scriptFetcher.get(); } - void setPendingModulesCount(unsigned); - void setIsLoading(bool); + void setPendingModulesCount(unsigned count) { m_pendingModulesCount = count; } + void setIsLoading(bool loading) { m_isLoading = loading; } void appendVisited(VM&, CyclicModuleRecord*); - bool containsVisited(CyclicModuleRecord*) const; + bool containsVisited(const AbstractModuleRecord* record) const { return m_visitedSet.contains(record); } void iterateVisited(auto&& function) const { for (const auto& barrier : m_visited) function(barrier.get()); } + // innerModuleLoading() drains this queue iteratively instead of recursing + // through hostLoadImportedModule -> continueModuleLoading. Entries are only + // present while a drain is on the stack and are otherwise reachable via the + // module registry / referrer.loadedModules, so raw pointers need no GC visit. + void enqueueInnerLoad(AbstractModuleRecord* record) { m_innerLoadQueue.append(record); } + AbstractModuleRecord* takeInnerLoad() { return m_innerLoadQueue.isEmpty() ? nullptr : m_innerLoadQueue.takeLast(); } + bool drainingInnerLoad() const { return m_drainingInnerLoad; } + void setDrainingInnerLoad(bool draining) { m_drainingInnerLoad = draining; } + private: ModuleGraphLoadingState(VM&, Structure*, JSPromise*, RefPtr); @@ -82,12 +91,16 @@ class ModuleGraphLoadingState final : public JSCell { WriteBarrier m_promise; // [[Visited]] Vector, 8> m_visited; - // Contains the same contents as m_visited, so no write barriers needed. - UncheckedKeyHashSet m_visitedSet; + // Contains the same contents as m_visited, so no write barriers needed. Typed + // as AbstractModuleRecord* so the hot containsVisited() check can run before + // dynamicDowncast on the per-edge fast path. + UncheckedKeyHashSet m_visitedSet; + Vector m_innerLoadQueue; // [[PendingModulesCount]] unsigned m_pendingModulesCount { 1 }; // [[IsLoading]] bool m_isLoading { true }; + bool m_drainingInnerLoad { false }; // [[HostDefined]] const RefPtr m_scriptFetcher; }; From 143fb225e137506e8bb9de59a0b9a646bb41aa25 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sun, 26 Apr 2026 01:24:57 +0000 Subject: [PATCH 2/3] drop unconditional resolveExportImpl cache read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit JSTests/modules/self-star-link.js: an Indirect cycle that falls back to star (JSC's IndirectFallback) makes the cached Resolved value path-dependent — C resolved as a root caches 'D', but C reached from A must yield 'E'. The original !foundStarLinks gate is correct here. The getModuleNamespace BFS doesn't depend on this read, so no perf change. --- .../runtime/AbstractModuleRecord.cpp | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp b/Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp index a78b4f3fdee2..0b3ae1122b69 100644 --- a/Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp +++ b/Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp @@ -689,16 +689,13 @@ auto AbstractModuleRecord::resolveExportImpl(JSGlobalObject* globalObject, const if (!moduleRecord->starExportEntries().isEmpty()) foundStarLinks = true; - // The cache only stores Resolved entries. A Resolved cached value means - // ResolveExport(moduleRecord, name, []) found exactly one binding T; - // any non-empty resolveSet can only turn paths to T into null, never - // surface a different binding (that would have made the [] result - // ambiguous and therefore uncached). null vs T merges to T either way, - // so reading the cache here is sound regardless of foundStarLinks. - if (std::optional cachedResolution = moduleRecord->tryGetCachedResolution(query.exportName.get())) { - if (!mergeToCurrentTop(*cachedResolution)) - return Resolution::ambiguous(); - continue; + // 4. Once we follow star links, we should not retrieve the result from the cache and should not cache the result. + if (!foundStarLinks) { + if (std::optional cachedResolution = moduleRecord->tryGetCachedResolution(query.exportName.get())) { + if (!mergeToCurrentTop(*cachedResolution)) + return Resolution::ambiguous(); + continue; + } } const std::optional optionalExportEntry = moduleRecord->tryGetExportEntry(query.exportName.get()); From 28f2acb75b8ba161d88bec343f863c16764c388d Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sun, 26 Apr 2026 03:37:05 +0000 Subject: [PATCH 3/3] drop loadRequestedModules memoization The per-module WriteBarrier kept one extra Promise alive for the lifetime of every module record (visible in fetch-leak's heap-stats threshold). The memoization only helps the rare case of two concurrent import()s of the same module before either fetches; the original per-state walk handles that correctly, just with a redundant traversal. Not worth a permanent +1 Promise per module. --- Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp | 6 ------ Source/JavaScriptCore/runtime/AbstractModuleRecord.h | 7 ------- Source/JavaScriptCore/runtime/JSModuleLoader.cpp | 8 -------- 3 files changed, 21 deletions(-) diff --git a/Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp b/Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp index 0b3ae1122b69..c38ea1d3f73f 100644 --- a/Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp +++ b/Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp @@ -96,7 +96,6 @@ void AbstractModuleRecord::visitChildrenImpl(JSCell* cell, Visitor& visitor) visitor.append(thisObject->m_cycleRoot); visitor.append(thisObject->m_topLevelCapability); visitor.append(thisObject->m_asyncCapability); - visitor.append(thisObject->m_loadRequestedModulesPromise); Locker locker { thisObject->cellLock() }; visitor.append(thisObject->m_asyncParentModules.begin(), thisObject->m_asyncParentModules.end()); auto values = thisObject->m_dependencies.values(); @@ -945,11 +944,6 @@ void AbstractModuleRecord::asyncCapability(VM& vm, JSPromise* promise) m_asyncCapability.set(vm, this, promise); } -void AbstractModuleRecord::setLoadRequestedModulesPromise(VM& vm, JSPromise* promise) -{ - m_loadRequestedModulesPromise.set(vm, this, promise); -} - void AbstractModuleRecord::setModuleEnvironment(JSGlobalObject* globalObject, JSModuleEnvironment* moduleEnvironment) { VM& vm = globalObject->vm(); diff --git a/Source/JavaScriptCore/runtime/AbstractModuleRecord.h b/Source/JavaScriptCore/runtime/AbstractModuleRecord.h index 86d8b5809af7..fa9b74535710 100644 --- a/Source/JavaScriptCore/runtime/AbstractModuleRecord.h +++ b/Source/JavaScriptCore/runtime/AbstractModuleRecord.h @@ -171,8 +171,6 @@ class AbstractModuleRecord : public JSInternalFieldObjectImpl<2> { AsyncEvaluationOrder asyncEvaluationOrder() const { return m_asyncEvaluationOrder; } std::optional pendingAsyncDependencies() const { return m_pendingAsyncDependencies; } bool hasTLA() const { return m_hasTLA; } - JSPromise* loadRequestedModulesPromise() const { return m_loadRequestedModulesPromise.get(); } - void setLoadRequestedModulesPromise(VM&, JSPromise*); JSPromise* topLevelCapability() const { return m_topLevelCapability.get(); } void cycleRoot(VM&, CyclicModuleRecord*); @@ -283,11 +281,6 @@ class AbstractModuleRecord : public JSInternalFieldObjectImpl<2> { WriteBarrier m_asyncCapability; - // Memoized result of LoadRequestedModules() — every concurrent caller waits - // on the first GraphLoadingState's capability instead of spawning its own - // and re-walking the same subgraph. - WriteBarrier m_loadRequestedModulesPromise; - // We assume that all the AbstractModuleRecord are retained by JSModuleLoader's registry. // So here, we don't visit each object for GC. The resolution cache map caches the once // looked up correctly resolved resolution, since (1) we rarely looked up the non-resolved one, diff --git a/Source/JavaScriptCore/runtime/JSModuleLoader.cpp b/Source/JavaScriptCore/runtime/JSModuleLoader.cpp index 0b66165afa50..a170ec743325 100644 --- a/Source/JavaScriptCore/runtime/JSModuleLoader.cpp +++ b/Source/JavaScriptCore/runtime/JSModuleLoader.cpp @@ -1035,16 +1035,8 @@ JSPromise* JSModuleLoader::loadRequestedModules(JSGlobalObject* globalObject, Ab // 1. If hostDefined is not present, let hostDefined be empty. // 2. Let pc be ! NewPromiseCapability(%Promise%). - // Step::Main calls this once per fetched module, so concurrent callers for - // the same module would each spawn their own GraphLoadingState and re-walk - // the same subgraph. Memoize the first capability; later callers wait on it. - // Don't memoize rejection — a fresh import() after a failed load must redo - // HostLoadImportedModule for the deps so transient errors aren't sticky. - if (JSPromise* existing = module->loadRequestedModulesPromise(); existing && existing->status() != JSPromise::Status::Rejected) - return existing; JSPromise* pc = JSPromise::create(vm, globalObject->promiseStructure()); // This will eventually be resolved with an AbstractModuleRecord*. pc->markAsHandled(); - module->setLoadRequestedModulesPromise(vm, pc); // 3. Let state be the GraphLoadingState Record { [[IsLoading]]: true, [[PendingModulesCount]]: 1, [[Visited]]: « », [[PromiseCapability]]: pc, [[HostDefined]]: hostDefined }. auto state = ModuleGraphLoadingState::create(vm, pc, WTF::move(scriptFetcher)); RETURN_IF_EXCEPTION(scope, nullptr);