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

Build: Move the vendor DLL to a commons chunk #11771

Conversation

blowery
Copy link
Contributor

@blowery blowery commented Mar 4, 2017

This PR gives us baked in hashes for the vendor bundle and a much simpler configuration. It also ends up invoking webpack once instead of twice and seems reasonably speedy (34s build on my laptop).

Note, this is currently stacked on top of #11437.

After all the effort to move to a DLL, you may be wondering why I'd pull it back out.

There were two goals with the DLL work: 1) Speed up builds and rebuilds and 2) give us a more stable chunk to load.

  1. turned out to be a red herring. Subsequent investigation has shown that rebuild times are mostly dictated by the number of chunks the edited module is part of. Switching to a DLL didn't really have much effect on rebuild speed.

  2. It did help out, but since, I've learned a lot more about how to make stable chunks. A stable chunk is one whose hash only changes when the contents of the chunk change.

Before all this work, we had two major problems. The build chunk hash was changing on every commit, even if nothing appeared to have changed within it, and when the build chunk changed, the vendor chunk hash would change as well.

We tried to fix this by moving vendor to a DLL, which isolated it and we started using a CommonsChunkPlugin to extract the manifest from the build chunk. Removing the manifest was the real key; once we started doing that, changes to child chunks no longer made the hash on the build chunk change.

However, this introduced a bug. We used the OccurrenceOrderPlugin to try to make module IDs more stable, but it also means that when we add new code, module IDs can shift a bit. If you look at the contents of the manifest, you'll find that it mostly contains the webpack loader and the mapping from chunk ID to URL. It does not contain a mapping of module IDs in any way. The bug is that the hash that webpack calculates for the chunk no longer seems to change when the module IDs change after removing the manifest.

See webpack/webpack#1856 for more discussion on this apparent bug.

#11437 seeks to work around the webpack bug by using a different strategy for calculating module IDs. Instead of calculating them based on occurrence order, which is unstable, it calculates them based on a hash of the require path, which is stable, but may have collisions.

But that also means that our motivation for having a vendor dll is misguided. We can simplify things by pulling it back into the main webpack configuration. This gives us a hash baked into the filename and the ability to use tools like webpack-bundle-analyzer more easily with Calypso. It will also let us more easily tweak the contents of the vendor bundle as needed.

This gives us baked in hashes for the vendor bundle and a much simpler configuration. It also ends up invoking webpack once instead of twice and seems reasonably speedy.
@matticbot
Copy link
Contributor

@matticbot matticbot added the [Size] M Medium sized issue label Mar 4, 2017
@blowery blowery requested review from ockham, mtias, gwwar, aduth and ehg March 4, 2017 14:39
@blowery blowery added Build [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 4, 2017
@blowery blowery changed the title Remove the vendor bundle and switch to a vendor commons Build: Move the vendor DLL to a commons chunk Mar 4, 2017
@blowery
Copy link
Contributor Author

blowery commented Mar 4, 2017

Also, this appears to work quite well with the webpack/persistent-caching feature turned on. If we could find a way to persist that cache for Docker builds, it appears we could cut the calypso build time for deploys down to 5~10s in many cases. /cc @samouri

@blowery blowery requested a review from samouri March 4, 2017 15:01
@ehg
Copy link
Member

ehg commented Mar 7, 2017

Also, this appears to work quite well with the webpack/persistent-caching feature turned on. If we could find a way to persist that cache for Docker builds, it appears we could cut the calypso build time for deploys down to 5~10s in many cases.

Just a drive by, before a full pass. I'm not sure whether using persistent caching in production is a good idea :]

But we could experiment with some envs (wpcalypso). I'm hoping we could get some Docker data volumes attached. From memory there are some other things we'd like to access locally, like the i18n translations for SSR.

@ehg
Copy link
Member

ehg commented Mar 7, 2017

Have we considered using records to keep the module IDs persistent? Now that we're talking about external volumes.

webpack/persistent-caching actually uses this.

@blowery
Copy link
Contributor Author

blowery commented Mar 7, 2017

I'm not sure whether using persistent caching in production is a good idea :]

I'm not either :D

But we could experiment with some envs (wpcalypso). I'm hoping we could get some Docker data volumes attached. From memory there are some other things we'd like to access locally, like the i18n translations for SSR.

We'd probably have to spin up a parallel build process to try that out, now that #11352 has landed.

Having data volumes seems orthogonal to keeping build state around between builds. Or am I misunderstanding the need?

@blowery
Copy link
Contributor Author

blowery commented Mar 7, 2017

Have we considered using records to keep the module IDs persistent? Now that we're talking about external volumes.

Yeah, I considered it. It does mean that we need to start keeping state around between builds in a consistent way (we already do afaik to some degree, as we can bypass the npm install most of the time) and introduces some interesting complications. What happens when we lose the state file? Or when the modules in the state file are no longer part of the build? I assume the state file would just keep growing forever as we move through versions of dependencies?

Having a stable way to map from path to module ID seems simpler to me, though it has it's own complication, hash collisions. Having record support could be nice though, though I'd like to see it as a build optimization, not a requirement for running a build.

@blowery
Copy link
Contributor Author

blowery commented Mar 7, 2017

rolled into #11867

@blowery blowery closed this Mar 7, 2017
@blowery blowery deleted the try/webpack/hashed-module-ids-with-no-dll branch March 7, 2017 22:09
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build [Size] M Medium sized issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants