Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: cache negative results in GetPackageJSON #56834

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dmichon-msft
Copy link

Fixes #56821

Ensure that GetPackageJSON caches which package.json files do not exist, not just the contents of the ones that do. In packages with at least one subfolder, it is normal for most folders in a package not to contain a package.json file, so this will significantly reduce the number of file system reads performed during GetNearestParentPackageJSON.

This change ensures that GetPackageJSON caches
which folders do *not* contain a package.json, to
prevent excessive file system probing.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jan 30, 2025
@ljharb
Copy link
Member

ljharb commented Jan 30, 2025

What happens if a package.json file is created after this is cached?

@dmichon-msft
Copy link
Author

What happens if a package.json file is created after this is cached?

The same thing that happens if you change the content of an existing package.json file after it gets read (and cached). The state of the world as of the first read of the folder is preserved.

@ljharb
Copy link
Member

ljharb commented Jan 30, 2025

How does one clear the cache, then, after making a change?

@dmichon-msft
Copy link
Author

How does one clear the cache, then, after making a change?

Usually by shutting down and restarting the process, since this cache is used during module loading.

@dmichon-msft
Copy link
Author

dmichon-msft commented Jan 31, 2025

Missing package.json files were cached in Node <= 20:

cache.set(jsonPath, result);

As near as I can tell, this behavior change was simply an oversight when migrating readPackageScope from the JS layer to the native layer.

Node <= 20 also does not provide any means for cache entries regarding package.json to be cleared.

src/node_modules.cc Outdated Show resolved Hide resolved
@@ -100,10 +100,18 @@ const BindingData::PackageConfig* BindingData::GetPackageJSON(
return &cache_entry->second;
}

if (!binding_data->missing_package_configs_.contains(path.data())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!binding_data->missing_package_configs_.contains(path.data())) {
if (binding_data->missing_package_configs_.contains(std::string(path))) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GetPackageJson internal binding should cache missing file results
4 participants