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

Faster babel build #13423

Merged
merged 7 commits into from Jun 11, 2021
Merged

Faster babel build #13423

merged 7 commits into from Jun 11, 2021

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Jun 4, 2021

Q                       A
Fixed Issues? Improve performance of our own build process
License MIT

This PR replaces gulp-babel by a jest-worker powered Babel farm. On a 4C8U machine, the time spent on yarn gulp build-no-bundle has improved from 12 seconds to 8.5 seconds.

Note: I also tried the worker-thread based piscinia on this branch. However the worker_threads method is slower (16 seconds) than single thread, which is expected as the build process contains many filesystem IO.

@JLHwung JLHwung added the PR: Internal 🏠 A type of pull request used for our changelog categories label Jun 4, 2021
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 4, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9203e0f:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@babel-bot
Copy link
Collaborator

babel-bot commented Jun 4, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/46832/

@JLHwung JLHwung marked this pull request as ready for review June 4, 2021 19:46
@@ -0,0 +1,39 @@
const { transformSync } = require("@babel/core");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We deviates from the conventional Gulp stream approach because:

  1. Passing the source code to sub process is more expensive than passing the path
  2. Passing the transformed code to main process in order to written via gulp.dest is expensive and ultimately counters the performance gain from multiple process

So we end up creating our own gulp-babel + gulp-newer in this file.

@@ -517,7 +522,7 @@ gulp.task(
gulp.series("copy-dts", () => buildRollupDts(dtsBundles))
);

gulp.task("build-babel", () => buildBabel(/* exclude */ libBundles));
gulp.task("build-babel", () => buildBabel(true, /* exclude */ libBundles));
Copy link
Member

@hzoo hzoo Jun 9, 2021

Choose a reason for hiding this comment

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

guess rename this to ignore?

Suggested change
gulp.task("build-babel", () => buildBabel(true, /* exclude */ libBundles));
gulp.task("build-babel", () => buildBabel(true, /* ignore */ libBundles));

}
}
const srcStat = statSync(src);
return srcStat.mtimeMs > destStat.mtimeMs;
Copy link
Member

Choose a reason for hiding this comment

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

were you able to test watching/incremental, wondering how effective this is? wondering whether we can reuse chokidar here too or is that slow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can combine it with an incremental watch pipeline, so only changed files are compiled. I can land it in a separate PR.

The stat comparison here is essentially a replacement for gulp-newer so when we run gulp build-no-bundle when we already have lib/**, we don't re-compile the compiled sources.

Gulpfile.mjs Outdated
numWorkers,
exposedMethods: ["transform"],
});
worker.getStdout().on("data", chunk => process.stdout.write(chunk));
Copy link
Member

Choose a reason for hiding this comment

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

Does worker.getStdout().pipe(process.stdout) work?

@JLHwung
Copy link
Contributor Author

JLHwung commented Jun 9, 2021

Note: This PR is also considered a prototype of future @babel/cli: We can use jest-worker when compiling a directory or many files.

function buildBabel(exclude) {
const base = monorepoRoot;
function createWorker(useWorker) {
const numWorkers = require("os").cpus().length / 2 - 1;
Copy link
Member

Choose a reason for hiding this comment

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

I imagine we can/might want to make this configurable at some point? (Like the --maxWorkers flag in jest)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Currently the default value is assuming the CPU has hyper-threading so it havles and minus the main thread. I tested different values locally, based on our scenario, increasing the numWorkers more than that does not yield faster performance.

Gulpfile.mjs Outdated Show resolved Hide resolved
Co-authored-by: Brian Ng <bng412@gmail.com>
@JLHwung JLHwung merged commit 69f423b into babel:main Jun 11, 2021
@JLHwung JLHwung deleted the faster-babel-build branch June 11, 2021 18:50
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Sep 11, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants