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: cache browserSelection by resolved browserslist #199

Merged

Conversation

thoughtspile
Copy link
Contributor

Stylelint is very slow in our projects (1m10s for 1300 files, 8m 3200 files) when using https://github.com/RJWadley/stylelint-no-unsupported-browser-features. After some research, we isolated the issue to doiuse.

I see @cmrn added missingSupportCache in #182 — however, when options.browsers is not provided (the default), CSS file path is used as the cache key, so the cache is not actually shared between files. The benchmark linked master...RJWadley:doiuse:benchmark-2 explictly passses browsers, so it likely mismeasures the perf impact.

browserslist had a similar caching issue, which we just fixed in v4.24.4 with browserslist#862 browserslist#864, so the config resolution should be fine now.

Building upon these enhancements, we can simplify caching: browserslist caches config resolution on its side, and doiuse only caches compileBrowserSupport.

This change speeds up stylelint 8x in our case, from 1m10s to 9s. @cmrn I see you have access to more benchmarks, maybe you could check them as well?

@thoughtspile
Copy link
Contributor Author

We could also bump browserslist depedendency to ^4.24.4 to ensure latest caching, but it's optional.

@thoughtspile
Copy link
Contributor Author

Hey @clshortfuse could you take a look?

@clshortfuse
Copy link
Collaborator

Hi, thanks for the commit! It would be clearer, though not functionally different, if the cache were part of the class as a static: eg: static #browserSupportCache = new Map();.

@thoughtspile
Copy link
Contributor Author

@clshortfuse clshortfuse merged commit b3bce40 into anandthakker:master Jan 13, 2025
3 checks passed
@thoughtspile
Copy link
Contributor Author

Cool, thanks for merging! Could we release this, or do you want to add something?

@thoughtspile
Copy link
Contributor Author

@clshortfuse would be great to release this!

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

Successfully merging this pull request may close these issues.

2 participants