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

Switch to ES Modules with Rollup #5939

Closed
mourner opened this issue Jan 3, 2018 · 10 comments
Closed

Switch to ES Modules with Rollup #5939

mourner opened this issue Jan 3, 2018 · 10 comments

Comments

@mourner
Copy link
Member

mourner commented Jan 3, 2018

I've been looking into this today, and to sum up, I think we should do it.

Up till now, the main blocker for this has been our desire to keep time-to-first-render low. According to research in #3153 by @jfirebaugh, the amount of code evaluated on the worker side is the biggest factor in that, so we've been assuming we need to automatically create slim worker bundles from the main bundle, stripping out all the unnecessary code. So far, this has only been possible with Browserify because it keeps all the code in a dependency tree which we can traverse at runtime. There's no way to do that in Rollup — see rollup/rollup#1029.

Note that when we implemented slimming down the worker bundle (browserify/webworkify#30), the overall effect on TTFR was not that big — only ~200ms despite the bundle becoming ~2x smaller.

Today, I decided to take a small part of GL JS and convert it to Rollup to see how it would look — specifically, the expression code. When uglified/gzipped, the ES modules based build is approximately 10% smaller than the browserify-based one. Additionally, I wrapped the whole code with a simple console.time/timeEnd — the browserify-based build executed in 5.5ms in Node but the rollup one in just 1.2ms. Then I also remembered this article by Nolan Lawson that benchmarked the runtime cost of different bundlers (and showed Browserify having a huge hit compared to Rollup).

All this made me realize that maybe we should have looked at Browserify itself as the culprit of long evaluation times, and not just the size of the bundle. What if the switch from Browserify to ES Modules / Rollup reduces TTFR so dramatically that we no longer have to worry about worker bundle size much? Then, we could just make a custom rollup prelude that would launch the whole bundle as workers without traversing the deps at runtime.

My hunch is that we should take the plunge with ES Modules. The only big drawback I see is that we'll have to complicate the unit test setup (possibly relying on something like @std/esm), but advantages are great:

  • smaller bundle
  • possibly better time-to-first-render
  • modern code base without the awkward mix of ES-like flow type imports and CommonJS
  • more strict compile-time checking of dependencies

What do you think?

cc @jfirebaugh @anandthakker

@mourner mourner changed the title Switch to ES6 Modules with Rollup Switch to ES Modules with Rollup Jan 3, 2018
@jfirebaugh
Copy link
Contributor

If the worker overhead is not too big, this would be ideal. Is there any way to check that short of doing the entire conversion?

@mourner
Copy link
Member Author

mourner commented Jan 3, 2018

@jfirebaugh by my estimates, the evaluated worker bundle will be 40% bigger, but is likely to be fully offset by faster evaluation (due to linear structure without lots of closures).

@mourner mourner self-assigned this Jan 4, 2018
@mourner
Copy link
Member Author

mourner commented Jan 5, 2018

Started a branch to see how this would look, converted most of style-spec code so far: https://github.com/mapbox/mapbox-gl-js/compare/rollup

Seems pretty straightforward, and satisfying too. I also suspect that converting the whole codebase will lead to bigger savings than that small sample because of all the require interdependencies that will be eliminated by having just one reference to each used thing in one big closure.

@stereobooster
Copy link

You can also try to reuse flow type information to get more advanced minification with Closure compiler sindresorhus/project-ideas#79

@mourner
Copy link
Member Author

mourner commented Jan 5, 2018

@stereobooster interesting! How does Closure Compiler take advantage of types to make smaller builds?

@anandthakker
Copy link
Contributor

anandthakker commented Feb 17, 2018

(Finally) got a fully working proof of concept going up at https://github.com/mapbox/mapbox-gl-js/tree/rollup-anandthakker

Promising initial results:

master:

screen shot 2018-02-16 at 9 03 50 pm

682K Feb 16 21:03 mapbox-gl.js

rollup-anandthakker:
screen shot 2018-02-16 at 9 03 18 pm

601K Feb 16 20:59 mapboxgl-rollup.js // (minified)

Haven't yet measured the time to workers-ready yet, but will check that out soon

@anandthakker
Copy link
Contributor

Looks like the time-to-first-render is about 100ms worse for the rollup build, due to the significantly larger worker bundle taking longer to compile/evaluate:

branch compile script evaluate script
master 28ms 66ms
rollup-anandthakker 53ms 98ms

So it looks like our hopeful "what if"

What if the switch from Browserify to ES Modules / Rollup reduces TTFR so dramatically that we no longer have to worry about worker bundle size much?

didn't pan out.

Rollup recently got code splitting, so next I'm going to see if we can use that to get the best of both worlds -- indeed, thanks to rollups es-module-based tree shaking, this could mean an even smaller worker bundle than we had with webworkify.

@anandthakker
Copy link
Contributor

Update: using code splitting gets us back to parity with master:

  • compile script: 41ms
  • evaluate script: 70ms

Note that according to the performance tab, "compile script" and "evaluate script" happen in parallel, so this really is pretty close to master.

screen shot 2018-02-21 at 2 20 03 pm

@stereobooster
Copy link

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

No branches or pull requests

4 participants