Transpile in the main process #945

jamestalmage · 2016-06-30T04:58:49Z

Fixes #577

Use the --precompile flag to apply transpilation to all file dependencies.

The idea would be to drop --require babel-register and use the --precompile flag instead.

It's pretty naive right now:

The shouldTranspile method just prevents transpiling node_modules, there's probably more logic that needs to be incorporated from Babels only and ignore options.
It caches using the hash of the transpilation output. This means the transpiler needs to run every time (and consequently babel-core gets loaded every time). We could speed things up more by trying to create a cache key from the input and the babel config, but that means resolving and parsing said config.
It can't handle dynamic requires, and using them will currently cause things to blow up (if the dynamic require needs transpilation). This could be improved by creating our own version of babel-register that does not load up babel-core except for dynamic requires (so you only pay the price when you actually use a dynamic require).
The test suite is very basic right now. Just a couple integration tests that verify it has an impact. This could be improved. Specifically, it probably needs some tests with dependencies in node_modules, and circular dependencies. It could also stand to have some true unit tests instead of just integration tests.

I would like it if a few people gave it a try to see if it improved performance at all. Especially people with lots of test files that are using --require babel-register. (paging @kentcdodds).

kentcdodds · 2016-06-30T13:06:49Z

Wahoo! Thanks for working on this. I'll give it a look in an hour or two and report back.

jamestalmage · 2016-06-30T21:18:01Z

So, some anecdotal numbers from a small project with just seven test files (23 tests):

ava --precompile              2.03s user  0.29s system  181% cpu  1.280 total
ava --require=babel-register  4.18s user  0.65s system  370% cpu  1.304 total

Total execution time is a smidge faster, but CPU time is much reduced (looks to be by about 50%).

jamestalmage · 2016-06-30T21:40:17Z

Wrote a copy script to turn my 7 test files into 70 (230 tests):

ava --require=babel-register  48.46s user  7.36s system  725% cpu  7.695 total

ava --precompile              16.34s user  2.41s system  577% cpu  3.250 total

nfcampos · 2016-06-30T21:41:23Z

either I'm not understanding how it works or it doesn't seem to work for me, when removing --require babel-register and replacing it with -precompile I'm getting this, which suggests that file is not being run through babel

jamestalmage · 2016-06-30T21:43:49Z

~~Hmm. Try it without the concurrency flag.~~

Scratch that. Works for me either way.

jamestalmage · 2016-06-30T21:45:39Z

Make sure to clear the cache:

rm -rf node_modules/.cache

nfcampos · 2016-06-30T21:47:54Z

clearing the cache didn't help

kentcdodds · 2016-06-30T21:48:36Z

Mine was interesting... So I just remembered that one of my tests has dynamic requires (so we instrument all our code for coverage), so I noticed the output indicating that which was cool. The tests went from 41 seconds to 19 seconds which is awesome. But I ended up with 5 test failures due to "Unexpected reserved word" and another test failure that's kind of interesting:

   6. Uncaught Exception
   TypeError: Expected `fromDir` and `moduleId` to be a string
    at module.exports (/Users/kdodds/Developer/paypal/p2pnodeweb/node_modules/resolve-from/index.js:7:9)
    at /Users/kdodds/Developer/paypal/p2pnodeweb/node_modules/ava/lib/precompiler.js:68:11
    at Array.map (native)
    at Precompiler.normalizeDependencies (/Users/kdodds/Developer/paypal/p2pnodeweb/node_modules/ava/lib/precompiler.js:67:4)
    at Precompiler.createHash (/Users/kdodds/Developer/paypal/p2pnodeweb/node_modules/ava/lib/precompiler.js:86:26)
    at Api._runFile (/Users/kdodds/Developer/paypal/p2pnodeweb/node_modules/ava/api.js:56:39)
    at Promise.map.concurrency (/Users/kdodds/Developer/paypal/p2pnodeweb/node_modules/ava/api.js:283:34)
    at tryCatcher (/Users/kdodds/Developer/paypal/p2pnodeweb/node_modules/ava/node_modules/bluebird/js/release/util.js:16:23)
...

I did clear the cache (note, with no cache it actually takes 26 seconds, but subsequent runs are 19ish seconds). I do have the concurrency option enabled.

So far these results are promising though!

nfcampos · 2016-06-30T21:55:29Z

so, it appears that some test files do run correctly, though not all, so it looks like you're finding the dependencies of some test files and missing the dependencies of others
edit: 21 files do not run because of not transpiled dependencies

jamestalmage · 2016-06-30T21:59:04Z

For dynamic requires, we probably need to give you a --force-precompile=pattern option. It won't be smart about what to precompile in that case, but just do everything that matches the force pattern.

Try with higher concurrency. You should be able to handle more now that babel-register isn't hogging memory in each thread.

I will look into that other error later tonight

jamestalmage · 2016-06-30T22:59:05Z

Are any of these open source projects?

kentcdodds · 2016-06-30T23:00:53Z

Sorry, not mine

nfcampos · 2016-06-30T23:27:39Z

mine isn't either

nfcampos · 2016-06-30T23:28:27Z

if any part of the ava or babel config or the file structure helps just ask

jamestalmage · 2016-07-01T00:26:01Z

Probably file structure and the list of imports for each.

nfcampos · 2016-07-01T09:47:55Z

deps.json.zip
this is a json map of files (sources and test files, test files are those that have tests in the path) to array of dependencies (each dependency is an array [string_in_the_import_statement, absolute_path]). does this help?

geowarin · 2016-07-01T14:02:28Z

lib/precompiler.js

+	var entry = this._cache[filename] = {};
+
+	var result = babelCore().transformFileSync(filename, {
+		plugins: [detective]


I think if you don't merge this with the babelConfig already defined, this will implicitly be merged with the .babelrc.
Which is totally cool if the babel option is set to "inherit". Otherwise, metadata generation might fail.

var config = objectAssign({}, this.babelConfig, { plugins: this.babelConfig.plugins.concat(detective) })

See this repo failing with this config

this will implicitly be merged with the .babelrc.

Yes. That's the idea. That is only precompiling your sources. We already precompile your tests in caching-precompiler.

@geowarin - see https://github.com/geowarin/repro-ava-precompile/issues/1 to fix yours.

jamestalmage · 2016-07-01T20:58:53Z

@nfcampos - What's up with importing .css files? Are they normally processed by babel-register?

nfcampos · 2016-07-01T21:01:05Z

@jamestalmage it's for using css-modules, they're dealt with by https://github.com/css-modules/css-modules-require-hook

You would still need to use that .css hook.

jamestalmage · 2016-07-01T21:06:09Z

So babel-register is completely uninvolved? The PR as it stands only precompiles .js files that are not in node_modules.

nfcampos · 2016-07-01T21:09:09Z

prior to this PR I had this array in the require entry in the ava config

"require": [
  "babel-register",
  "babel-polyfill",
  "css-modules-require-hook/preset"
]

to try out this PR I removed babel-register from that array and left the other two.
running ava --precompile after removing it resulted in some of the source files (.js) being transpiled and some not being transpiled

jamestalmage · 2016-07-01T21:23:32Z

@nfcampos - do you know which files?

jamestalmage · 2016-07-01T21:30:10Z

@nfcampos
Also, from your .json file. Why is ava resolving to /Users/nuno/cp/ava/index.js and not something in node_modules?

nfcampos · 2016-07-01T21:30:39Z

@jamestalmage because it's linked with npm link ava?

jamestalmage · 2016-07-01T21:32:09Z

Ah. I see.

sindresorhus · 2016-07-20T22:50:37Z

docs/experimental-features.md

+
+## Caveats
+
+Use of `test.only` does not work correctly when concurrency is limited. We think this is a solvable problem.


Can you linkify the text here to the relevant issue?

I know it's linked below, but would be useful to linkify it here too.

sindresorhus · 2016-07-20T22:57:57Z

I've commented on some nitpick, but generally looks great!

kentcdodds · 2016-07-20T23:14:57Z

I think that I've found why some of my tests are broken. This apparently doesn't transpile files that are required with proxyquire. This totally makes sense. Do you have any thoughts about what I could do to get around this? Or do you suppose this could be taken into account somehow?

dananichev · 2016-08-06T09:48:53Z

@jamestalmage Does it work with basic setup of nyc?

E.g.:

  "nyc": {
    "lines": 100,
    "statements": 100,
    "functions": 100,
    "branches": 100,
    "reporter": [
      "lcov",
      "text",
      "html"
    ],
    "include": [
      "src/**/*.js"
    ]
  }

And w/o babel-plugin-istanbul or babel-plugin-__coverage__.

Because with babel-require nyc produces coverage report but not with --precompile

dananichev · 2016-08-09T06:51:31Z

@jamestalmage nvm. Decided to remove babel-plugin-rewire and switch back to babel-plugin-istanbul (due to incompatibility between them). Works fine with --precompile.

michaelgilley · 2016-08-15T20:26:30Z

Hi all! Great work on the project. My team seeing really slow test times along with thrashed cpus when testing > 30 test files in a React app. This is especially the case for those using a VM (Ubuntu on PC). Any updates on this feature or ideas how to mitigate this issue? Thanks!!

jfairbank · 2016-08-15T20:52:26Z

Was going to pop in here too. I've run comparisons between "require": ["babel-register"] and "precompile": true, and this feature consistently halves our test suite run time, so I'm eager to see it merged.

@michaelgilley Have you tried the experimental --concurrency option for CPU thrashing? Tweaking the number of concurrent tests stopped the processes from overtaking my cores. I took some measurements too, and it didn't seem to slow down our test suite either.

michaelgilley · 2016-08-15T21:20:06Z

@jfairbank Yes, have tried that but as noted in #966 it appears to affect the process differently based on what's available including what other things are running in the background. @nickjvm has had issues running ava in his VM. It's even caused active Chrome tabs to crash! It seems like the sort of thing that ought to be baked into ava - to programmatically check what concurrency level it should be running. When running ava in npm scripts and often behind nyc for coverage it becomes awkward for a team to figure out how to individually configure ava to run correctly on their machines.

michaelgilley · 2016-08-29T22:36:11Z

What's the status on this PR? It seems pretty stale. We really need something like this to be done. With > 100 test files this really becomes essential. Any idea when Ava will be updated to support transpiling once, in the main process before tests are run?

For now we are setting concurrency to 6 but that means that 400 tests are taking at least several minutes to run.

sotojuan · 2016-09-04T20:56:22Z

Agreed with above but IIRC James is quite busy right now and probably not able to work on this.

@avajs/core This is the most needed open PR in my opinion, but it's not an easy problem to solve right? Anything we can do to help?

JaKXz · 2016-09-21T18:12:54Z

@sindresorhus would it be possible to get the documentation fixes merged in a separate PR so the --precompile flag can be shipped? cc @jamestalmage

novemberborn · 2016-09-22T09:41:07Z

I need to write up my thoughts but I'm doing some experiments with speeding up how we bootstrap Babel. That may make it feasible to load Babel in the test workers again, and even allow us to build in source compilation.

michaelgilley · 2016-10-05T19:30:48Z

Might I suggest taking a look at how Jest does this? We ended up switching and it's much, much faster. It may be prudent to take a page from their book. 😄

vadimdemedes · 2016-10-09T09:54:34Z

@michaelgilley I believe Jest does isolation via vm module, which indeed provides a sandbox for the tests. While it's an interesting solution, we are aiming for 100% isolation using forked node processes. As mentioned up the thread, @novemberborn has alternative speed-up implementation in the labs and there's also this one by @jamestalmage. Performance is one of our top concerns, we are constantly looking on how to speed your tests up.

novemberborn · 2017-01-17T17:08:55Z

Closing in favor of RFC001.

jamestalmage added 5 commits June 29, 2016 22:48

store a list of dependencies.

d34451e

transpile in the main process

6e04d1c

don't try to precompile core modules

ccf2205

don't precompile json

00ae6f3

allow missing sourcemaps

93b79b0

geowarin reviewed Jul 1, 2016
View reviewed changes

sindresorhus reviewed Jul 20, 2016
View reviewed changes

sindresorhus force-pushed the master branch from 5ccec78 to 30ab26f Compare August 18, 2016 16:21

novemberborn added the babel label Sep 25, 2016

novemberborn mentioned this pull request Sep 25, 2016

Alternative approach to Babel transpilation #1052

Closed

sindresorhus force-pushed the master branch 2 times, most recently from 4bc021e to 476c653 Compare October 18, 2016 03:23

sindresorhus force-pushed the master branch 4 times, most recently from 8610723 to 4acb78f Compare November 22, 2016 17:46

novemberborn mentioned this pull request Dec 20, 2016

RFC 001: Improving language support #1159

Merged

novemberborn closed this Jan 17, 2017

jamestalmage mentioned this pull request Nov 27, 2017

RFC: Move caching from babel-(loader|register) to babel-core and plugin framework babel/babel#5372

Open


		## Caveats

		Use of `test.only` does not work correctly when concurrency is limited. We think this is a solvable problem.

Transpile in the main process #945

Transpile in the main process #945

Conversation

jamestalmage commented Jun 30, 2016

kentcdodds commented Jun 30, 2016

jamestalmage commented Jun 30, 2016 • edited Loading

jamestalmage commented Jun 30, 2016

nfcampos commented Jun 30, 2016

jamestalmage commented Jun 30, 2016 • edited Loading

jamestalmage commented Jun 30, 2016

nfcampos commented Jun 30, 2016

kentcdodds commented Jun 30, 2016

nfcampos commented Jun 30, 2016 • edited Loading

jamestalmage commented Jun 30, 2016

jamestalmage commented Jun 30, 2016

kentcdodds commented Jun 30, 2016

nfcampos commented Jun 30, 2016

nfcampos commented Jun 30, 2016

jamestalmage commented Jul 1, 2016

nfcampos commented Jul 1, 2016

geowarin Jul 1, 2016

Choose a reason for hiding this comment

geowarin Jul 1, 2016

Choose a reason for hiding this comment

jamestalmage Jul 1, 2016

Choose a reason for hiding this comment

jamestalmage Jul 1, 2016

Choose a reason for hiding this comment

jamestalmage commented Jul 1, 2016

nfcampos commented Jul 1, 2016 • edited by jamestalmage Loading

jamestalmage commented Jul 1, 2016

nfcampos commented Jul 1, 2016 • edited Loading

jamestalmage commented Jul 1, 2016

jamestalmage commented Jul 1, 2016

nfcampos commented Jul 1, 2016

jamestalmage commented Jul 1, 2016

sindresorhus Jul 20, 2016

Choose a reason for hiding this comment

sindresorhus Jul 20, 2016

Choose a reason for hiding this comment

sindresorhus commented Jul 20, 2016

kentcdodds commented Jul 20, 2016

dananichev commented Aug 6, 2016

dananichev commented Aug 9, 2016

michaelgilley commented Aug 15, 2016

jfairbank commented Aug 15, 2016

michaelgilley commented Aug 15, 2016

michaelgilley commented Aug 29, 2016

sotojuan commented Sep 4, 2016 • edited Loading

JaKXz commented Sep 21, 2016 • edited Loading

novemberborn commented Sep 22, 2016

michaelgilley commented Oct 5, 2016

vadimdemedes commented Oct 9, 2016

novemberborn commented Jan 17, 2017

jamestalmage commented Jun 30, 2016 •

edited

Loading

jamestalmage commented Jun 30, 2016 •

edited

Loading

nfcampos commented Jun 30, 2016 •

edited

Loading

nfcampos commented Jul 1, 2016 •

edited by jamestalmage

Loading

nfcampos commented Jul 1, 2016 •

edited

Loading

sotojuan commented Sep 4, 2016 •

edited

Loading

JaKXz commented Sep 21, 2016 •

edited

Loading