Perf regression in Node 22/24 when loading JS files

[Tyoneb]

Version

v22.21.0

Platform

any

What steps will reproduce the bug?

There is a performance regression when loading JS files from a location with latency (e.g. network path).

Unfortunately I don't know how to provide a sample to reproduce it in other conditions than my own environment, BUT, I think I identified the root cause in the NodeJS code (see below).

How often does it reproduce? Is there a required condition?

The issue is not present in:

• v20.19.5
• v21.4.0

The issue is partially fixed in:

• v22.18.0
• v22.19.0

It is present in:

• v21.5.0
• v22.21.0
• v24.10.0

What is the expected behavior? Why is that the expected behavior?

The expected behavior is to load files as fast as in v20.x.

What do you see instead?

Files take up to 4.5 times slower to load (in my environment).

Additional information

I cloned the repo and tested each commit to reach the following conclusion:

1. The regression was introduced in v21.5.0 by src: move package_json_reader cache to c++ #50322.
2. A first fix in v22.19.0 (src: add cache to nearest parent package json #59086) fixed the issue partially but introduced an increase of memory consumption.
3. A second fix in v22.21.0 (src: reduce the nearest parent package JSON cache size #59888) resolved the memory issue but reintroduced the performance regression.

I went through the code of each PR and here is my understanding:

1. The initial change src: move package_json_reader cache to c++ #50322 moved the search of a package.json file from the JS side to the C++ side. I'm not a C++ dev but I believe a part of the previous algorithm was missed and now the cache only applies to the content of the package.json but not to the file system search. So when walking up the file tree, each possible location for the package.json is tested again and again. It becomes particularly painful when the package.json is located high up the file tree.That would very well explain why the performance degraded when the file system operations are costly, and was not seen in a purely local context.
2. The first fix reintroduced caching on the JS side (which probably defeats at least partially the purpose of the initial change), but it duplicated the content of the package.json for each starting path location, thus increasing the consumed memory significantly.
3. The second fix reintroduced the loop over the file system search on the JS side (which is kind of funny because it completely rolls back the initial change) while still not caching the results of the previous operations, thus bringing back the performance regression.

I tested the following change in the method findParentPackageJSON of lib\internal\modules\package_json_reader.js and it does resolve the performance issue:

const packageJSONLocationCache = new SafeMap();
// [...]
const maybePackageJSONPath = checkPath + path.sep + 'package.json';
if (packageJSONLocationCache.has(maybePackageJSONPath)) {
  if (packageJSONLocationCache.get(maybePackageJSONPath)) {
    return maybePackageJSONPath
  } else {
    continue
  }
}
const stat = internalFsBinding.internalModuleStat(checkPath + path.sep + 'package.json');

const packageJSONExists = stat === 0;
packageJSONLocationCache.set(maybePackageJSONPath, packageJSONExists)
if (packageJSONExists) {
  return maybePackageJSONPath;
}

However I'm not sure this should be merged. If the intent of the initial change was to bring all that logic to the C++ side for performance reasons, it might be preferable to rollback all the changes done on the JS side and to fix the caching on the C++ side.

Unfortunately, I'm a JS/TS dev and I failed miserably to fix the C++ code 😢

[michaelsmithxyz]

@Tyoneb Do you have a self-contained reproduction that demonstrates the regression? I'm the author of the third PR you linked there. I considered adding essentially the same cache you suggest at the time, but it didn't really move the needle on any of the specific tests I was using. My understanding of the performance regression in 1 (addressed by the first cache in 2) was that it was about C++ / JS FFI boundary-crossing, not filesystem operations.

[Tyoneb]

Hey, thanks for your quick feedback!

Do you have a self-contained reproduction that demonstrates the regression? I'm the author of the third PR you linked there. I considered adding essentially the same cache you suggest at the time, but it didn't really move the needle on any of the specific tests I was using.

I guess that it depends on the setup. If you load everything from your local disk, the file system operations generally don't cost much, but when you do a simple stat across the network (which is my use case), the impact can be quite significant.

I don't know how to force delayed FS operations to simulate a network latency, which is why I did not provide a reproduction sample. Maybe it could be simulated by having a large and deep folder structure with many different files, with a single package.json at its root, but I haven't tried.

My understanding of the performance regression in 1 (addressed by the first cache in 2) was that it was about C++ / JS FFI boundary-crossing, not filesystem operations.

I'm not so sure about that analysis. The main difference I see is that the initial change moved the whole filetree walk and checks for package.json to the C++ side, instead of looping and caching the results on the JS side. I would say that the boundary-crossing happened only once per file, where the current implementation loops on the JS side and crosses to the C++ side every time it checks for the existence of a package.json. So it seems to me that the change made sense and reduced the boundary-crossing. But the fact that the results of the FS operations were not cached (only the read+parse) introduced the regression compared to the initial implementation. However I haven't been able to fix the C++ caching to validate my hypothesis, which is why I opened this issue.

For the sake of adding a bit more details, here is an example with a simple folder structure:

A
├─── B
│    └─── C
│         └─── C1.js
│    └─── D
│         └─── D1.js
└─── package.json

When you load C1.js, the algorithm will perform a fs.stat (or the equivalent on C++ side):

• /A/B/C/package.json → nope
• /A/B/package.json → nope
• /A/package.json → found! Let's read and parse its content.

Then when you load D1.js, it will follow the same pattern:

• /A/B/D/package.json → nope
• /A/B/package.json → this check has already been performed, there is no need to call the FS again, let's read its results from the cache
• /A/package.json → this check has already been performed and the file was found and parsed, let's retrieve its content from the second cache

Even if the file system operations are not particularly slow, they're still more costly than a simple Map#get and so caching their results do make sense in my opinion (even if the cache implies a slight increase of memory consumption).

[michaelsmithxyz]

@Tyoneb Oh gotcha, your original message mentioned loading from a network path; I should have read it more carefully!

My understanding of the performance regression in 1 (addressed by the first cache in 2) was that it was about C++ / JS FFI boundary-crossing, not filesystem operations.

I'm not so sure about that analysis. The main difference I see is that the initial change moved the whole filetree walk and checks for package.json to the C++ side, instead of looping and caching the results on the JS side.

It also adopted simdjson for parsing package.json, which I understood to be the primary intent of that change, but I'm not totally sure! I was mostly relying on the stated reasoning in #59086

I agree with your analysis overall, though! To restate it slightly differently in case it's helpful for other readers:

1. The implementation prior to src: move package_json_reader cache to c++ #50322 cached the result of read. If package.json existed at that path, it would cache the contents. Importantly, if package.json didn't exist at that path, it would also implicitly cache the failed call to open at that path, which is why it performs fine if filesystem operations are slow. The C++-side cache introduced in that change doesn't cache a negative result like the JS-side cache did, which is the first regression.
2. My change in src: reduce the nearest parent package JSON cache size #59888 introduces a bunch of uncached stat calls

We probably could introduce that second cache like you suggest. That's what I tested originally, but wasn't a meaningful improvement in my testing (admittedly with a fast local disk). Alternatively, I wonder if what was reported in #58126 and addressed in #59086 as boundary crossing overhead is actually just about the original set of uncached filesystem operations. That would explain why I wasn't personally able to reproduce that regression on versions of Node that had it. If that's true, maybe it's better to unwind all of this and just fix the caching in C++. I'm in a similar boat to you where I'm a lot more comfortable in JS than I am in C++ though!

[michaelsmithxyz]

Here's a draft of a change that I think may handle this in C++ reasonably, if it turns out that the boundary-crossing overhead described in #59086 isn't an issue: #60425

[Tyoneb]

I've tested locally the fix from #60425 in the v22.x branch and I confirm that the performance issue is resolved! Thanks @michaelsmithxyz!

As for the discussion around the boundary-crossing, the measures in my environment show the following:

• v20.19.5 → 1.273s
• v21.4.0 → 657.488ms (I believe the perf improvement comes from the V8 upgrade)
• v21.5.0 → 3.951s (this version contains the change from #50322)
• v22.21.1-pre → 4.012s (latest v22 at the time I ran the test)
• v22.21.1-pre (with fix from #60425) → 904.5ms

The values don't really matter since I can't share a reproducible sample, but I'm posting them just to compare.

In the end, it seems that the change from #50322 does have a negative performance impact, at least in some cases. Since the author of the PR claimed that it boosted the perf in their benchmark regarding vite --version, I don't know what to conclude.

Tagging @anonrig in case you have an opinion on the matter!

[anonrig]

I'll take a detailed look but #59888 is a significant regression that defeats the purpose of my original PR - make package json finding a single C++ call.

Please tag me on pull requests. Happy to review.

I'll take a more in depth look soon.

[anonrig]

@Tyoneb can you share your code/with similar directory structure so I can reproduce it on my own.