Parallel babel transpilation #114

mikrostew · 2017-05-27T00:37:29Z

This modifies the options.resolveModuleSource and options.plugins APIs to be able to run babel transpilation in parallel using a worker pool. This API looks like:

['plugin-or-resolver-name', '/path/to/file-to-require.js', { /* options */ }]

That file that is the 2nd item in the array is required in the worker process. That file must define a function build() that returns the plugin or resolver to use. The build() function will be passed the options object that is the 3rd item from that array. Like so:

// file-to-require.js

module.exports = {
  name: 'some-plugin',
  build: function(options) {
    return 'transform-strict-mode';
    // or
    // return function() { /* blah blah */ }
  }
}

For these operations to actually run in parallel requires broccolijs/broccoli-persistent-filter#111 to be committed, so that the transpilations can happen concurrently.

stefanpenner

after reviewing this and the babel-persistent-filter pr, i feel this is the right direction.

Some open questions:

where do we do throttling?
how is the workflow \w errors (is the output mangled?)

stefanpenner · 2017-06-01T16:36:54Z

fixtures/amd-name-resolver-parallel/index.js

+module.exports = {
+  name: 'amd-name-resolver',
+
+  build: function(options) {


build(options) {

stefanpenner · 2017-06-01T16:36:59Z

fixtures/transform-es2015-block-scoping-parallel/index.js

+module.exports = {
+  name: 'transform-es2015-block-scoping',
+
+  build: function(options) {


stefanpenner · 2017-06-01T16:37:04Z

fixtures/transform-strict-mode-die-always/index.js

+module.exports = {
+  name: 'transform-strict-mode',
+
+  build: function(options) {


stefanpenner · 2017-06-01T16:39:38Z

index.js

@@ -25,16 +33,44 @@ function replaceExtensions(extensionsRegex, name) {
  return name;
 }

+function pluginCanBeParallelized(plugin) {


lets add a unit test for each of these helper methods

ok - I moved this stuff to a separate file as well to keep index.js a little cleaner

stefanpenner · 2017-06-01T16:41:32Z

index.js

-  return transpiler.transform(string, options);
+  if (transformIsParallelizable(options)) {
+    // send the job to the worker pool
+    return new Promise(function(resolve, reject) {


Can we extract this to its own file, so we can test this branch in isolation? Parallel/worker stuff is

Yeah I will extract this to a separate file

stefanpenner · 2017-06-01T16:43:52Z

index.js

-  return transpiler.transform(string, options);
+  if (transformIsParallelizable(options)) {
+    // send the job to the worker pool
+    return new Promise(function(resolve, reject) {


this promise wrapper is not required.

return pool.exec('transform', [string, options]).catch(err) { if (err.name === 'Error' && (err.message === 'Worker terminated unexpectedly' || err.message === 'Worker is terminated')) { // retry one time if it's a worker error resolve(pool.exec('transform', [string, options])); } else { throw err; } });

good call - simplifying that

stefanpenner · 2017-06-01T16:44:22Z

index.js

+          resolve(result);
+        },
+        function onRejected(err) {
+          if (err.name === 'Error' && (err.message === 'Worker terminated unexpectedly' ||


err may be undefined or null or a number you should likely add a guard.

typeof err === 'object' && err !== null

stefanpenner · 2017-06-05T20:30:24Z

lib/parallel-api.js

+
+
+module.exports = {
+  pluginCanBeParallelized: function(plugin) {


concise function pluginCanBeParallelized() {

stefanpenner · 2017-06-05T20:30:33Z

lib/parallel-api.js

+module.exports = {
+  pluginCanBeParallelized: function(plugin) {
+    return typeof plugin === 'string' ||
+           (Object.prototype.toString.call(plugin) === '[object Array]' &&


Array.isArray(plugin)

awesome, I knew there had to be a better way to check that

stefanpenner · 2017-06-05T20:30:46Z

lib/parallel-api.js

+
+  resolveModuleIsParallelizable: function(resolveModule) {
+    return resolveModule === undefined ||
+           (Object.prototype.toString.call(resolveModule) === '[object Array]' &&


Array.isArray(resolveModule)

stefanpenner · 2017-06-05T20:30:58Z

lib/parallel-api.js

+
+    if (options.resolveModuleSource !== undefined) {
+      // convert to work with Babel, if needed
+      if (Object.prototype.toString.call(options.resolveModuleSource) === '[object Array]') {


Array.isArray(options.resolveModuleSource)

stefanpenner · 2017-06-05T20:31:10Z

lib/parallel-api.js

+  transformOptions: function(options) {
+    var newOptions = options;
+
+    if (options.resolveModuleSource !== undefined) {


nested if seems redundant.

stefanpenner · 2017-06-05T20:31:28Z

lib/parallel-api.js

+    if (options.plugins !== undefined) {
+      // convert plugins to work with Babel, if needed
+      newOptions.plugins = options.plugins.map(function(plugin) {
+        if (Object.prototype.toString.call(plugin) === '[object Array]') {


Array.isArray(plugin)

stefanpenner · 2017-06-05T20:32:41Z

lib/parallel-api.js

+        if (typeof err === 'object' && err !== null &&
+            (err.message === 'Worker terminated unexpectedly' || err.message === 'Worker is terminated')) {
+          // retry one time if it's a worker error
+          return pool.exec('transform', [string, options]);


I don't believe we should retry (this will just slow things down). The work we doing isn't reliant on external resources that may be a partial failure, we should just assume one attempt === fatal.

Sounds good to me, and this makes things simpler

stefanpenner · 2017-06-05T20:34:00Z

lib/parallel-api.js

+      });
+    }
+    else {
+      return Promise.resolve(transpiler.transform(string, this.transformOptions(options)));


new Promise(resolve => resolve(transpiler.transform(string, this.transformOptions(options));

Otherwise if transpiler.transform throws synchronously, we will break the transformString signature by also throwing synchronously.

In general, if you function returns a promise it should always return a promise. Otherwise every caller must wrap in try/catch or promise constructor etc. So by wrapping at the "leaves" of a program, when we invoke external code that is not promise aware, we hide the rest of our program from these inconsistencies.

stefanpenner · 2017-06-05T20:41:34Z

based on our other discussion broccolijs/broccoli-persistent-filter#111 (comment) it seems like this PR will require throttling.

I helped @2cflor with this for broccoli-eyeglass and created a wrapper around async.queue that "just does the right thing" https://github.com/stefanpenner/async-promise-queue and will most likely also help you.

due to concurrency changes in babel-persistent-filter, the order of file processing and errors is non-deterministic also, fix pluralization in error message :)

concise methods Array.isArray() remove redundant nested if consistently return Promise

mikrostew · 2017-06-07T21:17:16Z

I added throttling/concurrency using async-promise-queue to the babel-persistent-filter PR with this commit.

There is already some throttling with this PR, due to how the workerpool module works. Each worker in the pool only runs one job at a time, and the pool is limited to a max number of workers.

stefanpenner · 2017-06-26T18:02:00Z

lib/parallel-api.js

+}
+
+function transformString(string, options) {
+  if (transformIsParallelizable(options)) {


we should debug log which branch we took.

Ok, adding debug logging like in broccoli-persistent-filter

stefanpenner · 2017-06-26T18:03:00Z

lib/worker.js

+// transpile the input string, using the input options
+function transform(string, options) {
+  return new Promise(function(resolve) {
+    var result = transpiler.transform(string, ParallelApi.transformOptions(options));


would it make more sense to name this ParallelApi.deserializeOptions ?

I think that makes sense, to match the other renaming

stefanpenner · 2017-06-26T18:03:32Z

lib/parallel-api.js

+
+function transformString(string, options) {
+  if (transformIsParallelizable(options)) {
+    return pool.exec('transform', [string, objectifyCallbacks(options)]);


would it make more sense to name objectifyCallbacks as serializeOptions ?

yeah I think that's clearer

stefanpenner · 2017-06-28T16:43:29Z

can we also document the JOBS env var

mikrostew · 2017-06-28T18:59:33Z

To test a against a decent sized project, I linked this into https://github.com/TryGhost/Ghost (which is using babel 6) and ran it there with no problems, same output as before.

stefanpenner · 2017-06-28T19:19:15Z

released as v6.1.0 🎉

mikrostew added 9 commits May 26, 2017 09:54

first hack at this with new API - needs testing

f4898e4

fix test failures

8939519

update parallelization logic, and add tests

f603760

cleanup some logic

c81bae7

use the same API for resolveModuleSource, + some cleanup

4314675

tests for error handling

dda1da1

check error message

184e74d

test worker termination

2c0e956

oops left some extra fixture in there

0b05a60

stefanpenner requested changes Jun 1, 2017

View reviewed changes

mikrostew added 7 commits June 1, 2017 12:00

handle case where new API is used in main process

05b0fe8

move stuff to lib/

aca8c22

add tests for transformOptions

3dca901

update fixtures with ES6 syntax

3dcc2c8

simplify transform() and add guard for error

3562938

refactor parallelization functions to separate file and add unit tests

936cdc1

refactor to a separate file

77a9e3b

stefanpenner requested changes Jun 5, 2017

View reviewed changes

mikrostew added 5 commits June 5, 2017 18:10

add test for thousands of files

5c893ce

update tests - can't rely on specific order of errors

de05d7d

due to concurrency changes in babel-persistent-filter, the order of file processing and errors is non-deterministic also, fix pluralization in error message :)

updates per SP feedback

9aab491

concise methods Array.isArray() remove redundant nested if consistently return Promise

no retry if worker process is terminated

f67ca8c

increase test timeout (sometimes takes longer in TravisCI)

189064f

configurable concurrency

51c7d56

mikrostew mentioned this pull request Jun 9, 2017

Parallel babel transpilation for the 5.x branch #115

Merged

stricter checking for parallel API

7a0597d

mikrostew mentioned this pull request Jun 12, 2017

Enable parallel transpile using API in broccoli-babel-transpiler ember-cli/ember-cli-htmlbars-inline-precompile#83

Merged

backwards-compatible API for resolveModuleSource option

7575c0b

mikrostew added 4 commits June 21, 2017 16:58

update how callback options are handled and parallelized

c0f89b4

refactor to avoid "this"

5c51f61

transform all callbacks to objects before sending to workers

78a05fc

parallelAPI -> _parallelAPI, and allow callback or build

a2cb3cc

mikrostew mentioned this pull request Jun 24, 2017

Enable parallel transpilation in broccoli-babel-transpiler ember-cli/amd-name-resolver#9

Merged

stefanpenner requested changes Jun 26, 2017

View reviewed changes

mikrostew added 3 commits June 26, 2017 16:11

better naming, debug logging, and updated dependency on BPF

3e95375

rework API as POJO with named params

1d220b7

documentation for the parallel API

c9d36c1

stefanpenner approved these changes Jun 28, 2017

View reviewed changes

document JOBS env var

2096c49

stefanpenner merged commit a3a5a23 into babel:master Jun 28, 2017

mikrostew mentioned this pull request Jul 24, 2017

Bump amd-name-resolver version to enable parallel babel transpile emberjs/data#5089

Merged

mikrostew mentioned this pull request Feb 24, 2018

Run uglify in parallel, using a workerpool ember-cli/broccoli-terser-sourcemap#63

Merged



		module.exports = {
		pluginCanBeParallelized: function(plugin) {

Parallel babel transpilation #114

Parallel babel transpilation #114

Conversation

mikrostew commented May 27, 2017

stefanpenner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanpenner commented Jun 5, 2017 • edited Loading

mikrostew commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanpenner commented Jun 28, 2017

mikrostew commented Jun 28, 2017

stefanpenner commented Jun 28, 2017

stefanpenner commented Jun 5, 2017 •

edited

Loading