Skip to content

Cache file existence in synchronizeHostData #48942

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

Closed
wants to merge 1 commit into from

Conversation

amcasey
Copy link
Member

@amcasey amcasey commented May 3, 2022

Saves 1-2% during mui project load (on Linux), mostly by eliminating duplicate non-existence results.

Saves 1-2% during mui project load (on Linux), mostly by eliminating duplicate non-existence results.
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 3, 2022
@amcasey
Copy link
Member Author

amcasey commented May 3, 2022

In very round numbers, I'm seeing updateGraph time drop from 5900ms to 5700ms.

@@ -1492,11 +1510,25 @@ namespace ts {
}

function fileExists(fileName: string): boolean {
// Check this before the file contents cache because it saves computing the Path
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you are not using changeCompilerHostLikeToUseCache

Copy link
Member Author

Choose a reason for hiding this comment

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

I coded it up that way, but found it confusing because of the interaction with HostCache. changeCompilerHostLikeToUseCache covers

  1. readFile, which is already handled, slightly differently, by HostCache
  2. fileExists
  3. directoryExists
  4. createDirectory, which would not be used
  5. writeFile, which would not be used
  6. getSourceFile, which the caller has to decide to hook up or not and overlaps with HostCache

Some of the nuances, e.g. readFile vs readFileWithCache, felt like they would be confusing in this specialized context.

I believe the HostCache is also used in a few places that consume the language service host, rather than the compiler host.

If you feel strongly about it, I can try to rewrite it in a way that eliminates HostCache altogether, in favor of mutating the host directly.

Copy link
Member

Choose a reason for hiding this comment

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

I am just wondering if we can get rid of host cache and use our standard method so that if any additions or changes we do in future are not missed? Looked at fileExists , readFile etc in services and its used for duration of program creation so would prefer to use compilerHost and changeCompilerHostLikeToUseCache instead of maintaining our own caches..

@@ -1492,11 +1510,25 @@ namespace ts {
}

function fileExists(fileName: string): boolean {
// Check this before the file contents cache because it saves computing the Path
Copy link
Member

Choose a reason for hiding this comment

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

I am just wondering if we can get rid of host cache and use our standard method so that if any additions or changes we do in future are not missed? Looked at fileExists , readFile etc in services and its used for duration of program creation so would prefer to use compilerHost and changeCompilerHostLikeToUseCache instead of maintaining our own caches..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants