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

Modifying generation script to correctly import and use the modules that export numbers. #8

Merged
merged 1 commit into from Oct 30, 2016

Conversation

Projects
None yet
2 participants
@aickin
Contributor

aickin commented Oct 25, 2016

I think I found a bug in the generation script for ES6 modules that affects the results in a real way.

As written, lib/es6-{num}/index.js gets written out like this:

var total = 0
import module_0 from './module_0'
total += 0
import module_1 from './module_1'
total += 1
/* and so on... */

This means that index.js never actually uses the value exported from the child modules. All those imports just get thrown away by rollup. I've changed the generation script so that it generates the following:

var total = 0
import module_0 from './module_0'
total += module_0
import module_1 from './module_1'
total += module_1
/* and so on... */

On my local runs of the benchmark, this change more than doubles the amount of time that rollup takes overall. (Closure is smart enough to inline the vars, btw.)

Rollup is still faster than webpack, even with optimize-js patched for webpack bundles, but the comparison is a heckuva lot closer.

Thanks for all your work on this; you are great!

@nolanlawson

This comment has been minimized.

Show comment
Hide comment
@nolanlawson

nolanlawson Oct 29, 2016

Owner

Ugh, that is a huge oversight that I made. I'm probably going to need to re-run the tests and write a new blog post. Thank you for catching and fixing this! Serious 🥚 on my face with this one.

Owner

nolanlawson commented Oct 29, 2016

Ugh, that is a huge oversight that I made. I'm probably going to need to re-run the tests and write a new blog post. Thank you for catching and fixing this! Serious 🥚 on my face with this one.

@aickin

This comment has been minimized.

Show comment
Hide comment
@aickin

aickin Oct 29, 2016

Contributor

Don't be too hard on yourself; I myself have also made many, many similar mistakes in benchmarks. It's super easy to do.

Also, I want to say that it's so great that you posted the benchmark publicly so that this kind of review and double-checking can happen. And that I really appreciate your work, your Twitter feed, and your commitment to making the web better. Onwards!

Contributor

aickin commented Oct 29, 2016

Don't be too hard on yourself; I myself have also made many, many similar mistakes in benchmarks. It's super easy to do.

Also, I want to say that it's so great that you posted the benchmark publicly so that this kind of review and double-checking can happen. And that I really appreciate your work, your Twitter feed, and your commitment to making the web better. Onwards!

@nolanlawson

This comment has been minimized.

Show comment
Hide comment
@nolanlawson

nolanlawson Oct 29, 2016

Owner

I'm kinda amazed nobody else saw this. 😛 I personally didn't notice because I just assumed Rollup was inlining the vars.

Owner

nolanlawson commented Oct 29, 2016

I'm kinda amazed nobody else saw this. 😛 I personally didn't notice because I just assumed Rollup was inlining the vars.

@nolanlawson nolanlawson merged commit 10ec8e4 into nolanlawson:master Oct 30, 2016

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