Skip to content

Conversation

@aduth
Copy link
Member

@aduth aduth commented Apr 7, 2019

This pull request seeks to try a few optimizations for the default Webpack configuration. In my testing using time ./node_modules/.bin/wp-scripts build, the approximate result in my environment is a net build time decrease of ~38% (18.5s to 11.5s) uncached to ~50% (18.5s to 9.3s) cached.

The optimizations are three:

  • Use thread-loader to complement Babel compilation. On its own, with a default configuration, this seemed to reduce build time by approximately 2-3 seconds. The result may vary by machine depending on the number of cores available (default threading is numCPU - 1 workers).
  • Opt-in to babel-loader's cacheDirectory option. This too appeared to reduce build time by approximately 2-3 seconds further. By default, this uses a cache directory local to the project's node_modules. Thus, it will not have an immediate benefit on a fresh clone, or if node_modules is removed for any reason. I've included an option to direct this to a more persistent cache directory, which is partly self-serving for one of my own projects where I'd like to use a more persistent cache.
  • Exclude source-map-loader from production builds. This had a slightly more significant impact to reduce build time by a further 3-4 seconds. My reading of the source-map-loader documentation is that it is intended to complement the devtool option, which we already configure only for non-production environments.
    • All source map data is passed to webpack for processing as per a chosen source map style specified by the devtool option in webpack.config.js.

A few others I considered, but did not include:

  • Updating Webpack to the latest version. Webpack 5.x promises performance improvements, but is currently in alpha, and thus I did not consider stable enough to explore here, or at least not as part of this pull request. The latest minor/patch version combination actually appeared to increase build time.
  • A few other tips from Webpack's Build Performance guide which did not appear to have much impact positive or negative (output.pathinfo).

References:

Testing instructions:

You may consider to compare build times between master and this branch:

time ./node_modules/.bin/wp-scripts build

(Note: Babel loader's cacheDirectory won't have an impact the first time it's run. You can look to see whether a cache exists as node_modules/.cache/babel-loader)

Otherwise, verify there are no regressions in the expected output from build scripts (runs in browser, no sourcemap/devtool regression).

@aduth aduth added [Type] Build Tooling Issues or PRs related to build tooling [Type] Performance Related to performance efforts [Tool] WP Scripts /packages/scripts labels Apr 7, 2019
@aduth aduth requested review from gziolo and oandregal April 7, 2019 19:56
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

It looks like a set of good improvements to land. I will give it a spin later this week and compare performance. Thank you for working on it 👍

options: getBabelLoaderOptions(),
},
use: [
'thread-loader',
Copy link
Contributor

Choose a reason for hiding this comment

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

over on Calypso, we found that the optimal number wasn't CPU-1, but more like CPU / 2. MIght be worth experimenting with a bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

over on Calypso, we found that the optimal number wasn't CPU-1, but more like CPU / 2. MIght be worth experimenting with a bit.

Hey Ben, thanks for checking in. I didn't note it in the original comments, but I did tinker with this value slightly, though in my case I tried increasing it to the exact number of CPUs, and had observed a degraded benefit. I hadn't considered lowering it, but will give it a try.

Is there a pull request or issue which I could reference for that and/or other settings you'd explored?

Copy link
Contributor

Choose a reason for hiding this comment

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


// Provide a fallback configuration if there's not
// one explicitly available in the project.
...( ! hasBabelConfig() && {
Copy link
Member

Choose a reason for hiding this comment

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

I had never liked the options weren't colocated with the loader, so I welcome this change!

Copy link
Member

Choose a reason for hiding this comment

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

object destructuring magic :)

@oandregal
Copy link
Member

This is the result of my testing:

first time second & subsequent times
master ~35s ~20s
PR ~30s ~10s

I've also played with what Ben suggested (a static number of workers from 1 to 4 + a dynamic number as in calypso): it didn't render any improvement over the PR here.

Interestingly, this is how my cores behave (tested it several times and can repro consistently, in an Intel® Core™ i7-5600U CPU @ 2.60GHz × 4). In master all my cores reach the 100% at some point but they require a bit more time to be coordinated:

Screenshot from 2019-04-12 12-57-08-master

In this PR, they seem to work in a more coordinated fashion:

Screenshot from 2019-04-12 12-54-17-withthread

@gziolo gziolo force-pushed the try/optimize-webpack branch from 294c42e to ba48f76 Compare April 12, 2019 11:47
// WP_DEVTOOL global variable controls how source maps are generated.
// See: https://webpack.js.org/configuration/devtool/#devtool.
config.devtool = process.env.WP_DEVTOOL || 'source-map';
config.module.rules.unshift( {
Copy link
Member

Choose a reason for hiding this comment

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

It might be push as well. I don't know if order matters. Better safe than sorry :)

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I applied changes so it might need another look. I'm giving ✅ based on my sanity check testing and feedback shared by @nosolosw.

Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

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

We need to make sure thread-loader is looked up in @wordpress/scripts node_modules folder.

Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

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

OK, this is now ready. Commited a change to fix the issue I reported on my previous review.

Tested it by using it in an external plugin and it works. No webpack output difference detected. Sanity-checked that Gutenberg works fine.

Let's save everyone's seconds for good! Life is so precious, we shouldn't be waiting for Webpack.

@gziolo gziolo merged commit c830087 into master Apr 14, 2019
@gziolo gziolo deleted the try/optimize-webpack branch April 14, 2019 13:37
@gziolo gziolo added this to the 5.5 (Gutenberg) milestone Apr 14, 2019
mchowning pushed a commit to mchowning/gutenberg that referenced this pull request Apr 15, 2019
* Scripts: Use thread-loader in Babel compilation

* Scripts: Include Sourcemap Loader only in development environments

* Scripts: Use cache directory for Babel loader

* Fix package-lock.json to stop installing packages locally

* Tell webpack to look for thread-loader within the scripts node_modules folder
@aduth
Copy link
Member Author

aduth commented Apr 26, 2019

CHANGELOG notes should have been included here. I committed them directly to master in c8695f7 .

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

Labels

[Tool] WP Scripts /packages/scripts [Type] Build Tooling Issues or PRs related to build tooling [Type] Performance Related to performance efforts

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants