-
Notifications
You must be signed in to change notification settings - Fork 12.8k
β‘ Performance: Project service doesn't cache all fs.statSync #59338
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
Comments
I will take a look but do you really need |
What options should they be setting to make that project not used? They definitely don't need auto import anything. |
|
@jakebailey can you try and see if un cached statsync and realpath calls reduce after that? |
Hm, I sent typescript-eslint/typescript-eslint#9586 but it's saying:
Have not tested locally (will do that shortly); maybe this func is new? |
Seems like atleast 5 year old |
Oh, the unit tests in typescript-eslint mock out the service - see https://github.com/typescript-eslint/typescript-eslint/blob/5542aeb9440280dd4c0be529e9842f12221737ff/packages/typescript-estree/tests/lib/createProjectService.test.ts#L8 |
Ah, yeah, so I'll definitely have to real-world test it. |
I think the eslint change to disable auto import provider should help with the stats cache issue. With that change those high number of stats cache hitting goes down significantly. I confirmed that by running tsserver that just by default returns "off" for that option. The other two locations where we get from cache are:
|
Before:
After typescript-eslint/typescript-eslint#9586:
So, clearly better by a little bit. I haven't yet done the same stat counting augmentation; I'm not sure which command to run to exactly reproduce what @JoshuaKGoldberg was doing anymore. |
Okay, I checked and setting |
Hm, then I remove the code that sets that and the result is the same; I may just be measuring this wrong. I think I'd need Josh to retest. |
I see it reducing the countsΒ (hooray!) down to three per file. Am I holding it wrong, maybe?
This from:
|
Then after I see:
So, yeah, there's something odd going on here. |
For files in |
Here are the three traces for a file, as an example:
|
Now, what might be possible for ts-eslint is when you infer that it's a single run, provide a host that caches everything, since you're assuming that it's just going to exit after running anyway. |
Acknowledgement
Comment
π Search Terms
project service fs stat statSync
π Actual behavior
When using typescript-eslint's
parserOptions.projectService
, type checking APIs switch from the traditional manual TypeScriptts.Program
approach to the editor-stylets.ProjectService
. We're observing excess calls to thets.sys.statSync
function on some paths innode_modules/
- up to a few dozen for some paths (!).π Expected behavior
There should be no uncached
statSync
calls, I'd think? Even if in a persistent session, I'd expect them to be debounced in some way.As a draft, I added a basic caching
Map
tostatSync
and ran a before & after comparison with hyperfine. The results showed a ~7-12% improvement in lint time:diff
patch to switch to the Caching variant...Additional information about the issue
On the typescript-eslint side:
These are the top 10 most common paths called by
statSync
...Here's an example call stack from the most common one...
cc @sheetalkamat as FYI, after a pairing with @jakebailey.
The text was updated successfully, but these errors were encountered: