GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Reuse Module Resolutions Within Project References During Initial Program Creation by gluxon · Pull Request #40964 · microsoft/TypeScript

Conversation

@gluxon

@gluxon Brandon Cheng (gluxon) commented Oct 6, 2020

Copy link
Copy Markdown
Contributor

Related to #40356 and Build-Free Editing (#32028)

tsserver creates a Program for each ConfiguredProject. If a project contains project references, createProgram will 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.resolvedModules map will be kept up-to-date by other Program objects running in tsserver by the time a new Program is 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 a and b are small but share a large dependency c.

Load time before this change (parent commit 5c55fc0):

  • package a: 25,450ms
  • package b: 9,774ms

Load time after this change:

  • package a: 25,365ms
  • package b: 2,716ms

These timings are non-scientific. I didn't average them over multiple runs.

Further Improvements

  • This PR only applies to initial program creation and not program reuse. Non-trivial refactoring in tryReuseStructureFromOldProgram would be required to make the later happen.
  • We currently recreate the structure of a program entirely when any project reference tsconfig changes. I wonder if it's possible to track which project reference's config changed and only update resolutions of affected source files. It's tricky because affected files would be both files within the project reference and files that import from it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@typescript-bot TypeScript Bot (typescript-bot) added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Oct 6, 2020
@ghost

Deleted user (ghost) commented Oct 6, 2020 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gluxon
Brandon Cheng (gluxon) commented Oct 6, 2020
View reviewed changes
Comment thread src/compiler/program.ts
// 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

@gluxon Brandon Cheng (gluxon) Oct 6, 2020

Copy link
Copy Markdown
Contributor Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
gluxon
Brandon Cheng (gluxon) commented Oct 6, 2020
View reviewed changes
Comment thread src/compiler/program.ts

// The above cases should exhaustively cover all branches. Defaulting to false
// since that's safer in unexpected situations.
default: return false;

@gluxon Brandon Cheng (gluxon) Oct 6, 2020

Copy link
Copy Markdown
Contributor Author

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
gluxon
Brandon Cheng (gluxon) commented Oct 6, 2020
View reviewed changes
Comment thread src/testRunner/unittests/reuseProgramStructure.ts
Comment on lines +831 to +844
// 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;
}

@gluxon Brandon Cheng (gluxon) Oct 6, 2020

Copy link
Copy Markdown
Contributor Author

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.

This part is a bit ugly. I'm welcome for suggestions.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Brandon Cheng (gluxon) added 4 commits October 6, 2020 14:33
@gluxon
Add comments to StructureIsReused enum
cc5cc95
@gluxon
Check structuralIsReused with switch statement
9981db8 
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
Reuse module resolutions in project references
7f9e40d
@gluxon
Add module resolution within project references test
b0f37be
@gluxon
Brandon Cheng (gluxon) force-pushed the reuse-module-resolutions-build-free branch from c9c4292 to b0f37be Compare October 6, 2020 18:34
@sheetalkamat

Sheetal Nandi (sheetalkamat) commented Oct 6, 2020

Copy link
Copy Markdown
Member

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.
gluxon reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gluxon

Brandon Cheng (gluxon) commented Oct 7, 2020

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gluxon Brandon Cheng (gluxon) closed this Oct 7, 2020
@gluxon

Brandon Cheng (gluxon) commented Oct 31, 2020

Copy link
Copy Markdown
Contributor Author

Sheetal Nandi (@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gluxon
Brandon Cheng (gluxon) deleted the reuse-module-resolutions-build-free branch December 7, 2020 23:45
@ericjeney

Eric Jeney (ericjeney) commented Jan 6, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Sheetal Nandi (@sheetalkamat) Are there any updates on when your cached resolution changes will be ready? My team is very excited for these performance improvements.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sheetalkamat

Sheetal Nandi (sheetalkamat) commented Jan 6, 2021

Copy link
Copy Markdown
Member

It is not scheduled for 4.2 #41004 is tracking the work
gluxon and ericjeney reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gluxon Brandon Cheng (gluxon) mentioned this pull request Nov 28, 2022
[WIP] Persist resolutions and program information and reuse it when building program incrementally or during editor #41004
Closed
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@gluxon @sheetalkamat @ericjeney @typescript-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.