⚡ Performance: Project service spends excess time cleaning client files when called synchronously

[JoshuaKGoldberg]

Acknowledgement

• I acknowledge that issues using this template may be closed without further explanation at the maintainer's discretion.

Comment

🔎 Search Terms

project service cleanupProjectsAndScriptInfos openClientFileWithNormalizedPath openClientFile

🙁 Actual behavior

TypeScript project services open a file with service.openClientFile(filePathAbsolute, ...). That internally calls to a cleanupProjectsAndScriptInfos to clean up projects, such as removing orphans. There's no way to skip that cleanup.

When using typescript-eslint's parserOptions.projectService, that cleanup adds (seemingly?) unnecessary overhead per file being linted.

🙂 Expected behavior

When I tried commenting out its contents of cleanupProjectsAndScriptInfos locally, the time for a lint run of ~1024 files with the project service speed up by ~15-20%, from ~3.2 seconds to ~2.5 seconds. ⚡

Request: could we add an option to the ProjectService class to skip cleanups, please?

Alternate request: failing that, maybe the cleanups can be debounced and/or delayed in some way? E.g. added to a queue that only executes if the project service is still active?

Additional information about the issue

On the TypeScript side:

• Point of comparison: JoshuaKGoldberg@2ae3568 would be one way of doing this.
• Handle config file change and default project management #58486 is the PR that introduced cleanupProjectsAndScriptInfos.

On the typescript-eslint side:

• https://github.com/typescript-eslint/performance is the performance repository showing how to create these profiles.
  • https://github.com/typescript-eslint/performance/blob/main/README.md#comparison-project-service-client-file-cleanups has the measurements for this specific investigation.
• feat(typescript-estree): add EXPERIMENTAL_useProjectService option to use TypeScript project service typescript-eslint/typescript-eslint#6754 is the PR that first added usage of the project service in typescript-eslint.
• We in ESLint-land can't explicitly tell TypeScript when files are opened or closed: feat: parsing session objects eslint/rfcs#102. But in typescript-eslint specifically we have an inference of when we're in "single-run" mode: https://v8--typescript-eslint.netlify.app/packages/parser#disallowautomaticsingleruninference.

cc @sheetalkamat as FYI, after a pairing with @jakebailey.

@jakebailey suggested I also include a screenshot like this speedscope.app view of an ESLint CLI run with parserOptions.projectService:

[sheetalkamat]

I will look into performance of cleanupProjectsAndScriptInfos but are you opening one file at a time. You batch open files using protocol.CommandTypes.UpdateOpen which uses projectService.applyChangesInOpenFiles (which i just realise is internal but worth a try) so cleanup happens at the end of opening all those files.

The cleanup is two folds:

• Deleted files and projects are deferred and kept around.
• Closed files and projects that are no longer needed are removed.
  So i am not sure its good idea to skip that part..

This is already delayed portion where we dont do this immediately (like file delete or close) but on file open because in most cases projects and script infos in editor scenario can be reused by next file open.

[jakebailey]

Unfortunately the access pattern of eslint itself is undefined; all it does is call the plugin to ask to parse a specific file. It doesn't provide the list up front or say when it's no longer using a file anymore... 😞

[sheetalkamat]

Can you please try out #59689 (comment) to see if it helps

[sheetalkamat]

Unfortunately the access pattern of eslint itself is undefined; all it does is call the plugin to ask to parse a specific file. It doesn't provide the list up front or say when it's no longer using a file anymore... 😞

I am also open to skipping this as an option but when do you suppose eslint will do "cleanup" to remove the files. We would need to expose that part as well. But I am trying to see if fast exits help or not before we do this.

[JoshuaKGoldberg]

Can you please try out #59689 (comment) to see if it helps

It does, fantastic! 👏 Looks like most, but not all, of the performance discrepancy is gone:

┌──────────┬───────┬───────────────────────┬───────────────────────┐
│ version  │ files │ project (even layout) │ service (even layout) │
┼──────────┼───────┼───────────────────────┼───────────────────────┤
│ baseline │ 1024  │ '2.524 s ±  0.016 s'  │ '3.338 s ±  0.013 s'  │
┼──────────┼───────┼───────────────────────┼───────────────────────┤
│ modified │ 1024  │ '2.531 s ±  0.010 s'  │ '2.846 s ±  0.010 s'  │
┴──────────┴───────┴───────────────────────┴───────────────────────┘

when do you suppose eslint will do "cleanup" to remove the files.

I don't know. I've asked around internally. In the meantime, the best answer I can guess at is "eventually but not extremely soon".

[sheetalkamat]

I don't know. I've asked around internally. In the meantime, the best answer I can guess at is "eventually but not extremely soon".

Till we have some idea on what to do here i dont think its advisable to let the collection not happen. In mean time we can get in that optimization PR

[JoshuaKGoldberg]

Update: there's no plan to give this sort of information (whether ESLint is in single-run mode, when files are done being linted, etc.) to parsers. We should assume parsers in ESLint-land won't have access to that for a while.

Anyway, thanks for working on this! Really encouraging to see so much of the performance discrepancy go away. ⚡