-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Make export-module and reference maps invertible #44402
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.
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
Changes from all commits
848e5df
9e6c43b
c824406
c1a4bd7
de9deff
70e0b14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,7 +45,7 @@ namespace ts { | |
/** | ||
* Newly computed visible to outside referencedSet | ||
*/ | ||
currentAffectedFilesExportedModulesMap?: Readonly<BuilderState.ComputingExportedModulesMap> | undefined; | ||
currentAffectedFilesExportedModulesMap?: BuilderState.ReadonlyManyToManyPathMap | undefined; | ||
/** | ||
* True if the semantic diagnostics were copied from the old state | ||
*/ | ||
|
@@ -113,8 +113,10 @@ namespace ts { | |
currentAffectedFilesSignatures: ESMap<Path, string> | undefined; | ||
/** | ||
* Newly computed visible to outside referencedSet | ||
* We need to store the updates separately in case the in-progress build is cancelled | ||
* and we need to roll back. | ||
*/ | ||
currentAffectedFilesExportedModulesMap: BuilderState.ComputingExportedModulesMap | undefined; | ||
currentAffectedFilesExportedModulesMap: BuilderState.ManyToManyPathMap | undefined; | ||
/** | ||
* Already seen affected files | ||
*/ | ||
|
@@ -212,7 +214,7 @@ namespace ts { | |
const copyLibFileDiagnostics = copyDeclarationFileDiagnostics && !compilerOptions.skipDefaultLibCheck === !oldCompilerOptions!.skipDefaultLibCheck; | ||
state.fileInfos.forEach((info, sourceFilePath) => { | ||
let oldInfo: Readonly<BuilderState.FileInfo> | undefined; | ||
let newReferences: BuilderState.ReferencedSet | undefined; | ||
let newReferences: ReadonlySet<Path> | undefined; | ||
|
||
// if not using old state, every file is changed | ||
if (!useOldState || | ||
|
@@ -221,7 +223,7 @@ namespace ts { | |
// versions dont match | ||
oldInfo.version !== info.version || | ||
// Referenced files changed | ||
!hasSameKeys(newReferences = referencedMap && referencedMap.get(sourceFilePath), oldReferencedMap && oldReferencedMap.get(sourceFilePath)) || | ||
!hasSameKeys(newReferences = referencedMap && referencedMap.getValues(sourceFilePath), oldReferencedMap && oldReferencedMap.getValues(sourceFilePath)) || | ||
// Referenced file was deleted in the new program | ||
newReferences && forEachKey(newReferences, path => !state.fileInfos.has(path) && oldState!.fileInfos.has(path))) { | ||
// Register file as changed file and do not copy semantic diagnostics, since all changed files need to be re-evaluated | ||
|
@@ -311,7 +313,7 @@ namespace ts { | |
newState.affectedFilesIndex = state.affectedFilesIndex; | ||
newState.currentChangedFilePath = state.currentChangedFilePath; | ||
newState.currentAffectedFilesSignatures = state.currentAffectedFilesSignatures && new Map(state.currentAffectedFilesSignatures); | ||
newState.currentAffectedFilesExportedModulesMap = state.currentAffectedFilesExportedModulesMap && new Map(state.currentAffectedFilesExportedModulesMap); | ||
newState.currentAffectedFilesExportedModulesMap = state.currentAffectedFilesExportedModulesMap?.clone(); | ||
newState.seenAffectedFiles = state.seenAffectedFiles && new Set(state.seenAffectedFiles); | ||
newState.cleanedDiagnosticsOfLibFiles = state.cleanedDiagnosticsOfLibFiles; | ||
newState.semanticDiagnosticsFromOldState = state.semanticDiagnosticsFromOldState && new Set(state.semanticDiagnosticsFromOldState); | ||
|
@@ -384,7 +386,7 @@ namespace ts { | |
// Get next batch of affected files | ||
if (!state.currentAffectedFilesSignatures) state.currentAffectedFilesSignatures = new Map(); | ||
if (state.exportedModulesMap) { | ||
if (!state.currentAffectedFilesExportedModulesMap) state.currentAffectedFilesExportedModulesMap = new Map(); | ||
state.currentAffectedFilesExportedModulesMap ||= BuilderState.createManyToManyPathMap(); | ||
} | ||
state.affectedFiles = BuilderState.getFilesAffectedBy(state, program, nextKey.value, cancellationToken, computeHash, state.currentAffectedFilesSignatures, state.currentAffectedFilesExportedModulesMap); | ||
state.currentChangedFilePath = nextKey.value; | ||
|
@@ -465,7 +467,7 @@ namespace ts { | |
* Handle the dts may change, so they need to be added to pending emit if dts emit is enabled, | ||
* Also we need to make sure signature is updated for these files | ||
*/ | ||
function handleDtsMayChangeOf(state: BuilderProgramState, path: Path, cancellationToken: CancellationToken | undefined, computeHash: BuilderState.ComputeHash) { | ||
function handleDtsMayChangeOf(state: BuilderProgramState, path: Path, cancellationToken: CancellationToken | undefined, computeHash: BuilderState.ComputeHash): void { | ||
removeSemanticDiagnosticsOf(state, path); | ||
|
||
if (!state.changedFilesSet.has(path)) { | ||
|
@@ -544,36 +546,36 @@ namespace ts { | |
} | ||
|
||
Debug.assert(!!state.currentAffectedFilesExportedModulesMap); | ||
|
||
const seenFileAndExportsOfFile = new Set<string>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Separate PR: @sheetalkamat can we lift this out a couple levels so we don't revisit the same paths repeatedly for different "affected" files? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or maybe it belongs on the state? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To share it across more than one affected file, it would have to be on the state. I'm going to try updating it in parallel with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assuming I've done it correctly, it saves about 10% of total time. Nothing to complain about, but much less than this change. |
||
// Go through exported modules from cache first | ||
// If exported modules has path, all files referencing file exported from are affected | ||
forEachEntry(state.currentAffectedFilesExportedModulesMap, (exportedModules, exportedFromPath) => | ||
exportedModules && | ||
exportedModules.has(affectedFile.resolvedPath) && | ||
state.currentAffectedFilesExportedModulesMap.getKeys(affectedFile.resolvedPath)?.forEach(exportedFromPath => | ||
forEachFilesReferencingPath(state, exportedFromPath, seenFileAndExportsOfFile, fn) | ||
); | ||
|
||
// If exported from path is not from cache and exported modules has path, all files referencing file exported from are affected | ||
forEachEntry(state.exportedModulesMap, (exportedModules, exportedFromPath) => | ||
!state.currentAffectedFilesExportedModulesMap!.has(exportedFromPath) && // If we already iterated this through cache, ignore it | ||
exportedModules.has(affectedFile.resolvedPath) && | ||
state.exportedModulesMap.getKeys(affectedFile.resolvedPath)?.forEach(exportedFromPath => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I couldn't find a place where |
||
// If the cache had an updated value, skip | ||
!state.currentAffectedFilesExportedModulesMap!.hasKey(exportedFromPath) && | ||
!state.currentAffectedFilesExportedModulesMap!.deletedKeys()?.has(exportedFromPath) && | ||
forEachFilesReferencingPath(state, exportedFromPath, seenFileAndExportsOfFile, fn) | ||
); | ||
} | ||
|
||
/** | ||
* Iterate on files referencing referencedPath | ||
*/ | ||
function forEachFilesReferencingPath(state: BuilderProgramState, referencedPath: Path, seenFileAndExportsOfFile: Set<string>, fn: (state: BuilderProgramState, filePath: Path) => void) { | ||
forEachEntry(state.referencedMap!, (referencesInFile, filePath) => | ||
referencesInFile.has(referencedPath) && forEachFileAndExportsOfFile(state, filePath, seenFileAndExportsOfFile, fn) | ||
function forEachFilesReferencingPath(state: BuilderProgramState, referencedPath: Path, seenFileAndExportsOfFile: Set<string>, fn: (state: BuilderProgramState, filePath: Path) => void): void { | ||
amcasey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
state.referencedMap!.getKeys(referencedPath)?.forEach(filePath => | ||
forEachFileAndExportsOfFile(state, filePath, seenFileAndExportsOfFile, fn) | ||
); | ||
} | ||
|
||
/** | ||
* fn on file and iterate on anything that exports this file | ||
*/ | ||
function forEachFileAndExportsOfFile(state: BuilderProgramState, filePath: Path, seenFileAndExportsOfFile: Set<string>, fn: (state: BuilderProgramState, filePath: Path) => void) { | ||
function forEachFileAndExportsOfFile(state: BuilderProgramState, filePath: Path, seenFileAndExportsOfFile: Set<string>, fn: (state: BuilderProgramState, filePath: Path) => void): void { | ||
if (!tryAddToSet(seenFileAndExportsOfFile, filePath)) { | ||
return; | ||
} | ||
|
@@ -583,23 +585,20 @@ namespace ts { | |
Debug.assert(!!state.currentAffectedFilesExportedModulesMap); | ||
// Go through exported modules from cache first | ||
// If exported modules has path, all files referencing file exported from are affected | ||
forEachEntry(state.currentAffectedFilesExportedModulesMap, (exportedModules, exportedFromPath) => | ||
exportedModules && | ||
exportedModules.has(filePath) && | ||
state.currentAffectedFilesExportedModulesMap.getKeys(filePath)?.forEach(exportedFromPath => | ||
forEachFileAndExportsOfFile(state, exportedFromPath, seenFileAndExportsOfFile, fn) | ||
); | ||
|
||
// If exported from path is not from cache and exported modules has path, all files referencing file exported from are affected | ||
forEachEntry(state.exportedModulesMap!, (exportedModules, exportedFromPath) => | ||
!state.currentAffectedFilesExportedModulesMap!.has(exportedFromPath) && // If we already iterated this through cache, ignore it | ||
exportedModules.has(filePath) && | ||
state.exportedModulesMap!.getKeys(filePath)?.forEach(exportedFromPath => | ||
// If the cache had an updated value, skip | ||
!state.currentAffectedFilesExportedModulesMap!.hasKey(exportedFromPath) && | ||
!state.currentAffectedFilesExportedModulesMap!.deletedKeys()?.has(exportedFromPath) && | ||
forEachFileAndExportsOfFile(state, exportedFromPath, seenFileAndExportsOfFile, fn) | ||
); | ||
|
||
// Remove diagnostics of files that import this file (without going to exports of referencing files) | ||
|
||
forEachEntry(state.referencedMap!, (referencesInFile, referencingFilePath) => | ||
referencesInFile.has(filePath) && | ||
state.referencedMap!.getKeys(filePath)?.forEach(referencingFilePath => | ||
!seenFileAndExportsOfFile.has(referencingFilePath) && // Not already removed diagnostic file | ||
fn(state, referencingFilePath) // Dont add to seen since this is not yet done with the export removal | ||
); | ||
|
@@ -756,18 +755,26 @@ namespace ts { | |
if (state.referencedMap) { | ||
referencedMap = arrayFrom(state.referencedMap.keys()).sort(compareStringsCaseSensitive).map(key => [ | ||
toFileId(key), | ||
toFileIdListId(state.referencedMap!.get(key)!) | ||
toFileIdListId(state.referencedMap!.getValues(key)!) | ||
]); | ||
} | ||
|
||
let exportedModulesMap: ProgramBuildInfoReferencedMap | undefined; | ||
if (state.exportedModulesMap) { | ||
exportedModulesMap = mapDefined(arrayFrom(state.exportedModulesMap.keys()).sort(compareStringsCaseSensitive), key => { | ||
const newValue = state.currentAffectedFilesExportedModulesMap && state.currentAffectedFilesExportedModulesMap.get(key); | ||
if (state.currentAffectedFilesExportedModulesMap) { | ||
if (state.currentAffectedFilesExportedModulesMap.deletedKeys()?.has(key)) { | ||
return undefined; | ||
} | ||
|
||
const newValue = state.currentAffectedFilesExportedModulesMap.getValues(key); | ||
if (newValue) { | ||
return [toFileId(key), toFileIdListId(newValue)]; | ||
} | ||
} | ||
|
||
// Not in temporary cache, use existing value | ||
if (newValue === undefined) return [toFileId(key), toFileIdListId(state.exportedModulesMap!.get(key)!)]; | ||
// Value in cache and has updated value map, use that | ||
else if (newValue) return [toFileId(key), toFileIdListId(newValue)]; | ||
return [toFileId(key), toFileIdListId(state.exportedModulesMap!.getValues(key)!)]; | ||
}); | ||
} | ||
|
||
|
@@ -1251,8 +1258,8 @@ namespace ts { | |
const state: ReusableBuilderProgramState = { | ||
fileInfos, | ||
compilerOptions: program.options ? convertToOptionsWithAbsolutePaths(program.options, toAbsolutePath) : {}, | ||
referencedMap: toMapOfReferencedSet(program.referencedMap), | ||
exportedModulesMap: toMapOfReferencedSet(program.exportedModulesMap), | ||
referencedMap: toManyToManyPathMap(program.referencedMap), | ||
exportedModulesMap: toManyToManyPathMap(program.exportedModulesMap), | ||
semanticDiagnosticsPerFile: program.semanticDiagnosticsPerFile && arrayToMap(program.semanticDiagnosticsPerFile, value => toFilePath(isNumber(value) ? value : value[0]), value => isNumber(value) ? emptyArray : value[1]), | ||
hasReusableDiagnostic: true, | ||
affectedFilesPendingEmit: map(program.affectedFilesPendingEmit, value => toFilePath(value[0])), | ||
|
@@ -1300,8 +1307,16 @@ namespace ts { | |
return filePathsSetList![fileIdsListId - 1]; | ||
} | ||
|
||
function toMapOfReferencedSet(referenceMap: ProgramBuildInfoReferencedMap | undefined): ReadonlyESMap<Path, BuilderState.ReferencedSet> | undefined { | ||
return referenceMap && arrayToMap(referenceMap, value => toFilePath(value[0]), value => toFilePathsSet(value[1])); | ||
function toManyToManyPathMap(referenceMap: ProgramBuildInfoReferencedMap | undefined): BuilderState.ManyToManyPathMap | undefined { | ||
if (!referenceMap) { | ||
return undefined; | ||
} | ||
|
||
const map = BuilderState.createManyToManyPathMap(); | ||
referenceMap.forEach(([fileId, fileIdListId]) => | ||
map.set(toFilePath(fileId), toFilePathsSet(fileIdListId)) | ||
); | ||
return map; | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewbranch I retained the existing behavior but, now that I look at it, it seems wrong. The value type of this map is a set - doesn't that mean that the new and old maps will refer to (and modify) the same sets? It seems like the intention was probably to do a deep copy.