Skip to content

Add cache to auto-import package.json filter #52422

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 6, 2023
Merged

Add cache to auto-import package.json filter #52422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/compiler/moduleSpecifiers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,9 @@ function computeModuleSpecifiers(
let redirectPathsSpecifiers: string[] | undefined;
let relativeSpecifiers: string[] | undefined;
for (const modulePath of modulePaths) {
const specifier = tryGetModuleNameAsNodeModule(modulePath, info, importingSourceFile, host, compilerOptions, userPreferences, /*packageNameOnly*/ undefined, options.overrideImportMode);
const specifier = modulePath.isInNodeModules
? tryGetModuleNameAsNodeModule(modulePath, info, importingSourceFile, host, compilerOptions, userPreferences, /*packageNameOnly*/ undefined, options.overrideImportMode)
: undefined;
nodeModulesSpecifiers = append(nodeModulesSpecifiers, specifier);
if (specifier && modulePath.isRedirect) {
// If we got a specifier for a redirect, it was a bare package specifier (e.g. "@foo/bar",
Expand Down
50 changes: 42 additions & 8 deletions src/services/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3663,7 +3663,13 @@ export function createPackageJsonImportFilter(fromFile: SourceFile, preferences:
).filter(p => p.parseable);

let usesNodeCoreModules: boolean | undefined;
return { allowsImportingAmbientModule, allowsImportingSourceFile, allowsImportingSpecifier };
let ambientModuleCache: Map<Symbol, boolean> | undefined;
let sourceFileCache: Map<SourceFile, boolean> | undefined;
Comment on lines +3666 to +3667
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we typically use the symbols or nodes themselves as cache keys; all caches I've dealt with or added use symbol/node IDs instead; not sure if that's intentional or not, though.

Copy link
Member

Choose a reason for hiding this comment

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

i think this should be ok now that we are using native maps and life of this cache is well maintained..

Copy link
Member

Choose a reason for hiding this comment

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

That does make me wonder if we'd gain anything by switching all of the other caches to work directly...

Copy link
Member Author

@andrewbranch andrewbranch Jan 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I had a vague memory of someone saying using the object instead of ids is preferred now. But I don’t know if there’s a real difference.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can @rbuckton or @DanielRosenwasser advise on this?

Copy link
Member

Choose a reason for hiding this comment

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

Node IDs are set to 0 unconditionally, and set lazily when we need to use a Node in a Map or WeakMap. I tried to remove some usages out of the compiler and didn't find a ton of wins.

#46595

The biggest risk here is that you keep a PackageJsonFilter around across language service operations - leading to either a memory leak, or corrupt state. A WeakMap works, but I'd rather we just be a little careful about how we use these.

So I agree with Sheetal - the cache doesn't escape so we should be okay with these - just make sure the PackageJsonFilter object doesn't either.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, let me rephrase that - using objects/sparse arrays over Maps actually is a win, but not if you rely on iteration order.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this object only lasts a single request.

return {
allowsImportingAmbientModule,
allowsImportingSourceFile,
allowsImportingSpecifier,
};

function moduleSpecifierIsCoveredByPackageJson(specifier: string) {
const packageName = getNodeModuleRootSpecifier(specifier);
Expand All @@ -3680,32 +3686,60 @@ export function createPackageJsonImportFilter(fromFile: SourceFile, preferences:
return true;
}

const declaringSourceFile = moduleSymbol.valueDeclaration.getSourceFile();
const declaringNodeModuleName = getNodeModulesPackageNameFromFileName(declaringSourceFile.fileName, moduleSpecifierResolutionHost);
if (typeof declaringNodeModuleName === "undefined") {
return true;
if (!ambientModuleCache) {
ambientModuleCache = new Map();
}
else {
const cached = ambientModuleCache.get(moduleSymbol);
if (cached !== undefined) {
return cached;
}
}

const declaredModuleSpecifier = stripQuotes(moduleSymbol.getName());
if (isAllowedCoreNodeModulesImport(declaredModuleSpecifier)) {
ambientModuleCache.set(moduleSymbol, true);
return true;
}

return moduleSpecifierIsCoveredByPackageJson(declaringNodeModuleName)
|| moduleSpecifierIsCoveredByPackageJson(declaredModuleSpecifier);
const declaringSourceFile = moduleSymbol.valueDeclaration.getSourceFile();
const declaringNodeModuleName = getNodeModulesPackageNameFromFileName(declaringSourceFile.fileName, moduleSpecifierResolutionHost);
if (typeof declaringNodeModuleName === "undefined") {
ambientModuleCache.set(moduleSymbol, true);
return true;
}

const result =
moduleSpecifierIsCoveredByPackageJson(declaringNodeModuleName) ||
moduleSpecifierIsCoveredByPackageJson(declaredModuleSpecifier);
ambientModuleCache.set(moduleSymbol, result);
return result;
}

function allowsImportingSourceFile(sourceFile: SourceFile, moduleSpecifierResolutionHost: ModuleSpecifierResolutionHost): boolean {
if (!packageJsons.length) {
return true;
}

if (!sourceFileCache) {
sourceFileCache = new Map();
}
else {
const cached = sourceFileCache.get(sourceFile);
if (cached !== undefined) {
return cached;
}
}

const moduleSpecifier = getNodeModulesPackageNameFromFileName(sourceFile.fileName, moduleSpecifierResolutionHost);
if (!moduleSpecifier) {
sourceFileCache.set(sourceFile, true);
return true;
}

return moduleSpecifierIsCoveredByPackageJson(moduleSpecifier);
const result = moduleSpecifierIsCoveredByPackageJson(moduleSpecifier);
sourceFileCache.set(sourceFile, result);
return result;
}

function allowsImportingSpecifier(moduleSpecifier: string) {
Expand Down