fix(language-service): create StaticReflector once only (#32543)
The creation of StaticReflector in createMetadataResolver() is a very expensive operation because it involves numerous module resolutions. To make matter worse, since the API of the Reflector does not provide the ability to invalidate its internal caches, it has to be destroyed and recreated on *every* program change. This has a HUGE impact on performance. This PR fixes this problem by carefully invalidating all StaticSymbols in a file that has changed, thereby reducing the overhead of recomputation on program change. PR Close #32543
This commit is contained in:
parent
900d0055e0
commit
adb562bca6
@ -86,6 +86,22 @@ export class StaticReflector implements CompileReflector {
|
|||||||
return this.symbolResolver.getResourcePath(staticSymbol);
|
return this.symbolResolver.getResourcePath(staticSymbol);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Invalidate the specified `symbols` on program change.
|
||||||
|
* @param symbols
|
||||||
|
*/
|
||||||
|
invalidateSymbols(symbols: StaticSymbol[]) {
|
||||||
|
for (const symbol of symbols) {
|
||||||
|
this.annotationCache.delete(symbol);
|
||||||
|
this.shallowAnnotationCache.delete(symbol);
|
||||||
|
this.propertyCache.delete(symbol);
|
||||||
|
this.parameterCache.delete(symbol);
|
||||||
|
this.methodCache.delete(symbol);
|
||||||
|
this.staticCache.delete(symbol);
|
||||||
|
this.conversionMap.delete(symbol);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
resolveExternalReference(ref: o.ExternalReference, containingFile?: string): StaticSymbol {
|
resolveExternalReference(ref: o.ExternalReference, containingFile?: string): StaticSymbol {
|
||||||
let key: string|undefined = undefined;
|
let key: string|undefined = undefined;
|
||||||
if (!containingFile) {
|
if (!containingFile) {
|
||||||
|
@ -63,7 +63,6 @@ export class StaticSymbolResolver {
|
|||||||
private metadataCache = new Map<string, {[key: string]: any}>();
|
private metadataCache = new Map<string, {[key: string]: any}>();
|
||||||
// Note: this will only contain StaticSymbols without members!
|
// Note: this will only contain StaticSymbols without members!
|
||||||
private resolvedSymbols = new Map<StaticSymbol, ResolvedStaticSymbol>();
|
private resolvedSymbols = new Map<StaticSymbol, ResolvedStaticSymbol>();
|
||||||
private resolvedFilePaths = new Set<string>();
|
|
||||||
// Note: this will only contain StaticSymbols without members!
|
// Note: this will only contain StaticSymbols without members!
|
||||||
private importAs = new Map<StaticSymbol, StaticSymbol>();
|
private importAs = new Map<StaticSymbol, StaticSymbol>();
|
||||||
private symbolResourcePaths = new Map<StaticSymbol, string>();
|
private symbolResourcePaths = new Map<StaticSymbol, string>();
|
||||||
@ -176,22 +175,24 @@ export class StaticSymbolResolver {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Invalidate all information derived from the given file.
|
* Invalidate all information derived from the given file and return the
|
||||||
|
* static symbols contained in the file.
|
||||||
*
|
*
|
||||||
* @param fileName the file to invalidate
|
* @param fileName the file to invalidate
|
||||||
*/
|
*/
|
||||||
invalidateFile(fileName: string) {
|
invalidateFile(fileName: string): StaticSymbol[] {
|
||||||
this.metadataCache.delete(fileName);
|
this.metadataCache.delete(fileName);
|
||||||
this.resolvedFilePaths.delete(fileName);
|
|
||||||
const symbols = this.symbolFromFile.get(fileName);
|
const symbols = this.symbolFromFile.get(fileName);
|
||||||
if (symbols) {
|
if (!symbols) {
|
||||||
this.symbolFromFile.delete(fileName);
|
return [];
|
||||||
for (const symbol of symbols) {
|
|
||||||
this.resolvedSymbols.delete(symbol);
|
|
||||||
this.importAs.delete(symbol);
|
|
||||||
this.symbolResourcePaths.delete(symbol);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
this.symbolFromFile.delete(fileName);
|
||||||
|
for (const symbol of symbols) {
|
||||||
|
this.resolvedSymbols.delete(symbol);
|
||||||
|
this.importAs.delete(symbol);
|
||||||
|
this.symbolResourcePaths.delete(symbol);
|
||||||
|
}
|
||||||
|
return symbols;
|
||||||
}
|
}
|
||||||
|
|
||||||
/** @internal */
|
/** @internal */
|
||||||
@ -277,10 +278,9 @@ export class StaticSymbolResolver {
|
|||||||
}
|
}
|
||||||
|
|
||||||
private _createSymbolsOf(filePath: string) {
|
private _createSymbolsOf(filePath: string) {
|
||||||
if (this.resolvedFilePaths.has(filePath)) {
|
if (this.symbolFromFile.has(filePath)) {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
this.resolvedFilePaths.add(filePath);
|
|
||||||
const resolvedSymbols: ResolvedStaticSymbol[] = [];
|
const resolvedSymbols: ResolvedStaticSymbol[] = [];
|
||||||
const metadata = this.getModuleMetadata(filePath);
|
const metadata = this.getModuleMetadata(filePath);
|
||||||
if (metadata['importAs']) {
|
if (metadata['importAs']) {
|
||||||
|
@ -280,7 +280,7 @@ export class UndecoratedClassesTransform {
|
|||||||
return [{
|
return [{
|
||||||
node,
|
node,
|
||||||
message: `Class cannot be migrated as the inherited metadata from ` +
|
message: `Class cannot be migrated as the inherited metadata from ` +
|
||||||
`${identifier.getText()} cannot be converted into a decorator. Please manually
|
`${identifier.getText()} cannot be converted into a decorator. Please manually
|
||||||
decorate the class.`,
|
decorate the class.`,
|
||||||
}];
|
}];
|
||||||
}
|
}
|
||||||
@ -431,7 +431,7 @@ export class UndecoratedClassesTransform {
|
|||||||
// future calls to "StaticReflector#annotations" are based on metadata files.
|
// future calls to "StaticReflector#annotations" are based on metadata files.
|
||||||
this.symbolResolver['_resolveSymbolFromSummary'] = () => null;
|
this.symbolResolver['_resolveSymbolFromSummary'] = () => null;
|
||||||
this.symbolResolver['resolvedSymbols'].clear();
|
this.symbolResolver['resolvedSymbols'].clear();
|
||||||
this.symbolResolver['resolvedFilePaths'].clear();
|
this.symbolResolver['symbolFromFile'].clear();
|
||||||
this.compiler.reflector['annotationCache'].clear();
|
this.compiler.reflector['annotationCache'].clear();
|
||||||
|
|
||||||
// Original summary resolver used by the AOT compiler.
|
// Original summary resolver used by the AOT compiler.
|
||||||
|
@ -166,9 +166,7 @@ export class TypeScriptServiceHost implements LanguageServiceHost {
|
|||||||
this.templateReferences = [];
|
this.templateReferences = [];
|
||||||
this.fileToComponent.clear();
|
this.fileToComponent.clear();
|
||||||
this.collectedErrors.clear();
|
this.collectedErrors.clear();
|
||||||
// TODO: This is only temporary. When https://github.com/angular/angular/pull/32543
|
this.resolver.clearCache();
|
||||||
// is merged this is no longer necessary.
|
|
||||||
this._resolver = undefined; // Invalidate the resolver
|
|
||||||
|
|
||||||
const analyzeHost = {isSourceFile(filePath: string) { return true; }};
|
const analyzeHost = {isSourceFile(filePath: string) { return true; }};
|
||||||
const programFiles = this.program.getSourceFiles().map(sf => sf.fileName);
|
const programFiles = this.program.getSourceFiles().map(sf => sf.fileName);
|
||||||
@ -289,37 +287,37 @@ export class TypeScriptServiceHost implements LanguageServiceHost {
|
|||||||
* Returns true if modules are up-to-date, false otherwise.
|
* Returns true if modules are up-to-date, false otherwise.
|
||||||
* This should only be called by getAnalyzedModules().
|
* This should only be called by getAnalyzedModules().
|
||||||
*/
|
*/
|
||||||
private upToDate() {
|
private upToDate(): boolean {
|
||||||
const program = this.program;
|
const {lastProgram, program} = this;
|
||||||
if (this.lastProgram === program) {
|
if (lastProgram === program) {
|
||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
|
this.lastProgram = program;
|
||||||
|
|
||||||
// Invalidate file that have changed in the static symbol resolver
|
// Invalidate file that have changed in the static symbol resolver
|
||||||
const seen = new Set<string>();
|
const seen = new Set<string>();
|
||||||
let hasChanges = false;
|
|
||||||
for (const sourceFile of program.getSourceFiles()) {
|
for (const sourceFile of program.getSourceFiles()) {
|
||||||
const fileName = sourceFile.fileName;
|
const fileName = sourceFile.fileName;
|
||||||
seen.add(fileName);
|
seen.add(fileName);
|
||||||
const version = this.tsLsHost.getScriptVersion(fileName);
|
const version = this.tsLsHost.getScriptVersion(fileName);
|
||||||
const lastVersion = this.fileVersions.get(fileName);
|
const lastVersion = this.fileVersions.get(fileName);
|
||||||
if (version !== lastVersion) {
|
this.fileVersions.set(fileName, version);
|
||||||
hasChanges = true;
|
// Should not invalidate file on the first encounter or if file hasn't changed
|
||||||
this.fileVersions.set(fileName, version);
|
if (lastVersion !== undefined && version !== lastVersion) {
|
||||||
this.staticSymbolResolver.invalidateFile(fileName);
|
const symbols = this.staticSymbolResolver.invalidateFile(fileName);
|
||||||
|
this.reflector.invalidateSymbols(symbols);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Remove file versions that are no longer in the file and invalidate them.
|
// Remove file versions that are no longer in the program and invalidate them.
|
||||||
const missing = Array.from(this.fileVersions.keys()).filter(f => !seen.has(f));
|
const missing = Array.from(this.fileVersions.keys()).filter(f => !seen.has(f));
|
||||||
missing.forEach(f => {
|
missing.forEach(f => {
|
||||||
this.fileVersions.delete(f);
|
this.fileVersions.delete(f);
|
||||||
this.staticSymbolResolver.invalidateFile(f);
|
const symbols = this.staticSymbolResolver.invalidateFile(f);
|
||||||
|
this.reflector.invalidateSymbols(symbols);
|
||||||
});
|
});
|
||||||
|
|
||||||
this.lastProgram = program;
|
return false;
|
||||||
|
|
||||||
return missing.length === 0 && !hasChanges;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
@ -51,22 +51,24 @@ describe('TypeScriptServiceHost', () => {
|
|||||||
expect(analyzedModules.files.length).toBe(0);
|
expect(analyzedModules.files.length).toBe(0);
|
||||||
expect(analyzedModules.ngModules.length).toBe(0);
|
expect(analyzedModules.ngModules.length).toBe(0);
|
||||||
expect(analyzedModules.ngModuleByPipeOrDirective.size).toBe(0);
|
expect(analyzedModules.ngModuleByPipeOrDirective.size).toBe(0);
|
||||||
expect(analyzedModules.symbolsMissingModule).toBeUndefined();
|
expect(analyzedModules.symbolsMissingModule).toEqual([]);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('should clear the caches if program changes', () => {
|
it('should clear the caches if new script is added', () => {
|
||||||
// First create a TypescriptHost with empty script names
|
// First create a TypescriptHost with empty script names
|
||||||
const tsLSHost = new MockTypescriptHost([]);
|
const tsLSHost = new MockTypescriptHost([]);
|
||||||
const tsLS = ts.createLanguageService(tsLSHost);
|
const tsLS = ts.createLanguageService(tsLSHost);
|
||||||
const ngLSHost = new TypeScriptServiceHost(tsLSHost, tsLS);
|
const ngLSHost = new TypeScriptServiceHost(tsLSHost, tsLS);
|
||||||
expect(ngLSHost.getAnalyzedModules().ngModules).toEqual([]);
|
const oldModules = ngLSHost.getAnalyzedModules();
|
||||||
|
expect(oldModules.ngModules).toEqual([]);
|
||||||
// Now add a script, this would change the program
|
// Now add a script, this would change the program
|
||||||
const fileName = '/app/main.ts';
|
const fileName = '/app/main.ts';
|
||||||
const content = tsLSHost.readFile(fileName) !;
|
const content = tsLSHost.readFile(fileName) !;
|
||||||
tsLSHost.addScript(fileName, content);
|
tsLSHost.addScript(fileName, content);
|
||||||
// If the caches are not cleared, we would get back an empty array.
|
// If the caches are not cleared, we would get back an empty array.
|
||||||
// But if the caches are cleared then the analyzed modules will be non-empty.
|
// But if the caches are cleared then the analyzed modules will be non-empty.
|
||||||
expect(ngLSHost.getAnalyzedModules().ngModules.length).not.toEqual(0);
|
const newModules = ngLSHost.getAnalyzedModules();
|
||||||
|
expect(newModules.ngModules.length).toBeGreaterThan(0);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('should throw if getSourceFile is called on non-TS file', () => {
|
it('should throw if getSourceFile is called on non-TS file', () => {
|
||||||
|
Loading…
x
Reference in New Issue
Block a user