Refactor export map cache to not store transient symbols#44816
Merged
andrewbranch merged 10 commits intomicrosoft:mainfrom Jul 6, 2021
Merged
Refactor export map cache to not store transient symbols#44816andrewbranch merged 10 commits intomicrosoft:mainfrom
andrewbranch merged 10 commits intomicrosoft:mainfrom
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
andrewbranch
commented
Jun 29, 2021
| @@ -0,0 +1,95 @@ | |||
| /*@internal*/ | |||
Member
Author
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, | ||
| }); | ||
| } | ||
| } | ||
| }); |
Member
Author
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; | ||
| } |
Member
Author
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.
| return originalSymbolToExportInfos; | ||
| } | ||
|
|
||
| function getDefaultLikeExportInfo(moduleSymbol: Symbol, checker: TypeChecker, compilerOptions: CompilerOptions) { |
Member
Author
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 { |
Member
Author
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, | ||
| }); |
Member
Author
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)); |
Member
There was a problem hiding this comment.
Probably out of scope, but I would find isTrue and isFalse more readable.
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.