Reuse Module Resolutions Within Project References During Initial Program Creation #40964
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
|
| // function to reuse resolutions for its checks. Returning true to accommodate | ||
| // for that specific scenario. | ||
| // | ||
| // TODO: Clean this because the above scenario is a bit unexpected. This case |
There was a problem hiding this comment.
I think I saw a rule that TODO lines need an associated issue, but I'll wait for someone to confirm that this comment is correct before I create that.
|
|
||
| // The above cases should exhaustively cover all branches. Defaulting to false | ||
| // since that's safer in unexpected situations. | ||
| default: return false; |
There was a problem hiding this comment.
This isn't an exhaustive switch statement because StructureIsReused sets bit flags manually. (I assume for performance reasons.) Is it worth keeping it that way?
| // Instead of hard-coding file system entries for this test, we could also write a function that more | ||
| // generally transforms a list of files into a tree of FileSystemEntries. | ||
| function getFileSystemEntries(path: string) { | ||
| const mapPathToFileSystemEntries: { [path: string]: FileSystemEntries | undefined } = { | ||
| "/a": { files: [], directories: ["src"] }, | ||
| "/a/src": { files: ["index.ts", "x.ts"], directories: [] } | ||
| }; | ||
|
|
||
| const entries = mapPathToFileSystemEntries[path]; | ||
| if (!entries) { | ||
| throw new Error(`Unexpected path "${path}" requested from readDirectory. Test is broken.`); | ||
| } | ||
| return entries; | ||
| } |
There was a problem hiding this comment.
This part is a bit ugly. I'm welcome for suggestions.
Returning `true` when `structuralIsReused === undefined` is a bit
unexpected, but matches the existing behavior of the program. Changing
it to `false` breaks the following test:
1) unittests:: Reuse program structure:: General
can reuse ambient module declarations from non-modified files:
should reuse 'fs' since node.d.ts was not changed
+ expected - actual
gluxon
force-pushed
the
reuse-module-resolutions-build-free
branch
from
c9c4292 to
b0f37be
Compare
|
Concept is great but this is not the implementation we are looking at. I am moving resolvedModules away from sourceFile into program to ensure that incremental builder will be able to use that. As part of that i am doing changes to get cached resolution from projectReferences if available and that achieves exactly same thing as this PR but more generically that concept can be used in multiple places like tsserver, compiling etc. |
|
Hey Sheetal, thanks for looking at this. I agree. This PR was meant to be a v1 of what this would look like. An alternative version that's more aware of project references would definitely be better from a maintainability perspective. Let me know if you'd like help! I'm not sure what my availability will look like week by week but we can talk about that if you want my involvement. If not I trust you and the team will do a fanstatic job like always. |
|
@sheetalkamat Is there a place the improved implementation is being tracked? At the moment loading all projects individually takes significantly longer than loading a single project with all files combined. I was using "Find All References" today and saw it trigger a recursive project load that hanged TS Server for 10+ minutes. I played with 1 tsconfig file that includes all TypeScript files. That loaded in ~1 minute. |
|
@sheetalkamat Are there any updates on when your cached resolution changes will be ready? My team is very excited for these performance improvements. |
|
It is not scheduled for 4.2 #41004 is tracking the work |
Related to #40356 and Build-Free Editing (#32028)
tsservercreates aProgramfor eachConfiguredProject. If a project contains project references,createProgramwill traverse the source files of those referenced projects recursively.When multiple projects are loaded, shared dependencies will have their module resolutions re-traversed. I think we can relax that behavior a bit to gain some performance improvements. The assumption is that the
SourceFile.resolvedModulesmap will be kept up-to-date by otherProgramobjects running intsserverby the time a newProgramis created. If this assumption is false, this PR should be rejected.I recommend reviewing commit by commit. The first 2 commits don't change any behavior and are small refactors to make the actual change easier to fit in.
Thanks to whoever reviews this. I'm sure changes will be requested.
Performance Differences
For an extremely contrived test case, this improved initial load time of a project by 72.21%. See ts-module-resolution-cache-testing for the test repo. Packages and
aandbare small but share a large dependencyc.Load time before this change (parent commit 5c55fc0):
Load time after this change:
These timings are non-scientific. I didn't average them over multiple runs.
Further Improvements
tryReuseStructureFromOldProgramwould be required to make the later happen.