Repository navigation
Fix performance regression when reusing old state - #28028
Sheetal Nandi (sheetalkamat) merged 2 commits into
Conversation
| let noDiagnosticsTypeChecker: TypeChecker; | ||
| let classifiableNames: UnderscoreEscapedMap<true>; | ||
| let modifiedFilePaths: Path[] | undefined; | ||
| let unmodifiedSourceFilesWithAmbientModules: SourceFile[] | undefined; |
There was a problem hiding this comment.
This could also be converted to a Map to further increase lookup performance.
|
The second commit of this PR uses a map instead of an array. This might not bring much additional performance improvement, but makes the lookup easier. |
|
How many time needed for merge the fix into master and release? |
|
Ryan Cavanaugh (@RyanCavanaugh) can someone look into this? This is really important performance fix. |
|
Reactive Fox (@thekiba) once this PR lands in master it needs to be backported to release 3.1 before it can be released in a patch release. |
| } | ||
|
|
||
| modifiedFilePaths = modifiedSourceFiles.map(f => f.newFile.path); | ||
| const modifiedFiles = modifiedSourceFiles.map(f => f.oldFile); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
|
||
| modifiedFilePaths = modifiedSourceFiles.map(f => f.newFile.path); | ||
| const modifiedFiles = modifiedSourceFiles.map(f => f.oldFile); | ||
| for (const oldFile of oldSourceFiles) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
Ryan Cavanaugh (@RyanCavanaugh) and/or Sheetal Nandi (@sheetalkamat) this issue is affecting more and more people as TS 3.1 gets widely used now since Angular 7 was released. I had to stall the upgrade to v7 because the regression is quite noticeable in our dev/CI environments. Klaus Meinhardt (@ajafff) answered Sheetal Nandi (@sheetalkamat) so is there further feedback or changes needed? Can we give some love to this issue? \: |
|
Sheetal Nandi (@sheetalkamat) could these changes be part of TS 3.1.6? This problem is affecting a lot of Angular users right now. Since Angular need to test compatibility with each TS minor release individually, we cannot change our peer dependency from ~3.1 to >3.2 without a lot of testing. Right now all users of Angular 7 are being affected by this performance regression. |
|
I opened #28282 to port this change to v3.1 |
Fix performance regression when reusing old state (#28028)
|
Check out TypeScript 3.1.6 for the fix. |
Fixes: #28025
This (hopefully) fixes the performance regression reported in #28025.
moduleNameResolvesToAmbientModuleInNonModifiedFilemay be executed for each import (or type reference directive) in every file of the program.Previously it would iterate through all files of the program where most of them are likely to have no ambient module names.
The new implementation stores a file of unmodified files that contain ambient module names. This list is typically a lot shorter than the list of all files.
Note that I removed some unnecessary parameters because their contents are already available in an enclosing scope.
Also note that the old implementation used
modifiedFilePathseven if it wasundefined. In that case it assumed all files to be unchanged. But in fact every file could have been changed because reusing the old program aborted before populating that variable.