Skip to content

Lazy calculation of expensive file explaining diagnsotics and some caching to be used to share the diagnostic data#58398

Merged
sheetalkamat merged 9 commits intomainfrom
forceConsistentFileNames
May 6, 2024
Merged

Lazy calculation of expensive file explaining diagnsotics and some caching to be used to share the diagnostic data#58398
sheetalkamat merged 9 commits intomainfrom
forceConsistentFileNames

Conversation

@sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented May 1, 2024 •
edited
Loading

  • Store information about file explaining diagnostics to be generated only when program diagnostics are asked. We store this already if it's the diagnostic while we are creating program graph, we now store other compiler options errors that explain file include reasons as well as lazy diagnostics to be generated and then actually populate it when program diagnostics are queried.
  • In diagnostic explaining the reason why file is in program, we are caching details as well as related info that can be reused when there are multiple diagnostics for same file. The cache is cleared after population of program diagnostics is complete so doesn't stay beyond the computation of these diagnostics.

Probably fixes

cc: @jakebailey
As discussed offline few days ago the real issue is that typescript-eslint is passing in canonical file path of tsconfig and that generates lot of errors in program because of forceConsistentCasingInFileNames being defaulting to true in later versions of typescript. I have typescript-eslint/typescript-eslint#9042 for tsconfig issue we discussed on wednesday but i dont know if there are any other eg source files or anything that are having these issues as i didnt get to look into code that deeply

Here are some perf numbers with the repro in #58011 (comment) where API is used with and without config file name lower casing. Special thanks to @reduckted to give us this repro so we can actually verify this.

When diagnostics are queried (runs emitFilesAndReportErrors) after creating watch program:

  tsc (5.4.5) eslint (5.4.5) delta (5.4.5) tsc (fix) eslint (fix) delta (fix) tsc (delta with fix) eslint (delta with fix)
Files: 4843 4843   4843 4843      
Memory used: 313744K 1366218K 335.46% 334185K 363474K 8.76% 6.52% -73.40%
Program time: 9.38s 31.98s 240.94% 9.16s 9.18s 0.22% -2.35% -71.29%
Total time: 11.22s 33.40s 197.68% 10.50s 10.72s 2.10% -6.42% -67.90%
Detailed Extended Diagnostics
  tsc (5.4.5) eslint (5.4.5) delta (5.4.5) tsc (fix) eslint (fix) delta (fix) tsc (delta with fix) eslint (delta with fix)
Files: 4843 4843   4843 4843      
Lines of Library: 38041 38041   38310 38310      
Lines of Definitions: 174189 174189   174189 174189      
Lines of TypeScript: 32561 32561   32561 32561      
Lines of JavaScript: 30 30   30 30      
Lines of JSON: 0 0   0 0      
Lines of Other: 0 0   0 0      
Identifiers: 201753 201753   201976 201976      
Symbols: 140060 140060   140227 140227      
Types: 2272 2272   2273 2273      
Instantiations: 168 168   168 168      
Memory used: 313744K 1366218K 335.46% 334185K 363474K 8.76% 6.52% -73.40%
Assignability cache size: 24 24   24 24      
Identity cache size: 0 0   0 0      
Subtype cache size: 0 0   0 0      
Strict subtype cache size: 0 0   0 0      
I/O Read time: 0.98s 1.16s   0.91s 0.93s      
Parse time: 1.61s 2.00s   1.50s 1.47s      
ResolveModule time: 1.34s 1.51s   1.25s 1.16s      
ResolveTypeReference time: 0.03s 0.03s   0.03s 0.03s      
ResolveLibrary time: 0.06s 0.04s   0.05s 0.05s      
Program time: 9.38s 31.98s 240.94% 9.16s 9.18s 0.22% -2.35% -71.29%
Bind time: 0.61s 0.67s   0.61s 0.67s      
Check time: 1.22s 0.74s   0.72s 0.87s      
printTime time: 0.00s 0.00s   0.00s 0.00s      
Emit time: 0.01s 0.01s   0.00s 0.00s      
Total time: 11.22s 33.40s 197.68% 10.50s 10.72s 2.10% -6.42% -67.90%

When diagnostics are not queried which is what es-lint does as per @jakebailey

  tsc (5.4.5) eslint (5.4.5) delta (5.4.5) tsc (fix) eslint (fix) delta (fix) tsc (delta with fix) eslint (delta with fix)
Files: 4843 4843   4843 4843      
Memory used: 290762K 1352409K 365.13% 324293K 320025K -1.32% 11.53% -76.34%
Program time: 9.63s 26.77s 177.99% 9.31s 9.60s 3.11% -3.32% -64.14%
Total time: 10.30s 27.50s 166.99% 9.89s 10.13s 14.31s 41.26% -63.16%
Detailed Extended Diagnostics
  tsc (5.4.5) eslint (5.4.5) delta (5.4.5) tsc (fix) eslint (fix) delta (fix) tsc (delta with fix) eslint (delta with fix)
Files: 4843 4843   4843 4843      
Lines of Library: 38041 38041   38310 38310      
Lines of Definitions: 174189 174189   174189 174189      
Lines of TypeScript: 32561 32561   32561 32561      
Lines of JavaScript: 30 30   30 30      
Lines of JSON: 0 0   0 0      
Lines of Other: 0 0   0 0      
Identifiers: 201753 201753   201976 201976      
Symbols: 140029 140029   140196 140196      
Types: 89 89   88 88      
Instantiations: 0 0   0 0      
Memory used: 290762K 1352409K 365.13% 324293K 320025K -1.32% 11.53% -76.34%
Assignability cache size: 0 0   0 0      
Identity cache size: 0 0   0 0      
Subtype cache size: 0 0   0 0      
Strict subtype cache size: 0 0   0 0      
I/O Read time: 0.97s 0.99s   0.93s 0.99s      
Parse time: 1.74s 1.66s   1.51s 1.59s      
ResolveModule time: 1.42s 1.22s   1.26s 1.20s      
ResolveTypeReference time: 0.03s 0.03s   0.03s 0.03s      
ResolveLibrary time: 0.05s 0.03s   0.06s 0.05s      
Program time: 9.63s 26.77s 177.99% 9.31s 9.60s 3.11% -3.32% -64.14%
Bind time: 0.67s 0.73s   0.58s 0.54s      
Total time: 10.30s 27.50s 166.99% 9.89s 10.13s 14.31s 41.26% -63.16%

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 1, 2024
@sheetalkamat
Copy link
Member Author

sheetalkamat commented May 2, 2024

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 2, 2024 •
edited
Loading

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
pack this ✅ Started ✅ Results

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 2, 2024 •
edited
Loading

Hey @sheetalkamat, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/161598/artifacts?artifactName=tgz&fileId=3F3D97042F3CD53F19137FD5481087CF59977AC9681B5F1C2DEDE0CAE6673D6E02&fileName=/typescript-5.5.0-insiders.20240502.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

This was referenced May 2, 2024
Closed
Closed
Closed
@sheetalkamat sheetalkamat changed the title [wip] on demand program diagnostics population Lazy calculation of expensive file explaining diagnsotics and some caching to be used to share the diagnostic data May 3, 2024
This was linked to issues May 3, 2024
Increased memory usage when creating a watch program that includes local JavaScript files in node_modules #58011
Closed
OOM typescript, mac m1+. #58274
Closed
@sheetalkamat sheetalkamat marked this pull request as ready for review May 3, 2024 19:29
@sheetalkamat sheetalkamat mentioned this pull request May 3, 2024
Merged
3 tasks
@jakebailey jakebailey mentioned this pull request May 6, 2024
Closed
jakebailey
jakebailey reviewed May 6, 2024
View reviewed changes
Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

Behavior seems correct, but a few small comments.

@sheetalkamat sheetalkamat force-pushed the forceConsistentFileNames branch from adfa25e to 78c42a7 Compare May 6, 2024 18:15
@sheetalkamat sheetalkamat force-pushed the forceConsistentFileNames branch from 78c42a7 to 4e58b5f Compare May 6, 2024 18:42
@sheetalkamat sheetalkamat requested a review from jakebailey May 6, 2024 19:22
@sheetalkamat sheetalkamat merged commit fd81d04 into main May 6, 2024
@sheetalkamat sheetalkamat deleted the forceConsistentFileNames branch May 6, 2024 23:30
@stackyism stackyism mentioned this pull request Jun 18, 2024
Closed
@kronodeus
Copy link

kronodeus commented Jun 28, 2024 •
edited
Loading

@sheetalkamat I am experiencing an error with these changes in my project when using getCombinedCodeFix:

TypeError: Cannot read properties of undefined (reading 'push')                                                                                                                                                                                                                                               
                                                                                                                                                                                                                                             
      at addLazyProgramDiagnosticExplainingFile (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:126723:41)                                                                                
      at checkSourceFilesBelongToPath (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:126142:11)                                                                                          
      at ../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:124558:37                                                                                                                         
      at getCommonSourceDirectory (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:117872:53)
      at Object.getCommonSourceDirectory2 [as getCommonSourceDirectory] (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:124553:31)
      at getDeclarationEmitOutputFilePath (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:20116:119)
      at getOutputPathsFor (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:117787:96)
      at transformRoot (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:115914:62)
      at transformation (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:117457:14)
      at transformRoot (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:117480:71)
      at transformNodes (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:117465:70)
      at getDeclarationDiagnostics (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:115644:18)
      at ../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:125420:14
      at runWithCancellationToken (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:125122:14)
      at getDeclarationDiagnosticsForFileNoCache (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:125418:12)
      at getAndCacheDiagnostics (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:125429:20)
      at getDeclarationDiagnosticsWorker (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:125415:12)
      at getDeclarationDiagnosticsForFile (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:125438:48)
      at getDiagnosticsHelper (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:125070:44)
      at Object.getDeclarationDiagnostics2 [as getDeclarationDiagnostics] (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:125108:14)
      at getDiagnostics (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:151176:18)
      at eachDiagnostic (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:151162:23)
      at Object.getAllCodeActions (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:154119:5)
      at Object.getAllFixes (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:151148:65)
      at Object.getCombinedCodeFix (../../node_modules/.pnpm/@[email protected]/node_modules/@ts-morph/common/dist/typescript.js:149433:31)

Unfortunately I am not exactly sure how to reproduce this issue outside of my codebase, but it seems there is some code path that calls addLazyProgramDiagnosticExplainingFile (which calls push on lazyProgramDiagnosticExplainingFile assuming it is defined) after updateAndGetProgramDiagnostics is called, which sets it to undefined.

Here is the call stack that hits updateAndGetProgramDiagnostics which sets the array to undefined before the above call stack:

updateAndGetProgramDiagnostics
getProgramDiagnostics
getSemanticDiagnosticsForFile
getDiagnosticsHelper
Object.getSemanticDiagnostics
getDiagnostics
eachDiagnostic
getAllCodeActions
Object.getAllFixes
Object.getCombinedCodeFix

This is occurring in 5.5.2

@kronodeus
Copy link

kronodeus commented Jun 28, 2024

In codeFixProvider.ts#getDiagnostics I see we are getting semantic diagnostics before getting declaration diagnostics. As you can see in the stack traces above, the code path which sets lazyProgramDiagnosticExplainingFile to undefined is reached while getting semantic diagnostics while the code path which attempts to push to it is reached while getting declaration diagnostics. The change to get declaration diagnostics after semantic diagnostics came from this commit.

My hunch is that in order to reproduce this issue you'd need to request code fixes for a file that has both semantic diagnostics AND declaration diagnostics. The error doesn't occur if I set "declaration": false and "composite": false in my tsconfig.json because that causes this if-statement to evaluate to false and skip declaration diagnostics.

@sheetalkamat
Copy link
Member Author

sheetalkamat commented Jun 28, 2024

Please send the tsserver log so we can investigate

@sheetalkamat
Copy link
Member Author

sheetalkamat commented Jun 28, 2024

Ok I have the repro .. will look into this

@sheetalkamat sheetalkamat mentioned this pull request Jun 28, 2024
Merged
@zachstence zachstence mentioned this pull request Jul 30, 2024
Closed
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jakebailey jakebailey jakebailey approved these changes

@andrewbranch andrewbranch Awaiting requested review from andrewbranch

Assignees

@sheetalkamat sheetalkamat

Labels

Author: Team 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.

OOM typescript, mac m1+. Increased memory usage when creating a watch program that includes local JavaScript files in node_modules

4 participants

@sheetalkamat @typescript-bot @kronodeus @jakebailey