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 next caching #162

Merged
merged 21 commits into from
Dec 20, 2018
Merged

Webpack next caching #162

merged 21 commits into from
Dec 20, 2018

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Dec 16, 2018

Implements the "next" webpack branch caching for #46.

Here are the stats for the self build:

  • Without cache: 31s
  • With cache: First build 32s, Subsequent builds 15s

And in fact the remaining time there is actually mostly minification, so it should be possible to get this down to of the order of seconds pretty easily if we go with the approach in #115.

The Webpack upgrade was mostly pretty smooth. There was an issue with the parser injection for the top-level return handling, which was tricky to fix as there seemed to be a different instance of the parser in use in Webpack to the one we could hook into.

Instead I have disabled that test for now, and also posted a Webpack bug and PR for the root cause at webpack/webpack#8509 and webpack/webpack#8510.

Cache Handling

I couldn't work out how to manage the cache with a custom persistence layer here, so Webpack is entirely owning the file system side of things here.

There is currently no cache eviction, this seems like it will be an area of development in Webpack 5 to handle evicting old items as the cache reaches a certain size based on usage, otherwise it it at risk of eventually "taking over" the file system entirely :P

The other thing that was mentioned in the PR thread was handling "relocations" of the cache. Eg if I have app1/node_modules/dep1 and another app has the same app2/node_modules/dep1 there is a lot of duplication going on. Again, this is something that would likely need to happen on the Webpack core side to better handle these types of redundancies. Perhaps this relocation is more of a problem for ncc maintaining its own cache, as webpack actually defaults to node_modules/.cache/webpack for a per-project cache, while the ncc cache is across-projects.

The final thing to watch out for will of course be invalidation issues, which will likely be the most important thing to test for releasing this.

API

The API gets a cache option which is the string directory to use for the cache defaulting to os.tmpdir/ncc-cache. The cache can be disabled with cache: false (it is on by default).

CLI

The CLI commands are ncc cache clean, ncc cache dir and ncc cache size.

It also comes with a new --no-cache flag.

@guybedford
Copy link
Contributor Author

CI failure here is due to Javascript heap out of memory :(

I've tried bumping the heap size.

@rauchg
Copy link
Member

rauchg commented Dec 16, 2018

Seems like it could be related to a memory leak in Webpack 5. I would refrain from bumping it further, as we shouldn't see such increased memory use relative to the webpack 4 branch

@guybedford
Copy link
Contributor Author

So it looks like this is actually only happening when running the tests with coverage (as is done on CI).

Seems like it might be jestjs/jest#5837 then.

@codecov-io
Copy link

codecov-io commented Dec 17, 2018

Codecov Report

Merging #162 into master will decrease coverage by 9.94%.
The diff coverage is 25.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #162      +/-   ##
==========================================
- Coverage   77.29%   67.35%   -9.95%     
==========================================
  Files          11       12       +1     
  Lines         511      579      +68     
==========================================
- Hits          395      390       -5     
- Misses        116      189      +73
Impacted Files Coverage Δ
src/cli.js 0% <0%> (ø) ⬆️
src/loaders/node-loader.js 100% <100%> (ø) ⬆️
src/loaders/relocate-loader.js 93.02% <100%> (+0.24%) ⬆️
src/utils/ncc-cache-dir.js 100% <100%> (ø)
src/index.js 68.8% <45.31%> (-24.68%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6aa2c4c...99b03ca. Read the comment docs.

@guybedford
Copy link
Contributor Author

Ok, finally got this fixed with a manual GC run. Turns out the bump before would have fixed it to, it was just done on the wrong test.

For the coverage - because we don't have coverage on the cli.js file, adding anything to this code decreases the overall percentage considerably. I can try and expose an API for the cli.js internals so we can call that programatically and get this back up. Created #163.

@guybedford guybedford force-pushed the webpack-next-cache branch 3 times, most recently from 34ad8c3 to 9911d86 Compare December 17, 2018 15:08
{
// enabling cache stops saslprep.js test from working
// which seems like a Webpack 5 cache bug...
cache: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The saslprep.js test was failing until I disabled the cache explicitly. The failure was that the asset wasn't being emitted.

When the saslprep.js test is run on its own in isolation it passes fine, regardless of cache state, so it seems like some kind of interaction between webpack cache and previous build memoization.

Because it works in isolation fine, and this is the only test that fails, I don't think this is as bad as it sounds, but it's worth bearing in mind.

@guybedford
Copy link
Contributor Author

The remaining error here is that switching from the "src" to the "dist" ncc build results in Webpack assigning different ids to the modules so the unit test output tests fail!

For now, I've just commited the two versions as I have no idea how to trace down why there is a difference between the two in Webpack 5 only...

@guybedford guybedford force-pushed the webpack-next-cache branch 2 times, most recently from 41d481a to 99b03ca Compare December 18, 2018 16:42
@guybedford
Copy link
Contributor Author

I've updated this branch to include a watcher for both the API and CLI through -w. This gets the rebuilds to the one second mark.

@sokra sokra force-pushed the webpack-next-cache branch from 99b03ca to 2a1a02d Compare December 19, 2018 23:14
@rauchg rauchg merged commit c2fb87e into master Dec 20, 2018
@rauchg rauchg deleted the webpack-next-cache branch December 20, 2018 03:20
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.

4 participants