Repository: nodejs/node
Pull request: src: cache missing package.json files in the C++ package config cache

Fixes https://github.com/nodejs/node/issues/60397

In https://github.com/nodejs/node/pull/59888 the nearest parent package JSON cache `package_json_reader.js` was adjusted from a map from any given module path to a representation of its parent `package.json` file to a map from `package.json` paths to a deserialized representation of their content. This addressed excessive memory usage caused by repeatedly caching identical deserialized `package.json` objects for modules that shared a parent `package.json`, but also reintroduced a filesystem traversal in `package_json_reader.js` which finds the nearest parent `package.json` file for a given module. The `stat` calls in this traversal are not cached, so we end up potentially issuing them for a bunch of duplicate paths. In the reported issue, this leads to poor performance for users using potentially high-latency network filesystems. Similar poor performance is also observed in Node versions that lack https://github.com/nodejs/node/pull/59086, which (re)introduced the JS-side cache initially.

This PR addresses this by unwinding the changes in https://github.com/nodejs/node/pull/59888 and instead making the C++-side `package.json` cache a bit more expressive, caching both a deserialized representation of a `package.json` file at a given path, as well as an indicator if no such file exists (modeled as an `std::optional`). This addresses the poor performance reported in https://github.com/nodejs/node/issues/60397 by:
1. Removing the repeated `stat` calls in `package_json_reader.js`
2. Avoiding repetitive attempts to read non-existent `package.json` paths on the C++ side, which also perform poorly on high-latency filesystems

While analyzing the performance of these changes, I noticed a confounding factor which is that the lazy-parsing and caching of `imports` and `exports` on deserialized package configuration objects in `deserializePackageJSON` wasn't working as expected and was also contributing to the varying performance we've been seeing across these changes:
1. The attempt to define lazy properties to parse and cache the JSON on demand didn't work as expected because the resulting object was immediately spread, meaning we'd immediately run the JSON parsing code anyway
2. Because the parsed representation of `imports` and `exports` is cached on deserialized `package.json` objects, it's important that a given `package.json` file map to the _same_ deserialized object. If we don't do this, we repeatedly re-parse these fields redundantly across calls. This motivates the sort of strange two-level caching scheme in `getNearestParentPackageJSON` that these changes introduce. The downside here is that we potentially redundantly call into `modulesBinding.getNearestParentPackageJSON` for a given path just to resolve the path to a `package.json` file that we may already have cached, but I don't see any way to avoid this.

# Benchmarks

I benchmarked this change with the same scripts I used in https://github.com/nodejs/node/pull/59888. The first is the reproduction script from https://github.com/nodejs/node/issues/58126:
```javascript
require('dd-trace').init();
const cdk = require('aws-cdk-lib');

const app = new cdk.App();
for (let i = 0; i < 1000; i++) {
  new cdk.Stack(app, `DdTraceStack${I}`)
}
```

The second is this:
```javascript
for (let i = 0; i < 1000; i++) {
  require('date-fns');
}
```

Each benchmark compares v22.19.0 (which does not include https://github.com/nodejs/node/pull/59888), v25.1.0 (the latest current release, which does include https://github.com/nodejs/node/pull/59888), and this change (which is just the `node` directory in the output).

## Fast disk

### `ddtrace` + CDK

```
➜ hyperfine --warmup 10 -L node_path ../node/node,../node_worktrees/v25.1.0/node,../node_worktrees/v22.19.0/node "{node_path} dd-cdk-benchmark.js"
Benchmark 1: ../node/node dd-cdk-benchmark.js
  Time (mean ± σ):     161.6 ms ±   1.6 ms    [User: 170.7 ms, System: 20.9 ms]
  Range (min … max):   159.2 ms … 164.9 ms    18 runs

Benchmark 2: ../node_worktrees/v25.1.0/node dd-cdk-benchmark.js
  Time (mean ± σ):     164.9 ms ±   0.9 ms    [User: 174.7 ms, System: 22.8 ms]
  Range (min … max):   163.7 ms … 166.8 ms    17 runs

Benchmark 3: ../node_worktrees/v22.19.0/node dd-cdk-benchmark.js
  Time (mean ± σ):     169.5 ms ±   1.0 ms    [User: 172.4 ms, System: 20.6 ms]
  Range (min … max):   167.9 ms … 172.0 ms    17 runs

Summary
  ../node/node dd-cdk-benchmark.js ran
    1.02 ± 0.01 times faster than ../node_worktrees/v25.1.0/node dd-cdk-benchmark.js
    1.05 ± 0.01 times faster than ../node_worktrees/v22.19.0/node dd-cdk-benchmark.js
```

### `date-fns`

```
➜ hyperfine --warmup 10 -L node_path ../node/node,../node_worktrees/v25.1.0/node,../node_worktrees/v22.19.0/node "{node_path} date-fns-benchmark.js"
Benchmark 1: ../node/node date-fns-benchmark.js
  Time (mean ± σ):      71.3 ms ±   1.1 ms    [User: 73.4 ms, System: 13.3 ms]
  Range (min … max):    69.7 ms …  75.2 ms    41 runs

Benchmark 2: ../node_worktrees/v25.1.0/node date-fns-benchmark.js
  Time (mean ± σ):      66.3 ms ±   0.7 ms    [User: 66.8 ms, System: 10.4 ms]
  Range (min … max):    64.9 ms …  69.1 ms    43 runs

Benchmark 3: ../node_worktrees/v22.19.0/node date-fns-benchmark.js
  Time (mean ± σ):     115.9 ms ±   0.7 ms    [User: 139.6 ms, System: 18.4 ms]
  Range (min … max):   114.1 ms … 117.5 ms    25 runs

Summary
  ../node_worktrees/v25.1.0/node date-fns-benchmark.js ran
    1.07 ± 0.02 times faster than ../node/node date-fns-benchmark.js
    1.75 ± 0.02 times faster than ../node_worktrees/v22.19.0/node date-fns-benchmark.js
```

## Slow disk

I emulated this by mounting an NFS volume from `localhost` with `noac` (to disable most caching).

### `ddtrace` + CDK

```
➜ hyperfine --warmup 10 -L node_path ../../node/node,../../node_worktrees/v25.1.0/node,../../node_worktrees/v22.19.0/node "{node_path} dd-cdk-benchmark.js"
Benchmark 1: ../../node/node dd-cdk-benchmark.js
  Time (mean ± σ):      3.976 s ±  1.193 s    [User: 0.210 s, System: 0.148 s]
  Range (min … max):    1.924 s …  5.314 s    10 runs

Benchmark 2: ../../node_worktrees/v25.1.0/node dd-cdk-benchmark.js
  Time (mean ± σ):      7.668 s ±  2.805 s    [User: 0.210 s, System: 0.345 s]
  Range (min … max):    5.307 s … 12.359 s    10 runs

Benchmark 3: ../../node_worktrees/v22.19.0/node dd-cdk-benchmark.js
  Time (mean ± σ):      3.810 s ±  1.343 s    [User: 0.205 s, System: 0.167 s]
  Range (min … max):    2.266 s …  5.542 s    10 runs

Summary
  ../../node_worktrees/v22.19.0/node dd-cdk-benchmark.js ran
    1.04 ± 0.48 times faster than ../../node/node dd-cdk-benchmark.js
    3.01 ± 1.02 times faster than ../../node_worktrees/v25.1.0/node dd-cdk-benchmark.js
```

### `date-fns`

```
➜ hyperfine --warmup 10 -L node_path ../../node/node,../../node_worktrees/v25.1.0/node,../../node_worktrees/v22.19.0/node "{node_path} date-fns-benchmark.js"
Benchmark 1: ../../node/node date-fns-benchmark.js
  Time (mean ± σ):     699.7 ms ±  20.3 ms    [User: 65.4 ms, System: 46.3 ms]
  Range (min … max):   668.5 ms … 727.1 ms    10 runs

Benchmark 2: ../../node_worktrees/v25.1.0/node date-fns-benchmark.js
  Time (mean ± σ):     977.5 ms ±  41.0 ms    [User: 68.1 ms, System: 62.0 ms]
  Range (min … max):   923.2 ms … 1038.0 ms    10 runs

Benchmark 3: ../../node_worktrees/v22.19.0/node date-fns-benchmark.js
  Time (mean ± σ):     825.4 ms ± 311.2 ms    [User: 112.7 ms, System: 60.5 ms]
  Range (min … max):   542.0 ms … 1350.4 ms    10 runs

Summary
  ../../node/node date-fns-benchmark.js ran
    1.18 ± 0.45 times faster than ../../node_worktrees/v22.19.0/node date-fns-benchmark.js
    1.40 ± 0.07 times faster than ../../node_worktrees/v25.1.0/node date-fns-benchmark.js
```

Changed files:
filename: lib/internal/modules/package_json_reader.js
patch: @@ -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;
 }
 

filename: src/node_modules.cc
patch: @@ -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<Value>& args) {
@@ -321,24 +333,49 @@ const BindingData::PackageConfig* BindingData::TraverseParent(
   return nullptr;
 }
 
-void BindingData::GetNearestParentPackageJSONType(
-    const FunctionCallbackInfo<Value>& args) {
+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);
+
+  ToNamespacedPath(realm->env(), path_value);
+
+  auto path = path_value->ToPath();
+
+  if (slashCheck) {
+    path /= "";
+  }
+
+  return path;
+}
+
+void BindingData::GetNearestParentPackageJSON(
+    const v8::FunctionCallbackInfo<v8::Value>& 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);
+  auto path = NormalizePath(realm, &path_value);
 
-  auto path = path_value.ToPath();
+  auto package_json = TraverseParent(realm, path);
 
-  if (slashCheck) {
-    path /= "";
+  if (package_json != nullptr) {
+    args.GetReturnValue().Set(package_json->Serialize(realm));
   }
+}
+
+void BindingData::GetNearestParentPackageJSONType(
+    const FunctionCallbackInfo<Value>& 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);
 
@@ -569,6 +606,10 @@ void BindingData::CreatePerIsolateProperties(IsolateData* isolate_data,
             target,
             "getNearestParentPackageJSONType",
             GetNearestParentPackageJSONType);
+  SetMethod(isolate,
+            target,
+            "getNearestParentPackageJSON",
+            GetNearestParentPackageJSON);
   SetMethod(
       isolate, target, "getPackageScopeConfig", GetPackageScopeConfig<false>);
   SetMethod(isolate, target, "getPackageType", GetPackageScopeConfig<true>);
@@ -624,6 +665,7 @@ void BindingData::RegisterExternalReferences(
     ExternalReferenceRegistry* registry) {
   registry->Register(ReadPackageJSON);
   registry->Register(GetNearestParentPackageJSONType);
+  registry->Register(GetNearestParentPackageJSON);
   registry->Register(GetPackageScopeConfig<false>);
   registry->Register(GetPackageScopeConfig<true>);
   registry->Register(EnableCompileCache);

filename: src/node_modules.h
patch: @@ -55,6 +55,8 @@ class BindingData : public SnapshotableObject {
   SET_MEMORY_INFO_NAME(BindingData)
 
   static void ReadPackageJSON(const v8::FunctionCallbackInfo<v8::Value>& args);
+  static void GetNearestParentPackageJSON(
+      const v8::FunctionCallbackInfo<v8::Value>& args);
   static void GetNearestParentPackageJSONType(
       const v8::FunctionCallbackInfo<v8::Value>& args);
   template <bool return_only_type>
@@ -72,8 +74,17 @@ class BindingData : public SnapshotableObject {
   static void RegisterExternalReferences(ExternalReferenceRegistry* registry);
 
  private:
-  std::unordered_map<std::string, PackageConfig> 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<std::string, std::optional<PackageConfig> >
+      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,

filename: typings/internalBinding/modules.d.ts
patch: @@ -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 }