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

🏗🚀 Speed up pre-closure babel transforms #27426

Merged
merged 1 commit into from Mar 26, 2020
Merged

🏗🚀 Speed up pre-closure babel transforms #27426

merged 1 commit into from Mar 26, 2020

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Mar 26, 2020

Background:

  • AMP's minified build (gulp dist) and type checker (gulp check-types) run a set of babel transforms on source files before invoking closure compiler.
  • Today, transforms are run on all source files at once, after which the results are written to disk in a temp directory. Following this, closure compiler is invoked on the temp directory.
  • This is particularly inefficient for flags like --core_runtime_only and --noextensions, which only compile a small subset of source files.

Screen Shot 2020-03-26 at 12 28 24 AM

This PR does the following:

  • Refactors pre-closure babel transforms to run directly on the gulp.src stream
  • Moves all transform code to build-system/compile/pre-closure-babel.js
  • Implements a caching mechanism so every source file is transformed at most once
  • Deletes transferSrcsToTempDir() and all associated code
  • Ensures that only those source files that need to be compiled are transformed

Performance improvements (Macbook Pro):

Command Before After
gulp dist --core_runtime_only ~50s ~30s
gulp check-types ~1m 40s ~1m 10s
gulp dist --extensions_from examples/article.amp.html ~2m ~1m 40s
gulp dist ~7m ~6m 30s
gulp dist --single_pass ~5m ~5m

Coming up:

Addresses #26779 (comment)

@rsimha
Copy link
Contributor Author

rsimha commented Mar 26, 2020

@erwinmombay @kristoferbaxter Extensively tested this locally, and it's now ready for a review. This will pave the way for minified lazy builds, and the pattern could potentially be adopted for targeted integration tests (--files) as well (they currently do an all-at-once transform).

@rsimha rsimha merged commit 2fadaff into ampproject:master Mar 26, 2020
@rsimha rsimha deleted the 2020-03-25-StreamBabelTransforms branch March 26, 2020 22:08
@samouri
Copy link
Member

samouri commented Mar 26, 2020

Awesome PR. Thank you for speeding up builds! ⭐️

build-system/compile/pre-closure-babel.js Show resolved Hide resolved
build-system/compile/pre-closure-babel.js Show resolved Hide resolved
@@ -308,10 +306,17 @@ exports.getGraph = function(entryModules, config) {
});

config.babel = config.babel || {};
const babelPlugins = conf
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we prioritize #27161? We're adding more and more call-site configurations that will make it difficult to synchronize changes across environments.

Copy link
Contributor Author

@rsimha rsimha Mar 26, 2020

Choose a reason for hiding this comment

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

I believe @rcebulko is already working on this. (A fair bit has changed in recent days, so that PR will need a full rebase before proceeding. Shouldn't be bad though.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can/will do 👍

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

Successfully merging this pull request may close these issues.

None yet

7 participants