From 85426cae10ec4178bbed0eb6ce801f80bf768489 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Sun, 26 Oct 2025 21:27:52 -0400 Subject: [PATCH 1/2] src: cache missing package.json files in the C++ package config cache --- lib/internal/modules/package_json_reader.js | 133 +++++++++----------- src/node_modules.cc | 55 +++++++- src/node_modules.h | 11 +- typings/internalBinding/modules.d.ts | 1 + 4 files changed, 123 insertions(+), 77 deletions(-) diff --git a/lib/internal/modules/package_json_reader.js b/lib/internal/modules/package_json_reader.js index af8fcd28c5259e..6c6bf0383bc338 100644 --- a/lib/internal/modules/package_json_reader.js +++ b/lib/internal/modules/package_json_reader.js @@ -6,9 +6,7 @@ const { ObjectDefineProperty, RegExpPrototypeExec, SafeMap, - StringPrototypeEndsWith, StringPrototypeIndexOf, - StringPrototypeLastIndexOf, StringPrototypeSlice, } = primordials; const { @@ -28,11 +26,9 @@ const { const { kEmptyObject } = require('internal/util'); const modulesBinding = internalBinding('modules'); const path = require('path'); -const permission = require('internal/process/permission'); const { validateString } = require('internal/validators'); const internalFsBinding = internalBinding('fs'); -const nearestParentPackageJSONCache = new SafeMap(); /** * @typedef {import('typings/internalBinding/modules').DeserializedPackageConfig} DeserializedPackageConfig @@ -68,28 +64,41 @@ function deserializePackageJSON(path, contents) { const pjsonPath = optionalFilePath ?? path; - return { - data: { + const data = { + __proto__: null, + ...(name != null && { name }), + ...(main != null && { main }), + ...(type != null && { type }), + }; + + if (plainExports !== null) { + ObjectDefineProperty(data, 'exports', { + __proto__: null, + configurable: true, + enumerable: true, + get() { + const value = requiresJSONParse(plainExports) ? JSONParse(plainExports) : plainExports; + ObjectDefineProperty(data, 'exports', { __proto__: null, enumerable: true, value }); + return value; + }, + }); + } + + if (plainImports !== null) { + ObjectDefineProperty(data, 'imports', { __proto__: null, - ...(name != null && { name }), - ...(main != null && { main }), - ...(type != null && { type }), - ...(plainImports != null && { - // This getters are used to lazily parse the imports and exports fields. - get imports() { - const value = requiresJSONParse(plainImports) ? JSONParse(plainImports) : plainImports; - ObjectDefineProperty(this, 'imports', { __proto__: null, value }); - return this.imports; - }, - }), - ...(plainExports != null && { - get exports() { - const value = requiresJSONParse(plainExports) ? JSONParse(plainExports) : plainExports; - ObjectDefineProperty(this, 'exports', { __proto__: null, value }); - return this.exports; - }, - }), - }, + configurable: true, + enumerable: true, + get() { + const value = requiresJSONParse(plainImports) ? JSONParse(plainImports) : plainImports; + ObjectDefineProperty(data, 'imports', { __proto__: null, enumerable: true, value }); + return value; + }, + }); + } + + return { + data, exists: true, path: pjsonPath, }; @@ -131,43 +140,23 @@ function read(jsonPath, { base, specifier, isESM } = kEmptyObject) { } /** - * Given a file path, walk the filesystem upwards until we find its closest parent - * `package.json` file, stopping when: - * 1. we find a `package.json` file; - * 2. we find a path that we do not have permission to read; - * 3. we find a containing `node_modules` directory; - * 4. or, we reach the filesystem root - * @returns {undefined | string} + * A cache mapping a module's path to its parent `package.json` file's path. + * This is used in concert with `deserializedPackageJSONCache` to improve + * the performance of `getNearestParentPackageJSON` when called repeatedly + * on the same module paths. */ -function findParentPackageJSON(checkPath) { - const enabledPermission = permission.isEnabled(); - - const rootSeparatorIndex = StringPrototypeIndexOf(checkPath, path.sep); - let separatorIndex; - - do { - separatorIndex = StringPrototypeLastIndexOf(checkPath, path.sep); - checkPath = StringPrototypeSlice(checkPath, 0, separatorIndex); +const moduleToParentPackageJSONCache = new SafeMap(); - if (enabledPermission && !permission.has('fs.read', checkPath + path.sep)) { - return undefined; - } - - if (StringPrototypeEndsWith(checkPath, path.sep + 'node_modules')) { - return undefined; - } - - const maybePackageJSONPath = checkPath + path.sep + 'package.json'; - const stat = internalFsBinding.internalModuleStat(checkPath + path.sep + 'package.json'); - - const packageJSONExists = stat === 0; - if (packageJSONExists) { - return maybePackageJSONPath; - } - } while (separatorIndex > rootSeparatorIndex); - - return undefined; -} +/** + * A cache mapping the path of a `package.json` file to its + * {@link DeserializedPackageConfig deserialized representation}, + * as produced by {@link deserializedPackageJSONCache}. The purpose of this + * cache is to ensure that we always return the same + * {@link DeserializedPackageConfig} instance for a given `package.json`, + * which is necessary to ensure that we don't re-parse `imports` and + * `exports` redundantly. + */ +const deserializedPackageJSONCache = new SafeMap(); /** * Get the nearest parent package.json file from a given path. @@ -176,26 +165,22 @@ function findParentPackageJSON(checkPath) { * @returns {undefined | DeserializedPackageConfig} */ function getNearestParentPackageJSON(checkPath) { - const nearestParentPackageJSON = findParentPackageJSON(checkPath); - - if (nearestParentPackageJSON === undefined) { - return undefined; + const parentPackageJSONPath = moduleToParentPackageJSONCache.get(checkPath); + if (parentPackageJSONPath !== undefined) { + return deserializedPackageJSONCache.get(parentPackageJSONPath); } - if (nearestParentPackageJSONCache.has(nearestParentPackageJSON)) { - return nearestParentPackageJSONCache.get(nearestParentPackageJSON); - } + const result = modulesBinding.getNearestParentPackageJSON(checkPath); + const packageConfig = deserializePackageJSON(checkPath, result); - const result = modulesBinding.readPackageJSON(nearestParentPackageJSON); + moduleToParentPackageJSONCache.set(checkPath, packageConfig.path); - if (result === undefined) { - nearestParentPackageJSONCache.set(checkPath, undefined); - return undefined; + const maybeCachedPackageConfig = deserializedPackageJSONCache.get(packageConfig.path); + if (maybeCachedPackageConfig !== undefined) { + return maybeCachedPackageConfig; } - const packageConfig = deserializePackageJSON(checkPath, result); - nearestParentPackageJSONCache.set(nearestParentPackageJSON, packageConfig); - + deserializedPackageJSONCache.set(packageConfig.path, packageConfig); return packageConfig; } diff --git a/src/node_modules.cc b/src/node_modules.cc index 9131c4f27e7d83..17ca70fbf2897d 100644 --- a/src/node_modules.cc +++ b/src/node_modules.cc @@ -97,15 +97,27 @@ const BindingData::PackageConfig* BindingData::GetPackageJSON( auto cache_entry = binding_data->package_configs_.find(path.data()); if (cache_entry != binding_data->package_configs_.end()) { - return &cache_entry->second; + auto& cache_value = cache_entry->second; + if (cache_value) { + return &*cache_value; + } + + // If we have a cache entry without a value, we've already + // attempted to open and read this path and couldn't (it most + // likely doesn't exist) + return nullptr; } PackageConfig package_config{}; package_config.file_path = path; // No need to exclude BOM since simdjson will skip it. if (ReadFileSync(&package_config.raw_json, path.data()) < 0) { + // Add `nullopt` to the package config cache so that we don't + // need to open and attempt to read this path again + binding_data->package_configs_.insert({std::string(path), std::nullopt}); return nullptr; } + simdjson::ondemand::document document; simdjson::ondemand::object main_object; simdjson::error_code error = @@ -238,7 +250,7 @@ const BindingData::PackageConfig* BindingData::GetPackageJSON( auto cached = binding_data->package_configs_.insert( {std::string(path), std::move(package_config)}); - return &cached.first->second; + return &*cached.first->second; } void BindingData::ReadPackageJSON(const FunctionCallbackInfo& args) { @@ -321,6 +333,40 @@ const BindingData::PackageConfig* BindingData::TraverseParent( return nullptr; } +void BindingData::GetNearestParentPackageJSON( + const v8::FunctionCallbackInfo& args) { + CHECK_GE(args.Length(), 1); + CHECK(args[0]->IsString()); + + Realm* realm = Realm::GetCurrent(args); + BufferValue path_value(realm->isolate(), args[0]); + // Check if the path has a trailing slash. If so, add it after + // ToNamespacedPath() as it will be deleted by ToNamespacedPath() + bool slashCheck = path_value.ToStringView().ends_with(kPathSeparator); + + ToNamespacedPath(realm->env(), &path_value); + + std::string path_value_str = path_value.ToString(); + if (slashCheck) { + path_value_str.push_back(kPathSeparator); + } + + std::filesystem::path path; + +#ifdef _WIN32 + std::wstring wide_path = ConvertToWideString(path_value_str, GetACP()); + path = std::filesystem::path(wide_path); +#else + path = std::filesystem::path(path_value_str); +#endif + + auto package_json = TraverseParent(realm, path); + + if (package_json != nullptr) { + args.GetReturnValue().Set(package_json->Serialize(realm)); + } +} + void BindingData::GetNearestParentPackageJSONType( const FunctionCallbackInfo& args) { CHECK_GE(args.Length(), 1); @@ -569,6 +615,10 @@ void BindingData::CreatePerIsolateProperties(IsolateData* isolate_data, target, "getNearestParentPackageJSONType", GetNearestParentPackageJSONType); + SetMethod(isolate, + target, + "getNearestParentPackageJSON", + GetNearestParentPackageJSON); SetMethod( isolate, target, "getPackageScopeConfig", GetPackageScopeConfig); SetMethod(isolate, target, "getPackageType", GetPackageScopeConfig); @@ -624,6 +674,7 @@ void BindingData::RegisterExternalReferences( ExternalReferenceRegistry* registry) { registry->Register(ReadPackageJSON); registry->Register(GetNearestParentPackageJSONType); + registry->Register(GetNearestParentPackageJSON); registry->Register(GetPackageScopeConfig); registry->Register(GetPackageScopeConfig); registry->Register(EnableCompileCache); diff --git a/src/node_modules.h b/src/node_modules.h index e4ba6b75bc86d1..78aacda7f66d00 100644 --- a/src/node_modules.h +++ b/src/node_modules.h @@ -55,6 +55,8 @@ class BindingData : public SnapshotableObject { SET_MEMORY_INFO_NAME(BindingData) static void ReadPackageJSON(const v8::FunctionCallbackInfo& args); + static void GetNearestParentPackageJSON( + const v8::FunctionCallbackInfo& args); static void GetNearestParentPackageJSONType( const v8::FunctionCallbackInfo& args); template @@ -72,7 +74,14 @@ class BindingData : public SnapshotableObject { static void RegisterExternalReferences(ExternalReferenceRegistry* registry); private: - std::unordered_map package_configs_; + /* + * This map caches `PackageConfig` values by `package.json` path. + * An empty optional value indicates that no `package.json` file + * at the given path exists, which we cache to avoid repeated + * attempts to open the same non-existent paths. + */ + std::unordered_map > + package_configs_; simdjson::ondemand::parser json_parser; // returns null on error static const PackageConfig* GetPackageJSON( diff --git a/typings/internalBinding/modules.d.ts b/typings/internalBinding/modules.d.ts index c5a79ba9c2957e..0b1d0e2938319f 100644 --- a/typings/internalBinding/modules.d.ts +++ b/typings/internalBinding/modules.d.ts @@ -23,6 +23,7 @@ export type SerializedPackageConfig = [ export interface ModulesBinding { readPackageJSON(path: string): SerializedPackageConfig | undefined; getNearestParentPackageJSONType(path: string): PackageConfig['type'] + getNearestParentPackageJSON(path: string): SerializedPackageConfig | undefined getPackageScopeConfig(path: string): SerializedPackageConfig | undefined getPackageType(path: string): PackageConfig['type'] | undefined enableCompileCache(path?: string): { status: number, message?: string, directory?: string } From dd093fe9592dba130efc62bf21b6379d5c34d69e Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Sat, 8 Nov 2025 10:44:52 -0500 Subject: [PATCH 2/2] feedback and incorporate changes to path handling from rebase --- src/node_modules.cc | 47 ++++++++++++++++++--------------------------- src/node_modules.h | 2 ++ 2 files changed, 21 insertions(+), 28 deletions(-) diff --git a/src/node_modules.cc b/src/node_modules.cc index 17ca70fbf2897d..08177c5b3eb8b5 100644 --- a/src/node_modules.cc +++ b/src/node_modules.cc @@ -333,32 +333,32 @@ const BindingData::PackageConfig* BindingData::TraverseParent( return nullptr; } -void BindingData::GetNearestParentPackageJSON( - const v8::FunctionCallbackInfo& args) { - CHECK_GE(args.Length(), 1); - CHECK(args[0]->IsString()); - - Realm* realm = Realm::GetCurrent(args); - BufferValue path_value(realm->isolate(), args[0]); +const std::filesystem::path BindingData::NormalizePath( + Realm* realm, BufferValue* path_value) { // Check if the path has a trailing slash. If so, add it after // ToNamespacedPath() as it will be deleted by ToNamespacedPath() - bool slashCheck = path_value.ToStringView().ends_with(kPathSeparator); + bool slashCheck = path_value->ToStringView().ends_with(kPathSeparator); - ToNamespacedPath(realm->env(), &path_value); + ToNamespacedPath(realm->env(), path_value); + + auto path = path_value->ToPath(); - std::string path_value_str = path_value.ToString(); if (slashCheck) { - path_value_str.push_back(kPathSeparator); + path /= ""; } - std::filesystem::path path; + return path; +} -#ifdef _WIN32 - std::wstring wide_path = ConvertToWideString(path_value_str, GetACP()); - path = std::filesystem::path(wide_path); -#else - path = std::filesystem::path(path_value_str); -#endif +void BindingData::GetNearestParentPackageJSON( + const v8::FunctionCallbackInfo& args) { + CHECK_GE(args.Length(), 1); + CHECK(args[0]->IsString()); + + Realm* realm = Realm::GetCurrent(args); + BufferValue path_value(realm->isolate(), args[0]); + + auto path = NormalizePath(realm, &path_value); auto package_json = TraverseParent(realm, path); @@ -374,17 +374,8 @@ void BindingData::GetNearestParentPackageJSONType( Realm* realm = Realm::GetCurrent(args); BufferValue path_value(realm->isolate(), args[0]); - // Check if the path has a trailing slash. If so, add it after - // ToNamespacedPath() as it will be deleted by ToNamespacedPath() - bool slashCheck = path_value.ToStringView().ends_with(kPathSeparator); - - ToNamespacedPath(realm->env(), &path_value); - - auto path = path_value.ToPath(); - if (slashCheck) { - path /= ""; - } + auto path = NormalizePath(realm, &path_value); auto package_json = TraverseParent(realm, path); diff --git a/src/node_modules.h b/src/node_modules.h index 78aacda7f66d00..d610306a3a3111 100644 --- a/src/node_modules.h +++ b/src/node_modules.h @@ -83,6 +83,8 @@ class BindingData : public SnapshotableObject { std::unordered_map > package_configs_; simdjson::ondemand::parser json_parser; + static const std::filesystem::path NormalizePath(Realm* realm, + BufferValue* path_value); // returns null on error static const PackageConfig* GetPackageJSON( Realm* realm,