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.

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

Jump to bottom

Fix performance regression when reusing old state #28028

Merged
merged 2 commits into from Oct 25, 2018
Merged

Fix performance regression when reusing old state #28028

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea0c7eb
Fix performance regression when reusing old state
ajafff Oct 21, 2018
63a8cb6
use Map instead of Array
ajafff Oct 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 20 additions & 27 deletions src/compiler/program.ts
View file
Open in desktop
Expand Up @@ -594,7 +594,7 @@ namespace ts {
let diagnosticsProducingTypeChecker: TypeChecker;
let noDiagnosticsTypeChecker: TypeChecker;
let classifiableNames: UnderscoreEscapedMap<true>;
let modifiedFilePaths: Path[] | undefined;
const ambientModuleNameToUnmodifiedFileName = createMap<string>();

const cachedSemanticDiagnosticsForFile: DiagnosticCache<Diagnostic> = {};
const cachedDeclarationDiagnosticsForFile: DiagnosticCache<DiagnosticWithLocation> = {};
Expand Down Expand Up @@ -880,21 +880,14 @@ namespace ts {
return classifiableNames;
}

interface OldProgramState {
program: Program | undefined;
oldSourceFile: SourceFile | undefined;
/** The collection of paths modified *since* the old program. */
modifiedFilePaths: Path[] | undefined;
}

function resolveModuleNamesReusingOldState(moduleNames: string[], containingFile: string, file: SourceFile, oldProgramState: OldProgramState) {
function resolveModuleNamesReusingOldState(moduleNames: string[], containingFile: string, file: SourceFile) {
if (structuralIsReused === StructureIsReused.Not && !file.ambientModuleNames.length) {
// If the old program state does not permit reusing resolutions and `file` does not contain locally defined ambient modules,
// the best we can do is fallback to the default logic.
return resolveModuleNamesWorker(moduleNames, containingFile, /*reusedNames*/ undefined, getResolvedProjectReferenceToRedirect(file.originalFileName));
}

const oldSourceFile = oldProgramState.program && oldProgramState.program.getSourceFile(containingFile);
const oldSourceFile = oldProgram && oldProgram.getSourceFile(containingFile);
if (oldSourceFile !== file && file.resolvedModules) {
// `file` was created for the new program.
//
Expand Down Expand Up @@ -958,7 +951,7 @@ namespace ts {
}
}
else {
resolvesToAmbientModuleInNonModifiedFile = moduleNameResolvesToAmbientModuleInNonModifiedFile(moduleName, oldProgramState);
resolvesToAmbientModuleInNonModifiedFile = moduleNameResolvesToAmbientModuleInNonModifiedFile(moduleName);
}

if (resolvesToAmbientModuleInNonModifiedFile) {
Expand Down Expand Up @@ -1001,12 +994,9 @@ namespace ts {

// If we change our policy of rechecking failed lookups on each program create,
// we should adjust the value returned here.
function moduleNameResolvesToAmbientModuleInNonModifiedFile(moduleName: string, oldProgramState: OldProgramState): boolean {
if (!oldProgramState.program) {
return false;
}
const resolutionToFile = getResolvedModule(oldProgramState.oldSourceFile!, moduleName); // TODO: GH#18217
const resolvedFile = resolutionToFile && oldProgramState.program.getSourceFile(resolutionToFile.resolvedFileName);
function moduleNameResolvesToAmbientModuleInNonModifiedFile(moduleName: string): boolean {
const resolutionToFile = getResolvedModule(oldSourceFile!, moduleName);
const resolvedFile = resolutionToFile && oldProgram!.getSourceFile(resolutionToFile.resolvedFileName);
if (resolutionToFile && resolvedFile && !resolvedFile.externalModuleIndicator) {
// In the old program, we resolved to an ambient module that was in the same
// place as we expected to find an actual module file.
Expand All @@ -1016,16 +1006,14 @@ namespace ts {
}

// at least one of declarations should come from non-modified source file
const firstUnmodifiedFile = oldProgramState.program.getSourceFiles().find(
f => !contains(oldProgramState.modifiedFilePaths, f.path) && contains(f.ambientModuleNames, moduleName)
);
const unmodifiedFile = ambientModuleNameToUnmodifiedFileName.get(moduleName);

if (!firstUnmodifiedFile) {
if (!unmodifiedFile) {
return false;
}

if (isTraceEnabled(options, host)) {
trace(host, Diagnostics.Module_0_was_resolved_as_ambient_module_declared_in_1_since_this_file_was_not_modified, moduleName, firstUnmodifiedFile.fileName);
trace(host, Diagnostics.Module_0_was_resolved_as_ambient_module_declared_in_1_since_this_file_was_not_modified, moduleName, unmodifiedFile);
}
return true;
}
Expand Down Expand Up @@ -1213,14 +1201,20 @@ namespace ts {
return oldProgram.structureIsReused;
}

modifiedFilePaths = modifiedSourceFiles.map(f => f.newFile.path);
const modifiedFiles = modifiedSourceFiles.map(f => f.oldFile);
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 you can get away with creating duplicate array by using every function on modifiedSourceFiles to find if oldFile isn't in modified files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if I correctly understand what your comment means:
If you mean I should prefer !modifiedSourceFiles.some((f) => f.oldFile === oldFile) in the loop below, I don't think it's worth changing. That would need to allocate a closure for every lookup and basically does the mapping inplace just to avoid allocating a new (probably very small) array.

for (const oldFile of oldSourceFiles) {
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 needs to be a function that either gets the value that's cached or calculate this so that we avoid doing this if there are no modules in new files etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this loop is critical for performance as it's only executed once and it's very likely that this information is needed later anyway.
In addition modifiedSourceFiles is only available in the scope of this function. I would need to move it to an outer scope to access it later. I'd like to avoid that as it increases the likelihood to retain references to old sourcefiles that could otherwise be garbage collected

if (!contains(modifiedFiles, oldFile)) {
for (const moduleName of oldFile.ambientModuleNames) {
ambientModuleNameToUnmodifiedFileName.set(moduleName, oldFile.fileName);
}
}
}
// try to verify results of module resolution
for (const { oldFile: oldSourceFile, newFile: newSourceFile } of modifiedSourceFiles) {
const newSourceFilePath = getNormalizedAbsolutePath(newSourceFile.originalFileName, currentDirectory);
if (resolveModuleNamesWorker) {
const moduleNames = getModuleNames(newSourceFile);
const oldProgramState: OldProgramState = { program: oldProgram, oldSourceFile, modifiedFilePaths };
const resolutions = resolveModuleNamesReusingOldState(moduleNames, newSourceFilePath, newSourceFile, oldProgramState);
const resolutions = resolveModuleNamesReusingOldState(moduleNames, newSourceFilePath, newSourceFile);
// ensure that module resolution results are still correct
const resolutionsChanged = hasChangesInResolutions(moduleNames, resolutions, oldSourceFile.resolvedModules, moduleResolutionIsEqualTo);
if (resolutionsChanged) {
Expand Down Expand Up @@ -2422,8 +2416,7 @@ namespace ts {
if (file.imports.length || file.moduleAugmentations.length) {
// Because global augmentation doesn't have string literal name, we can check for global augmentation as such.
const moduleNames = getModuleNames(file);
const oldProgramState: OldProgramState = { program: oldProgram, oldSourceFile: oldProgram && oldProgram.getSourceFile(file.fileName), modifiedFilePaths };
const resolutions = resolveModuleNamesReusingOldState(moduleNames, getNormalizedAbsolutePath(file.originalFileName, currentDirectory), file, oldProgramState);
const resolutions = resolveModuleNamesReusingOldState(moduleNames, getNormalizedAbsolutePath(file.originalFileName, currentDirectory), file);
Debug.assert(resolutions.length === moduleNames.length);
for (let i = 0; i < moduleNames.length; i++) {
const resolution = resolutions[i];
Expand Down