Skip to content
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

Slow performance due to unnecessary updateProgram synchronisation #52117

Closed
marvinhagemeister opened this issue Jan 5, 2023 · 2 comments
Closed
Assignees
Labels
Needs More Info The issue still hasn't been fully clarified

Comments

@marvinhagemeister
Copy link

marvinhagemeister commented Jan 5, 2023

Bug Report

I'm currently investigating a performance issue with TypeScript as part of a lint task with eslint and @typescript/eslint-plugin. Looking at a CPU trace (generated via node --cpu-prof) I can see that TypeScript's getSourceVersion takes up 8% of the total linting time.

Screenshot 2023-01-05 at 14 44 16

Note: This is a left-heavy flamegraph, meaning bars don't represent individual calls

The implementation of that function looks pretty harmless, so I did some further digging and noticed that this function is called a whopping 33,000,000 times during the whole duration of the linting task. Whereas the compiler host only has references to a few thousand files in its caches. Note that eslint is NOT running in watch mode, and no files changed on disk.

What's even more odd is that the surrounding sourceFileNotUptoDate function always returns false. This leads me to suspect that files are checked despite nothing having changed. Following the callstack, the origin where those up-to-date checks original from resides in @typescript-eslint/typescript-estree:

https://github.com/typescript-eslint/typescript-eslint/blob/4ab9bd7f7c1812df4371d1fd3202969c1039e8a7/packages/typescript-estree/src/create-program/getWatchProgramsForProjects.ts#L194

    let fileList = programFileListCache.get(tsconfigPath);
    let updatedProgram: ts.Program | null = null;
    if (!fileList) {
      updatedProgram = existingWatch.getProgram().getProgram();
      fileList = updateCachedFileList(
        tsconfigPath,
        updatedProgram,
        parseSettings,
      );
    }

   if (fileList.has(filePath)) {
      log('Found existing program for file. %s', filePath);

      updatedProgram =
        updatedProgram ?? existingWatch.getProgram().getProgram(); // <-- THIS HERE TRIGGERS ALL FILES TO BE RE-CHECKED
      // sets parent pointers in source files
      updatedProgram.getTypeChecker();

      return [updatedProgram];
  }

Based on that my understanding is that whenever eslint comes across a new file that should be linted, it may already be loaded by TypeScript itself. From what I'm gathering an additional call to .getProgram().getProgram() forces TypeScript to re-evaluate the whole world and check if every single file it knows about changed.

I've tried creating a reproducible scenario that could be shared from that project, but I haven't been able to do so after a day of trying.

I'm unsure how to proceed from here and need some guidance with folks more familiar with TypeScript's internals and how the compiler API is supposed to be used. My initial reaction was that it's a bit unexpected that .getProgram() re-evaluates every file. Or maybe there is a different API that should be used instead? Is it maybe a misusage of .getProgram() and the result should be manually cached instead?

🔎 Search Terms

Performance, eslint, @typescript-eslint, eslint-plugin

🕗 Version & Regression Information

  • This is a crash - No
  • This changed between versions ______ and _______ - Doesn't seem to be a regression
  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about - yep
  • I was unable to test this on prior versions because Oldest version I tested was 4.7.2 where the issue is present too.

⏯ Playground Link

I've tried creating a reproducible scenario that could be shared from that project, but I'm haven't been able to do so after a day of trying.

💻 Code

see above.

🙁 Actual behavior

TypeScript does a lot more checks if files are up to date than expected, despite nothing having changed on disk.

🙂 Expected behavior

TypeScript only checks if something changed if a file actually changed or a specific file is invalidated via some API call.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Jan 5, 2023
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 5.0.1 milestone Jan 5, 2023
@sheetalkamat
Copy link
Member

By any chance is this run when there is custom module resolver (https://github.com/typescript-eslint/typescript-eslint/blob/4ab9bd7f7c1812df4371d1fd3202969c1039e8a7/packages/typescript-estree/src/create-program/getWatchProgramsForProjects.ts#L285). You probably watch to implement hasInvalidatedResolution do determine if the program can be reused because the module resolutions in the file didnt change. #50776 made this API public.

@sheetalkamat sheetalkamat added Needs More Info The issue still hasn't been fully clarified and removed Needs Investigation This issue needs a team member to investigate its status. labels Feb 8, 2023
@marvinhagemeister
Copy link
Author

Could very well be that a custom module resolver was used. I'm not part of the project anymore where I encountered that issue so I cannot check. Closing this issue since there is no actionable step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs More Info The issue still hasn't been fully clarified
Projects
None yet
Development

No branches or pull requests

3 participants