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.

Sign up for GitHub

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

Jump to bottom

Significantly speed up gulp dist #10277

Closed
rsimha opened this issue Jul 6, 2017 · 22 comments · Fixed by #15256 or #21939
Closed

Significantly speed up gulp dist #10277

rsimha opened this issue Jul 6, 2017 · 22 comments · Fixed by #15256 or #21939
Assignees
@erwinmombay @rsimha
Labels
P1: High Priority Type: Bug WG: infra
Projects
Build Size
Milestone
Backlog Bugs

Comments

@rsimha
Copy link
Contributor

rsimha commented Jul 6, 2017

The build command we run on Travis before running integration tests is node_modules/gulp/bin/gulp.js dist --fortesting. This currently takes more than 13 minutes on Travis, while it takes only ~3 minutes on my linux workstation and macbook pro.

This issue is to track the compiler work that's being done to speed up the build. Doing so will enable us to turn on integration testing for all PRs, and help keep master green while not overwhelming the Travis build queue.
@rsimha rsimha added this to the Backlog Bugs milestone Jul 6, 2017
@rsimha
Copy link
Contributor Author

rsimha commented Jul 6, 2017

/cc @erwinmombay @choumx @cramforce

@rsimha
Copy link
Contributor Author

rsimha commented Jul 6, 2017

Note to self: given the huge difference in the build times on Travis vs. local machines, it might be worth doing some fine grained profiling of the build to see if there are any steps in particular that take significantly longer on Travis.

@rsimha rsimha mentioned this issue Jul 6, 2017
Merged
@rsimha
Copy link
Contributor Author

rsimha commented Jul 7, 2017

Next step: Enable build logging on Travis and see if there's any step that takes a particularly long time.

@rsimha rsimha mentioned this issue Jul 10, 2017
Closed
@rsimha
Copy link
Contributor Author

rsimha commented Jul 10, 2017

@erwinmombay @cramforce @choumx: I did a fine grained build time measurement of gulp build and gulp dist on Travis, and while build is nearly as fast on Travis as it is on a workstation, dist is significantly slower, most likely due to the lack of multi-core parallelism on the Travis VMs.

@rsimha
Copy link
Contributor Author

rsimha commented Aug 4, 2017

Good news. With #10785 being fixed, we now run integration and unit tests on Travis for every PR that touches the runtime, and the entire cycle takes 25 minutes CPU time / 13 mins wall clock time.
See https://travis-ci.org/ampproject/amphtml/builds/261173604

We still run gulp dist on master, so I'm leaving this bug open to make some gains there. I believe @erwinmombay has been working on some changes to speed up the closure compiler.

@rsimha
Copy link
Contributor Author

rsimha commented Sep 11, 2017

The good news: We now use a new closure compiler. (Issue: #11229 Fix: #10791)
The bad news: The time taken for a dist on Travis has risen from ~14 min to ~17 min over the recent past.

Leaving this issue open, in search of a fix.

@rsimha
Copy link
Contributor Author

rsimha commented Sep 11, 2017

@erwinmombay, I remember you mentioning that we can speed up dist by not invoking the closure compiler once per compilation unit (for some definition of "unit"), but instead, invoke it once across all units, to the extent possible. (https://github.com/google/closure-compiler#compiling-multiple-scripts)

Is there an issue tracking this? Is it worth looking into afresh this quarter?

/cc @cramforce @choumx

@erwinmombay
Copy link
Member

yes i am looking into this bug:

screen shot 2017-09-11 at 2 51 57 pm

where splittable blows up on a dependency that does not export anything

reduction:
https://github.com/erwinmombay/split

@cramforce
Copy link
Member

Two ways to fix:

  • use require (maybe)
  • Just export something. Anything, really.

This is a bug in CC, of course.

@ampprojectbot
Copy link
Member

This issue hasn't been updated in awhile. @erwinmombay Do you have any updates?

@rsimha rsimha mentioned this issue Mar 13, 2018
Closed
@rsimha
Copy link
Contributor Author

rsimha commented Mar 13, 2018

@erwinmombay Any updates on the work you were doing to speed up gulp dist? It's now taking ~22 minutes, and is timing out on Travis. See #13954

This was referenced May 14, 2018
Merged
Closed
@dreamofabear
Copy link

Accidental close.

@dreamofabear dreamofabear reopened this May 15, 2018
@rsimha
Copy link
Contributor Author

rsimha commented May 15, 2018 •
edited

Sheesh, Required in order to fix closed this :/

@rsimha rsimha added this to To do in Infrastructure Jun 13, 2018
@erwinmombay erwinmombay changed the title Significantly speed up gulp dist on Travis Significantly speed up gulp dist on Travis by doing a single pass of closure compiler Jul 2, 2018
@erwinmombay erwinmombay removed this from To do in Infrastructure Jul 2, 2018
@erwinmombay erwinmombay added this to In progress in Build Size Jul 2, 2018
@prateekbh
Copy link
Member

@erwinmombay before shipping this, we'll need to do the effort of copying over v0.js to v0-module.js and replacing this->self hack. This can be left with a TODO on my name to be removed once we actually start emitting out ES2015 bundles

@ampprojectbot
Copy link
Member

This is a high priority issue but it hasn't been updated in awhile. @erwinmombay Do you have any updates?

5 similar comments
@ampprojectbot
Copy link
Member

This is a high priority issue but it hasn't been updated in awhile. @erwinmombay Do you have any updates?

@ampprojectbot
Copy link
Member

This is a high priority issue but it hasn't been updated in awhile. @erwinmombay Do you have any updates?

@ampprojectbot
Copy link
Member

This is a high priority issue but it hasn't been updated in awhile. @erwinmombay Do you have any updates?

@ampprojectbot
Copy link
Member

This is a high priority issue but it hasn't been updated in awhile. @erwinmombay Do you have any updates?

@ampprojectbot
Copy link
Member

This is a high priority issue but it hasn't been updated in awhile. @erwinmombay Do you have any updates?

@rsimha rsimha changed the title Significantly speed up gulp dist on Travis by doing a single pass of closure compiler Significantly speed up gulp dist Apr 23, 2019
@rsimha
Copy link
Contributor Author

rsimha commented Apr 23, 2019 •
edited

Dusting off this really old (and still important) issue. (Today, gulp dist takes ~30 minutes on Travis.)

I have a new approach under development, which switches to a single invocation of closure compiler, but still does multiple passes. This is enabled by https://github.com/facebook/nailgun.

Stay tuned for a PR and new numbers.

/cc @erwinmombay @cramforce @jridgewell @choumx

@rsimha rsimha mentioned this issue Apr 23, 2019
Merged
Build Size automation moved this from In progress to Done Apr 23, 2019
@rsimha
Copy link
Contributor Author

rsimha commented Apr 23, 2019

New numbers look good: #21939 (comment)

Happy to have finally closed this issue 😃

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

@erwinmombay erwinmombay

@rsimha rsimha

Labels
P1: High Priority Type: Bug WG: infra
Projects
Build Size
  
Done
Milestone
Backlog Bugs
6 participants
@cramforce @erwinmombay @prateekbh @dreamofabear @ampprojectbot @rsimha