Repository: nodejs/node
Pull request: src: reduce the nearest parent package JSON cache size

Fixes https://github.com/nodejs/node/issues/59733

This PR addresses an increase in heap usage in Node versions containing the change introduced in https://github.com/nodejs/node/pull/59086. That change added a cache to `getNearestParentPackageJSON` in `package_reader_json` to avoid the overhead of repeatedly calling in to C++ when this function is invoked repeatedly on the same file path. There was a similar cache in the original implementation that existed prior to a change (https://github.com/nodejs/node/pull/50322) which moved this `package.json` reading to C++. However, the original cache was a map from `package.json` path to a deserialized object, whereas the new cache is a map from any given module file path to an object representing its parent `package.json` file. Because it's common for multiple files to share a parent `package.json`, this cache often contains many more entries than original cache, with many of them pointing to duplicate data.

I've addressed the issue here by moving the filesystem traversal which finds the parent `package.json` file for a given file back to the JS side of the fence, and adjusting the cache to be a map from `package.json` file path to its deserialized object. Doing this makes the cache size manageable again. The traversal code is basically lifted from the original JS-side implementation that got removed. `GetNearestParentPackageJSON` on the C++ side isn't called anywhere anymore, so I've also removed that.

## Memory benchmarks

I measured memory usage with this script. `date-fns` is structured in a way that clearly exhibits the heap usage change:

```json
{
  "dependencies": {
    "date-fns": "^4.1.0",
  }
}
```

```javascript
const dateFns = require('date-fns');

const main = async () => {
  await global.gc({ execution: 'async', type: 'major-snapshot', filename: `heap-${process.version}.heapsnapshot` });
  console.log(process.memoryUsage());
};

main().then(() => process.exit(0));
```

### `main`

```
{
  rss: 273694720,
  heapTotal: 28590080,
  heapUsed: 27558272,
  external: 10461,
  arrayBuffers: 10475
}
```

### Post-change

```
{
  rss: 102776832,
  heapTotal: 7094272,
  heapUsed: 6154856,
  external: 10461,
  arrayBuffers: 10475
}
```

## Runtime benchmarks

I tested runtime impact with two scripts, one of which is the benchmark from the original PR that re-introduced the cache. I will say that I'm not totally confident in the DataDog/CDK reproduction from the original issue. Even without any caching at all, I don't see performance as bad as the original reports.

```json
{
  "dependencies": {
    "aws-cdk-lib": "^2.214.0",
    "constructs": "^10.4.2",
    "date-fns": "^4.1.0",
    "dd-trace": "^5.65.0"
  }
}
```

```javascript
for (let i = 0; i < 1000; i++) {
  require('date-fns');
}
```

```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}`)
}
```

### `main`

```
➜ hyperfine --warmup 5 -N "../node/node date-fns-benchmark.js"
Benchmark 1: ../node/node date-fns-benchmark.js
  Time (mean ± σ):      84.0 ms ±   0.6 ms    [User: 96.9 ms, System: 15.6 ms]
  Range (min … max):    82.9 ms …  86.4 ms    35 runs
```

```
➜ hyperfine --warmup 5 -N "../node/node dd-cdk-benchmark.js"
Benchmark 1: ../node/node dd-cdk-benchmark.js
  Time (mean ± σ):     154.3 ms ±   0.8 ms    [User: 162.5 ms, System: 21.5 ms]
  Range (min … max):   153.0 ms … 155.8 ms    19 runs
```

### Post-change

```
➜ hyperfine --warmup 5 -N "../node/node date-fns-benchmark.js"
Benchmark 1: ../node/node date-fns-benchmark.js
  Time (mean ± σ):      50.5 ms ±   0.5 ms    [User: 48.4 ms, System: 11.4 ms]
  Range (min … max):    49.4 ms …  52.4 ms    59 runs
```

```
➜ hyperfine --warmup 5 -N "../node/node dd-cdk-benchmark.js"
Benchmark 1: ../node/node dd-cdk-benchmark.js
  Time (mean ± σ):     155.9 ms ±   1.0 ms    [User: 160.6 ms, System: 24.6 ms]
  Range (min … max):   154.2 ms … 157.7 ms    19 runs
```

Changed files:
filename: lib/internal/modules/package_json_reader.js
patch: @@ -6,7 +6,9 @@ const {
   ObjectDefineProperty,
   RegExpPrototypeExec,
   SafeMap,
+  StringPrototypeEndsWith,
   StringPrototypeIndexOf,
+  StringPrototypeLastIndexOf,
   StringPrototypeSlice,
 } = primordials;
 const {
@@ -26,6 +28,7 @@ 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');
 
@@ -127,26 +130,71 @@ 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}
+ */
+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);
+
+    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;
+}
+
 /**
  * Get the nearest parent package.json file from a given path.
  * Return the package.json data and the path to the package.json file, or undefined.
  * @param {string} checkPath The path to start searching from.
  * @returns {undefined | DeserializedPackageConfig}
  */
 function getNearestParentPackageJSON(checkPath) {
-  if (nearestParentPackageJSONCache.has(checkPath)) {
-    return nearestParentPackageJSONCache.get(checkPath);
+  const nearestParentPackageJSON = findParentPackageJSON(checkPath);
+
+  if (nearestParentPackageJSON === undefined) {
+    return undefined;
+  }
+
+  if (nearestParentPackageJSONCache.has(nearestParentPackageJSON)) {
+    return nearestParentPackageJSONCache.get(nearestParentPackageJSON);
   }
 
-  const result = modulesBinding.getNearestParentPackageJSON(checkPath);
+  const result = modulesBinding.readPackageJSON(nearestParentPackageJSON);
 
   if (result === undefined) {
     nearestParentPackageJSONCache.set(checkPath, undefined);
     return undefined;
   }
 
   const packageConfig = deserializePackageJSON(checkPath, result);
-  nearestParentPackageJSONCache.set(checkPath, packageConfig);
+  nearestParentPackageJSONCache.set(nearestParentPackageJSON, packageConfig);
 
   return packageConfig;
 }

filename: src/node_modules.cc
patch: @@ -320,40 +320,6 @@ const BindingData::PackageConfig* BindingData::TraverseParent(
   return nullptr;
 }
 
-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);
-
-  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<Value>& args) {
   CHECK_GE(args.Length(), 1);
@@ -680,10 +646,6 @@ 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>);
@@ -740,7 +702,6 @@ 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,8 +55,6 @@ 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>

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