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

Performance issues after updating to 3.8.0 #348

Closed
jtarvainen opened this issue Feb 16, 2025 · 16 comments · Fixed by #352
Closed

Performance issues after updating to 3.8.0 #348

jtarvainen opened this issue Feb 16, 2025 · 16 comments · Fixed by #352

Comments

@jtarvainen
Copy link

I encountered a massive performance hit in my Meteor project after updating from 3.7.0 to 3.8.0. An eslint run that previously took approx. 6 seconds locally (MacOS) now took approx. 75 s (12x). I also noticed the impact in a Linux environment, though there it was less severe (approx. 40 % slowdown).

I assume this is related to the switch from fast-glob to tinyglobby. Perhaps something to look into?

@SukkaW
Copy link
Collaborator

SukkaW commented Feb 16, 2025

I encountered a massive performance hit in my Meteor project after updating from 3.7.0 to 3.8.0. An eslint run that previously took approx. 6 seconds locally (MacOS) now took approx. 75 s (12x). I also noticed the impact in a Linux environment, though there it was less severe (approx. 40 % slowdown).

I assume this is related to the switch from fast-glob to tinyglobby. Perhaps something to look into?

#345 could also be a suspect as well.

I will look into this when I have time. In the meantime, you could try git bisect and npm install [path/to/local/clone] to figure out which commit is bad.

Also cc @carlocorradini on this.

@carlocorradini
Copy link
Contributor

Before invoking the mapping function, we must ensure that the current file is allowed to resolve the path (this eliminates a variety of issues).
The mapping structure is constructed by the initMapper function, which expands the glob pattern for searching the tsconfig.json files. Then, for each tsconfig.json, we expand the glob patterns in the include and file.
I'm not sure which glob implementation is faster. Nonetheless, I dislike tinyglobby since the specified input requires POSIX separators (else it won't work) and the returned array of files contains POSIX separators. As a result, for both input and output, we must convert the path to its native format.
Moreover, I don't know how the various caches work in eslint (even though I've seen it quite a lot in the index.ts file).
In conclusion, I think the only two possible optimizations are:

  1. Choose a faster and better glob implementation
  2. Improve the caching mechanism (even though it's already present)

What do you think?

@carlocorradini
Copy link
Contributor

@jtarvainen Give additional information about your environment:

  • OS?
  • Number of tsconfig.json files?
  • Number of files given to ESLint?
  • Additional context/information

Thanks

@justnewbee
Copy link

My lint-stage hangs on eslint forever even only 1 file is changed. Seems 3.7 is OK. Had to lock that version for now.

@carlocorradini
Copy link
Contributor

My lint-stage hangs on eslint forever even only 1 file is changed. Seems 3.7 is OK. Had to lock that version for now.

Can you give us a reproducible example so we have something to analyze?
Thanks 🙏

@carlocorradini
Copy link
Contributor

@jtarvainen @justnewbee
Does your tsconfig.json have an include or files option?
If none of them have been defined, we use the TypeScript "algorithm" and search for everything within the same directory with glob **/* via tinyglobby

@jtarvainen
Copy link
Author

@carlocorradini At least mine doesn't.

@carlocorradini
Copy link
Contributor

@jtarvainen
Maybe we found the cause 😬
Try adding include (if you have many files and want to use a glob pattern) or files (if you have a very small amount of files and don't want to use a glob pattern) to your tsconfig.json and see if anything changes.

@ehoogeveen-medweb
Copy link

I'm also seeing a huge performance regression between 3.7.0 and 3.8.0 - from 1.5 minutes to 11.5 minutes on our main project, and from 13 seconds to 218 seconds on a smaller project.

include or files is set in all our tsconfig.json files, so I don't think that's the issue. I probably won't have time to investigate until Wednesday, but I'll try to get a smaller reproduction at that point if no one beats me to it.

@melihplt
Copy link

On MacOS, when I upgrade to 3.8.0 I see several prompts to access different folders.

  • ...would like to access data from other apps.
  • ...would like to access photos
  • ...would like to access downloads etc.

This feels like something serious may be happenning here. It took a couple hours to understand that this is caused because of this package only. I made sure no other packages, including secondary dependencies were updated.

For precaution I didn't give access and I rolled back the upgrade.

@carlocorradini
Copy link
Contributor

PR #352 should fix the issue
While resolving the tsconfig include glob, the cwd is now set to the same directory as tsconfig.json.
I didn't notice it previously.
Thank you for your feedback and I apologize to everyone for the inconvenience. 😔🙏
When the PR is merged and the package is updated with the new patch, I'll wait for your feedback.

@jtarvainen
Copy link
Author

@carlocorradini Will this fix the issue even if one's tsconfig doesn't contain the include option (as mine doesn't)?

@carlocorradini
Copy link
Contributor

carlocorradini commented Feb 17, 2025

@jtarvainen If neither the include nor the files options are specified, we search for everything (**/*) in the same cwd as the tsconfig.json (ignoring the node_modules directory and everything in the exclude option). This is the same behavior as TypeScript.
Nonetheless, it's always ideal to set include or files options to limit the range of files given to TypeScript but also other tools like this one.

PS: How many files are in your project?
PSPS:

globSync(defaultInclude, {
ignore: [
...(tsconfigResult.config.exclude ?? []),
...defaultIgnore,
],
absolute: true,
cwd: path.dirname(tsconfigResult.path),
})

@ehoogeveen-medweb
Copy link

I can confirm that the change in #352 fixes the regression for our projects.

@carlocorradini
Copy link
Contributor

@ehoogeveen-medweb Awesome thanks! 🥳

@JounQin
Copy link
Collaborator

JounQin commented Feb 17, 2025

v3.8.1 has been released, thanks @carlocorradini!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

7 participants