Refactor export map cache to not store transient symbols #44816
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Uncommitted Bug
andrewbranch
commented
Jun 29, 2021
| @@ -0,0 +1,95 @@ | |||
| /*@internal*/ | |||
There was a problem hiding this comment.
Moved verbatim from utilities.ts
Comment on lines
-319
to
-347
| const result: MultiMap<string, SymbolExportInfo> = createMultiMap(); | ||
| const compilerOptions = program.getCompilerOptions(); | ||
| const target = getEmitScriptTarget(compilerOptions); | ||
| forEachExternalModuleToImportFrom(program, host, /*useAutoImportProvider*/ true, (moduleSymbol, _moduleFile, program, isFromPackageJson) => { | ||
| const checker = program.getTypeChecker(); | ||
| const defaultInfo = getDefaultLikeExportInfo(moduleSymbol, checker, compilerOptions); | ||
| if (defaultInfo && !checker.isUndefinedSymbol(defaultInfo.symbol)) { | ||
| const name = getNameForExportedSymbol(getLocalSymbolForExportDefault(defaultInfo.symbol) || defaultInfo.symbol, target); | ||
| result.add(key(name, defaultInfo.symbol, moduleSymbol, checker), { | ||
| symbol: defaultInfo.symbol, | ||
| moduleSymbol, | ||
| exportKind: defaultInfo.exportKind, | ||
| exportedSymbolIsTypeOnly: isTypeOnlySymbol(defaultInfo.symbol, checker), | ||
| isFromPackageJson, | ||
| }); | ||
| } | ||
| const seenExports = new Map<Symbol, true>(); | ||
| for (const exported of checker.getExportsAndPropertiesOfModule(moduleSymbol)) { | ||
| if (exported !== defaultInfo?.symbol && addToSeen(seenExports, exported)) { | ||
| result.add(key(getNameForExportedSymbol(exported, target), exported, moduleSymbol, checker), { | ||
| symbol: exported, | ||
| moduleSymbol, | ||
| exportKind: ExportKind.Named, | ||
| exportedSymbolIsTypeOnly: isTypeOnlySymbol(exported, checker), | ||
| isFromPackageJson, | ||
| }); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
This core part of getSymbolToExportInfoMap moved to exportInfoMap.ts mostly unchanged.
Comment on lines
-370
to
-399
| function wrapMultiMap(map: MultiMap<string, SymbolExportInfo>, isFromPreviousProjectVersion: boolean): SymbolToExportInfoMap { | ||
| const wrapped: SymbolToExportInfoMap = { | ||
| get: (importedName, symbol, moduleSymbol, checker) => { | ||
| const info = map.get(key(importedName, symbol, moduleSymbol, checker)); | ||
| return isFromPreviousProjectVersion ? info?.map(info => replaceTransientSymbols(info, checker)) : info; | ||
| }, | ||
| forEach: (getChecker, action) => { | ||
| map.forEach((info, key) => { | ||
| const { symbolName, ambientModuleName } = parseKey(key); | ||
| action( | ||
| isFromPreviousProjectVersion ? info.map(i => replaceTransientSymbols(i, getChecker(i.isFromPackageJson))) : info, | ||
| symbolName, | ||
| !!ambientModuleName); | ||
| }); | ||
| }, | ||
| }; | ||
| if (Debug.isDebugging) { | ||
| Object.defineProperty(wrapped, "__cache", { get: () => map }); | ||
| } | ||
| return wrapped; | ||
|
|
||
| function replaceTransientSymbols(info: SymbolExportInfo, checker: TypeChecker) { | ||
| if (info.symbol.flags & SymbolFlags.Transient) { | ||
| info.symbol = checker.getMergedSymbol(info.symbol.declarations?.[0]?.symbol || info.symbol); | ||
| } | ||
| if (info.moduleSymbol.flags & SymbolFlags.Transient) { | ||
| info.moduleSymbol = checker.getMergedSymbol(info.moduleSymbol.declarations?.[0]?.symbol || info.moduleSymbol); | ||
| } | ||
| return info; | ||
| } |
There was a problem hiding this comment.
This is the main part of the code that was rewritten, but there are conceptual analogs present in exportInfoMap.ts.
| @@ -773,21 +667,6 @@ namespace ts.codefix { | |||
| return originalSymbolToExportInfos; | |||
| } | |||
|
|
|||
| function getDefaultLikeExportInfo(moduleSymbol: Symbol, checker: TypeChecker, compilerOptions: CompilerOptions) { | |||
There was a problem hiding this comment.
The rest of the removals in this file just moved to exportInfoMap.ts unchanged.
| } | ||
| return cache; | ||
|
|
||
| function rehydrateCachedInfo(info: CachedSymbolExportInfo): SymbolExportInfo { |
There was a problem hiding this comment.
This function, wrapped by the get and forEach methods of the returned cache, turn CachedSymbolExportInfo, which might be missing symbol or moduleSymbol if they were transient, by pulling those from the short-term symbols cache, or falling back to fetching fresh versions of them from the current program.
andrewbranch
commented
Jun 29, 2021
Comment on lines
+84
to
+99
| const storedSymbol = symbol.flags & SymbolFlags.Transient ? undefined : symbol; | ||
| const storedModuleSymbol = moduleSymbol.flags & SymbolFlags.Transient ? undefined : moduleSymbol; | ||
| if (!storedSymbol || !storedModuleSymbol) symbols.set(id, [symbol, moduleSymbol]); | ||
|
|
||
| exportInfo.add(key(importedName, symbol, moduleName, checker), { | ||
| id, | ||
| symbolName: importedName, | ||
| moduleName, | ||
| moduleFile, | ||
| moduleFileName: moduleFile?.fileName, | ||
| exportKind, | ||
| isTypeOnly: !(skipAlias(symbol, checker).flags & SymbolFlags.Value), | ||
| isFromPackageJson, | ||
| symbol: storedSymbol, | ||
| moduleSymbol: storedModuleSymbol, | ||
| }); |
There was a problem hiding this comment.
Symbols stored in symbols if transient, exportInfo if not. I tried never storing any symbols, but the performance hit was too great.
amcasey
approved these changes
Jul 1, 2021
| const { exportMapCache, checker } = setup(); | ||
| assert.ok(exportMapCache.get(bTs.path as Path, checker)); | ||
| const { exportMapCache } = setup(); | ||
| assert.ok(exportMapCache.isUsableByFile(bTs.path as Path)); |
There was a problem hiding this comment.
Probably out of scope, but I would find isTrue and isFalse more readable.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-up to #44176 in a way, but fixes a memory leak that has been present for much longer. Previously, the cache stored exported symbols as-is, including transient ones, which can store a Type, which stores the TypeChecker it came from. We were never retaining more than one stale checker at a time, and were clearing it frequently as the program changed shape and the user moved between different files. Still, it was a mistake, and also caused part of #44176 to not work in certain edge cases (see the test added to
completionsIncomplete.ts).This PR also renames and relocates a lot of things to make more sense. I’ll point out where the significant changes are in review comments.