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鈥檒l 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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 6 additions & 18 deletions build-system/compile/compile.js
Expand Up @@ -31,9 +31,10 @@ const {
} = require('./closure-compile');
const {checkForUnknownDeps} = require('./check-for-unknown-deps');
const {checkTypesNailgunPort, distNailgunPort} = require('../tasks/nailgun');
const {CLOSURE_SRC_GLOBS, SRC_TEMP_DIR} = require('./sources');
const {CLOSURE_SRC_GLOBS} = require('./sources');
const {isTravisBuild} = require('../common/travis');
const {postClosureBabel} = require('./post-closure-babel');
const {preClosureBabel} = require('./pre-closure-babel');
const {singlePassCompile} = require('./single-pass');
const {VERSION: internalRuntimeVersion} = require('./internal-version');

Expand All @@ -46,17 +47,6 @@ let inProgress = 0;
// See https://github.com/google/closure-compiler-npm/issues/9
const MAX_PARALLEL_CLOSURE_INVOCATIONS = isTravisBuild() ? 4 : 1;

/**
* Prefixes the tmp directory if we need to shadow files that have been
* preprocessed by babel in the `dist` task.
*
* @param {!Array<string>} paths
* @return {!Array<string>}
*/
function convertPathsToTmpRoot(paths) {
return paths.map(path => path.replace(/^(!?)(.*)$/, `$1${SRC_TEMP_DIR}/$2`));
}

// Compiles AMP with the closure compiler. This is intended only for
// production use. During development we intend to continue using
// babel, as it has much faster incremental compilation.
Expand Down Expand Up @@ -358,8 +348,6 @@ function compile(
delete compilerOptions.define;
}

compilerOptions.js_module_root.push(SRC_TEMP_DIR);
rsimha marked this conversation as resolved.
Show resolved Hide resolved

const compilerOptionsArray = [];
Object.keys(compilerOptions).forEach(function(option) {
const value = compilerOptions[option];
Expand All @@ -376,11 +364,10 @@ function compile(
}
});

const gulpSrcs = convertPathsToTmpRoot(srcs);

if (options.typeCheckOnly) {
return gulp
.src(gulpSrcs, {base: SRC_TEMP_DIR})
.src(srcs, {base: '.'})
.pipe(preClosureBabel())
.pipe(sourcemaps.init({loadMaps: true}))
.pipe(gulpClosureCompile(compilerOptionsArray, checkTypesNailgunPort))
.on('error', err => {
Expand All @@ -392,7 +379,8 @@ function compile(
} else {
timeInfo.startTime = Date.now();
return gulp
.src(gulpSrcs, {base: SRC_TEMP_DIR})
.src(srcs, {base: '.'})
.pipe(preClosureBabel())
.pipe(sourcemaps.init({loadMaps: true}))
.pipe(gulpClosureCompile(compilerOptionsArray, distNailgunPort))
.on('error', err => {
Expand Down
88 changes: 88 additions & 0 deletions build-system/compile/pre-closure-babel.js
@@ -0,0 +1,88 @@
/**
* Copyright 2020 The AMP HTML Authors. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS-IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
'use strict';

const argv = require('minimist')(process.argv.slice(2));
const babel = require('@babel/core');
const conf = require('./build.conf');
const globby = require('globby');
const path = require('path');
const through = require('through2');
const {BABEL_SRC_GLOBS, THIRD_PARTY_TRANSFORM_GLOBS} = require('./sources');

const ROOT_DIR = path.resolve(__dirname, '../../');

/**
* Files on which to run pre-closure babel transforms.
*
* @private @const {!Array<string>}
*/
const filesToTransform = getFilesToTransform();

/**
* Used to cache babel transforms.
*
* @private @const {!Object<string, string>}
*/
const cachedTransforms = {};

/**
* Computes the set of files on which to run pre-closure babel transforms.
*
* @return {!Array<string>}
*/
function getFilesToTransform() {
return globby
.sync([...BABEL_SRC_GLOBS, '!node_modules/', '!third_party/'])
.concat(globby.sync(THIRD_PARTY_TRANSFORM_GLOBS));
}

/**
* Apply babel transforms prior to closure compiler pass.
*
* When a source file is transformed for the first time, it is written to an
* in-memory cache from where it is retrieved every subsequent time without
* invoking babel.
*
* @return {!Promise}
*/
function preClosureBabel() {
return through.obj((file, enc, next) => {
const cachedTransform = cachedTransforms[file.path];
if (cachedTransform) {
file.contents = Buffer.from(cachedTransform);
} else if (filesToTransform.includes(path.relative(ROOT_DIR, file.path))) {
const babelPlugins = conf.plugins({
isForTesting: !!argv.fortesting,
isEsmBuild: !!argv.esm,
isSinglePass: !!argv.single_pass,
isChecktypes: argv._.includes('check-types'),
});
const {code} = babel.transformFileSync(file.path, {
rsimha marked this conversation as resolved.
Show resolved Hide resolved
plugins: babelPlugins,
retainLines: true,
compact: false,
});
cachedTransforms[file.path] = code;
file.contents = Buffer.from(code);
rsimha marked this conversation as resolved.
Show resolved Hide resolved
}
return next(null, file);
});
}

module.exports = {
preClosureBabel,
};
26 changes: 16 additions & 10 deletions build-system/compile/single-pass.js
Expand Up @@ -45,10 +45,10 @@ const {
handleSinglePassCompilerError,
} = require('./closure-compile');
const {checkForUnknownDeps} = require('./check-for-unknown-deps');
const {preClosureBabel} = require('./pre-closure-babel');
const {TopologicalSort} = require('topological-sort');
const TYPES_VALUES = Object.keys(TYPES).map(x => TYPES[x]);
const wrappers = require('./compile-wrappers');
const {SRC_TEMP_DIR} = require('../compile/sources');
const {VERSION: internalRuntimeVersion} = require('./internal-version');

const argv = minimist(process.argv.slice(2));
Expand Down Expand Up @@ -164,7 +164,7 @@ exports.getBundleFlags = function(g) {
.forEach(function(pkg) {
g.bundles[mainBundle].modules.push(pkg);
fs.outputFileSync(
`${g.tmp}/${pkg}`,
pkg,
JSON.stringify(JSON.parse(readFile(pkg)), null, 4)
);
});
Expand All @@ -185,7 +185,7 @@ exports.getBundleFlags = function(g) {
// TODO(erwinm): This access will break
const bundle = g.bundles[originalName];
bundle.modules.forEach(function(js) {
srcs.push(`${g.tmp}/${js}`);
srcs.push(js);
});
let name;
let info = extensionsInfo[bundle.name];
Expand Down Expand Up @@ -265,7 +265,6 @@ exports.getBundleFlags = function(g) {
throw new Error('Expect to build more than one bundle.');
}
});
flagsArray.push('--js_module_root', `${g.tmp}/`);
return flagsArray;
};

Expand Down Expand Up @@ -296,7 +295,6 @@ exports.getGraph = function(entryModules, config) {
},
},
packages: {},
tmp: SRC_TEMP_DIR,
};

TYPES_VALUES.forEach(type => {
Expand All @@ -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 馃憤

.plugins({
isEsmBuild: !!argv.esm,
isSinglePass: true,
isForTesting: !!argv.fortesting,
})
.concat(['transform-es2015-modules-commonjs']);

// Use browserify with babel to learn about deps.
const b = browserify(entryModules, {
basedir: SRC_TEMP_DIR,
basedir: '.',
browserField: 'module',
debug: true,
deps: true,
Expand All @@ -323,7 +328,7 @@ exports.getGraph = function(entryModules, config) {
.transform(babelify, {
compact: false,
cwd: process.cwd(),
plugins: ['transform-es2015-modules-commonjs'],
plugins: babelPlugins,
});
// This gets us the actual deps. We collect them in an array, so
// we can sort them prior to building the dep tree. Otherwise the tree
Expand All @@ -346,13 +351,13 @@ exports.getGraph = function(entryModules, config) {
})
.forEach(function(row) {
const id = unifyPath(
exports.maybeAddDotJs(path.relative(SRC_TEMP_DIR, row.id))
exports.maybeAddDotJs(path.relative('.', row.id))
);
topo.addNode(id, id);
const deps = Object.keys(row.deps)
.sort()
.map(dep => {
dep = unifyPath(path.relative(SRC_TEMP_DIR, row.deps[dep]));
dep = unifyPath(path.relative('.', row.deps[dep]));
if (dep.startsWith('node_modules/')) {
const pkgJson = pkgUp.sync({cwd: path.dirname(dep)});
const jsonId = unifyPath(path.relative(process.cwd(), pkgJson));
Expand Down Expand Up @@ -680,7 +685,8 @@ function compile(flagsArray) {
// TODO(@cramforce): Run the post processing step
return new Promise(function(resolve, reject) {
gulp
.src(srcs, {base: SRC_TEMP_DIR})
.src(srcs, {base: '.'})
.pipe(preClosureBabel())
.pipe(sourcemaps.init({loadMaps: true}))
.pipe(gulpClosureCompile(flagsArray))
.on('error', err => {
Expand Down
5 changes: 0 additions & 5 deletions build-system/compile/sources.js
Expand Up @@ -20,10 +20,6 @@
* for both the babel and closure sources to be as close as possible.
*/

const tempy = require('tempy');

const SRC_TEMP_DIR = tempy.directory();

const COMMON_GLOBS = [
'third_party/amp-toolbox-cache-url/**/*.js',
'third_party/caja/html-sanitizer.js',
Expand Down Expand Up @@ -154,6 +150,5 @@ const THIRD_PARTY_TRANSFORM_GLOBS = [
module.exports = {
BABEL_SRC_GLOBS,
CLOSURE_SRC_GLOBS,
SRC_TEMP_DIR,
THIRD_PARTY_TRANSFORM_GLOBS,
};
3 changes: 0 additions & 3 deletions build-system/tasks/check-types.js
Expand Up @@ -14,7 +14,6 @@
* limitations under the License.
*/

const argv = require('minimist')(process.argv.slice(2));
const log = require('fancy-log');
const {
checkTypesNailgunPort,
Expand All @@ -29,7 +28,6 @@ const {cleanupBuildDir, closureCompile} = require('../compile/compile');
const {compileCss} = require('./css');
const {extensions, maybeInitializeExtensions} = require('./extension-helpers');
const {maybeUpdatePackages} = require('./update-packages');
const {transferSrcsToTempDir} = require('./helpers');

/**
* Dedicated type check path.
Expand All @@ -41,7 +39,6 @@ async function checkTypes() {
process.env.NODE_ENV = 'production';
cleanupBuildDir();
maybeInitializeExtensions();
transferSrcsToTempDir({isChecktypes: true, isEsmBuild: argv.esm || false});
const compileSrcs = [
'./src/amp.js',
'./src/amp-shadow.js',
Expand Down
7 changes: 0 additions & 7 deletions build-system/tasks/dist.js
Expand Up @@ -31,7 +31,6 @@ const {
printConfigHelp,
printNobuildHelp,
toPromise,
transferSrcsToTempDir,
} = require('./helpers');
const {
createCtrlcHandler,
Expand Down Expand Up @@ -102,12 +101,6 @@ async function dist() {
await compileCss();
await compileJison();

transferSrcsToTempDir({
isForTesting: !!argv.fortesting,
isEsmBuild: !!argv.esm,
isSinglePass: !!argv.single_pass,
});

await copyCss();
await copyParsers();
await bootstrapThirdPartyFrames(/* watch */ false, /* minify */ true);
Expand Down