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

webpack resolver: cache instance(s) of resolve function #1091

Merged
merged 4 commits into from
May 3, 2018

Conversation

benmosher
Copy link
Member

5x speedup in resolver construction by caching the resolveSync if the webpack config+path matches.

Fixes #788, AFAICT. createResolveSync function was responsible for 83% of the resolver setup time in my tests. After this PR, it is <10%. Went from >5s to ~250ms over a whole project.

@benmosher benmosher requested a review from ljharb April 30, 2018 11:13
@benmosher benmosher changed the title cache instance(s) of resolve function webpack resolver: cache instance(s) of resolve function Apr 30, 2018
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 96.48% when pulling 198a9a8 on cache-webpack-config into 8c9c3b8 on master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 96.48% when pulling 198a9a8 on cache-webpack-config into 8c9c3b8 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 96.48% when pulling 198a9a8 on cache-webpack-config into 8c9c3b8 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 96.48% when pulling 198a9a8 on cache-webpack-config into 8c9c3b8 on master.

@coveralls
Copy link

coveralls commented Apr 30, 2018

Coverage Status

Coverage increased (+1.7%) to 98.177% when pulling 4a3d5ad on cache-webpack-config into 8c9c3b8 on master.

@@ -1,6 +1,6 @@
var findRoot = require('find-root')
, path = require('path')
, get = require('lodash.get')
, _ = require('lodash')
Copy link
Member

Choose a reason for hiding this comment

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

i believe this should actually be get = require('lodash/get') for maximum tree-shake-ability, and for future compat with lodash 5.

Copy link
Member Author

Choose a reason for hiding this comment

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

ooo cool, thanks. I have yet to get familiar with digging things directly out of lodash

var _cache = []
function getResolveSync(configPath, webpackConfig) {
var cacheKey = { configPath: configPath, webpackConfig: webpackConfig }
var cached = find(_cache, function (entry) { return _.isEqual(entry.key, cacheKey) })
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason to use an array here, rather than a Set, given that eslint only works on node 4+?

Copy link
Member Author

Choose a reason for hiding this comment

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

you could arguably still use the latest webpack resolver with much older versions of ESLint and this plugin (at work I only just recently upgraded us from ESLint 2 and plugin v1).

I don't think it is critical to maintain the ES5, at this point, but I also didn't want to cross that bridge today if there wasn't a compelling reason. was easy enough to write this PR in ES5.

- also dropped webpack resolver support for node 0.10 (lol)
😅 _now_ it doesn't support 0.10.x anymore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Eslint is slow when used with webpack
3 participants