Skip to content
This repository was archived by the owner on Feb 18, 2024. It is now read-only.

Improve the performance of config generation #239

Closed
eliperelman opened this issue Jun 7, 2017 · 9 comments
Closed

Improve the performance of config generation #239

eliperelman opened this issue Jun 7, 2017 · 9 comments

Comments

@eliperelman
Copy link
Member

Not sure how this would work yet, but it would be great to cache the accumulation of middleware between command runs. Some sort of combination of hashing package.json, yarn.lock, .neutrinorc.js, and any other used middleware, stored in a dot file in cwd.

@edmorley
Copy link
Member

edmorley commented Oct 6, 2017

I guess one complication is that if we only hashed .neutrinorc.js, it wouldn't cover local files imported therein.

In addition to more Neutrino-specific caching, I guess it might be worth looking into v8-compile-cache. Initial testing locally (ie adding it to the main neutrino package.json and then adding require('v8-compile-cache'); to the top of packages/neutrino/bin/neutrino), sped up time packages/neutrino/bin/neutrino lint --inspect for this repo from 1.75s to 1.44s. Not much, but a start.

We may also want to do some profiling to see if we can fix any underlying slowness, which depending on wins might mean we don't need Neutrino-specific caching (this would help speed up the cold run cases too).

@eliperelman
Copy link
Member Author

Didn't even know about the compile cache package, that's pretty neat.

@eliperelman
Copy link
Member Author

@edmorley
Copy link
Member

edmorley commented Sep 1, 2018

So I've been digging into this a bit.

For Treeherder's WIP Neutrino 9 .neutrinorc.js (which uses @neutrinojs/airbnb, @neutrinojs/react, @neutrinojs/copy and a custom (neutrino) =>{} block), running ESLint pointing at the Neutrino-generated .eslintrc.js took 2400ms longer than if the .eslintrc.js was converted to a fixed object export containing the same options.

This is pretty awful, and something we really need to fix.

I tried v8-compile-cache (which ESLint's CLI doesn't already use, unlike webpack-cli), and that reduces the time by 300-350ms, which is a start but still a long way to go.

I then used time-require, which showed that virtually all of the time is taken up with the many require()s - and in the case of the .eslintrc() output handler most of the imported files aren't even used (for example the webpack plugins). Just to emphasis the difference this makes, after commenting out all presets but @neutrinojs/airbnb, the overhead dropped from 2400ms to 250ms.

As such, I think a big win would be to lazy-require the plugins, that way for use-cases that don't generate a full webpack config (eg linting, jest, mocha) no time is wasted importing the plugins, and even for cases where a webpack config is generated, we would avoid require()ing plugins that were not applicable to that environment (eg we'd skip the require of clean/mini-css-extract/... plugins in development).

To achieve this, I was thinking we could modify webpack-chain, so that if the plugin was passed as a string, then webpack-chain would require() it during the .toConfig(). Then the Neutrino presets would just pass the string from require.resolve(...). Though not sure how this would work for the plugins that aren't the default export (eg const { EnvironmentPlugin } = require('webpack')) - other than perhaps just requiring the plugin directly (eg require.resolve('webpack/lib/EnvironmentPlugin')).

@eliperelman - thoughts?

@edmorley
Copy link
Member

edmorley commented Sep 1, 2018

I guess this is another case where the inability of middleware to know what command is being run (since middleware accumulation occurs before calling the output handler) limits what we can do. If we were starting from scratch, it would seem that perhaps the middleware API would allow presets to have multiple phases/events, so accumulation, registering commands and actual execution were all separate (and sometimes optional) steps.

@eliperelman
Copy link
Member Author

@edmorley we could support the string require use case, and worst case scenario, a plugin could be defined that did:

plugin('alpha')
  .use((...args) => new require('alpha')(...args), [1, 2, 3])

@edmorley
Copy link
Member

To achieve this, I was thinking we could modify webpack-chain, so that if the plugin was passed as a string, then webpack-chain would require() it during the .toConfig()

I've opened neutrinojs/webpack-chain#102

@edmorley edmorley changed the title Cache middleware accumulation Improve the performance of config generation Sep 12, 2018
edmorley added a commit that referenced this issue Sep 13, 2018
Since we don't need all of `yargs`, just the smaller parsing component,
which pulls in fewer dependencies.

This reduces `time node -e "require('neutrino')([]);"` (ie the baseline
overhead from Neutrino before any middleware is used) on my machine
from 295ms to 185ms.

Refs #239.
edmorley added a commit that referenced this issue Sep 14, 2018
webpack-chain 4.11.0's `.plugin('abc').use(...)` now supports being
passed the path to a plugin, instead of the plugin itself. See:
neutrinojs/webpack-chain#102

This means we can avoid the expensive `require()` of plugins in cases
where the plugin isn't used - such as when:
* running ESLint's CLI with a Neutrino-generated `.eslintrc.js`
* running tests with Jest/Mocha (which don't perform a webpack build)
* the plugin isn't needed for that `NODE_ENV` (eg `clean-webpack-plugin`
  isn't used in development).

For example, this reduces `time node .eslintrc.js` for a React+AirBnb
project from 1700ms to 370ms - and for projects that use more of the
non-default core Neutrino presets, the improvement will be even more
noticeable.

As an added bonus, plugins specified by path also have their `require()`
statement generated automatically when using `toString()`, meaning
that the configuration output by `--inspect` no longer needs any
adjustments before it can be run.

ie this "just works" with the core presets:

```
$ echo "module.exports = $(yarn neutrino --inspect --mode production);" > exported-config.js
$ yarn webpack --config exported-config.js
```

The webpack-chain docs have also been synced with those from upstream.

Refs #239.
@eliperelman
Copy link
Member Author

Should we consider this fixed now?

@edmorley
Copy link
Member

Yeah config generation is now 5x faster for the eslintrc case, and further improvements would mean more invasive changes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

2 participants