Transpile in main thread #390

jamestalmage · 2015-12-30T07:30:04Z

Manual reimplementation of #349.

This moves Babel transforms to the main process, and caches the transpiled results.

It uses caching-transform to provide caching. Expensive requires (like babel) are placed inside the factory function, which caching-transform only calls when it determines it needs to transform a file.

The path to each precompiled test is passed in the options object to fork:

fork('/path/to/file.js', {
  precompiled: {
    '/path/to/file.js': '/path/to/cached/file.js'
  }
});

It uses require-precompiled to install a require hook that pulls the transformed file from the cache.

The caching directory is used by the users package.json, and installing in node_modules/.cache/ava.
If the cache directory cannot be found (i.e. if they have no package.json), or the user provides a --no-cache flag, pre-compilation still happens on the main thread - but results are published to a temp directory.

jamestalmage · 2015-12-30T07:46:23Z

Using got 6.0.0 branch:

master - first run (no cache)

./node_modules/.bin/ava  22.74s user 1.81s system 372% cpu 6.587 total

master - second run (with cache)

./node_modules/.bin/ava  6.65s user 0.77s system 175% cpu 4.240 total

this branch - first run (no cache)

./node_modules/.bin/ava  7.78s user 0.79s system 132% cpu 6.474 total

this branch - second run (w /cache)

./node_modules/.bin/ava  6.76s user 0.77s system 178% cpu 4.226 total

Huge advantage when the cache is empty.

jamestalmage · 2015-12-30T20:49:28Z

@sindresorhus, @vdemedes, et al. - ready for review.

ariporad · 2015-12-30T22:07:03Z

lib/caching-precompiler.js

+
+function CachingPrecompiler(cacheDir) {
+	if (!(this instanceof CachingPrecompiler)) {
+		throw new Error('CachingPrecompiler must be called with new');


@jamestalmage: What we do everywhere else is just invoke ourself with new and return it. Maybe switch to that?:

if (!(this instanceof CachingPrecompiler)) { return new CachingPrecompiler(cacheDir); }

Yeah sure. Probably worth matching style.

That said, after doing it the way you suggest for a long time, I am starting to think it's a mistake. ES2015 classes throw when function called, so magic auto-newing is not future proof. Also, it introduces an easy to overlook failure point when refactoring the function signature. I know I have forgotten to update that statement inside the if condition before.

I prefer being explicit and use new for classes. ES2015 seems to agree with me.

OK, changed it back to the original behavior (throw if they forget to use new).

ariporad · 2015-12-30T22:23:29Z

@jamestalmage: Looks pretty good, expect for the few things I mentioned. Also, it looks like there aren't very many tests on the CachingPrecompiler itself (I don't know if there needs to be).

sindresorhus · 2016-01-01T21:24:37Z

lib/caching-precompiler.js

+			filename: filename,
+			sourceMaps: true,
+			ast: false,
+			inputSourceMap: sourceMap && sourceMap.toObject()


Make sure to include the babelrc: false thing here.

sindresorhus · 2016-01-01T21:40:43Z

Woo, nice improvement on cold start! 👍

Generally looks good to me. Could use a test though.

@vdemedes ?

jamestalmage · 2016-01-01T23:01:48Z

could use a test though.

Yeah. How far do you want me to take it? For nyc, we run every test twice (once with caching on, once with caching off). That was easy to pull off with nyc due to how we build our test suite.

This is what I am thinking for tests:

Test a fixture project with caching on. Once the test completes ensure there are X number of files in the cache directory.
Delete the cache and test the same fixture from 1 again. Ensure the cache stays empty.
Duplicate some of our source map tests with caching off? I might need @novemberborn's help here since I know next to zero about how a good source map test should look.

sindresorhus · 2016-01-01T23:07:27Z

I don't think we should run all tests with and without. Just need some integration tests to ensure it's working correctly. What you've proposed sounds fine.

vadimdemedes · 2016-01-02T09:42:00Z

api.js

@@ -137,6 +144,11 @@ Api.prototype.run = function () {
 				return Promise.reject(new AvaError('Couldn\'t find any files to test'));
 			}

+			var cache = self.options.cache !== false;


I'd use isCacheEnabled, something more verbose & logical given the boolean variable value.

How about just cacheEnabled?

Or that, yeah

vadimdemedes · 2016-01-02T09:44:49Z

I really dig the perf improvement! I commented on a few things, but never mind. We can do a cleanup later, I'd rather have this PR merged ASAP.

Squashed commits: transpile in the main thread drop unused dependencies incorporate PR feedback add tests add unit tests

jamestalmage · 2016-01-02T10:00:34Z

OK, Incorporated the last round of feedback from @vdemedes. Once CI passes again, I intend to merge.

Transpile in main thread

jamestalmage · 2016-01-02T10:14:42Z

Landed.

jamestalmage mentioned this pull request Dec 30, 2015

Move Babel transforms to the main process. #349

Closed

jamestalmage changed the title ~~[WIP] transpile in main thread~~ Transpile in main thread Dec 30, 2015

ariporad reviewed Dec 30, 2015
View reviewed changes

jamestalmage mentioned this pull request Dec 31, 2015

Use an alternative to Bluebird in forked threads. #393

Closed

sindresorhus reviewed Jan 1, 2016
View reviewed changes

jamestalmage mentioned this pull request Jan 2, 2016

Stop reading .babelrc files. #398

Merged

jamestalmage force-pushed the transpile-in-main-thread branch 3 times, most recently from 785c6c8 to 8bc8387 Compare January 2, 2016 05:13

vadimdemedes reviewed Jan 2, 2016
View reviewed changes

Transpile test files in the main process:

614eb12

Squashed commits: transpile in the main thread drop unused dependencies incorporate PR feedback add tests add unit tests

jamestalmage force-pushed the transpile-in-main-thread branch from 8bc8387 to 614eb12 Compare January 2, 2016 09:59

jamestalmage added a commit that referenced this pull request Jan 2, 2016

Merge pull request #390 from jamestalmage/transpile-in-main-thread

ce47844

Transpile in main thread

jamestalmage merged commit ce47844 into avajs:master Jan 2, 2016

jamestalmage deleted the transpile-in-main-thread branch January 2, 2016 10:14

This was referenced Feb 29, 2016

Disable Babel #130

Closed

Improve babelification #152

Closed

vadimdemedes mentioned this pull request Mar 1, 2016

Transpile source files in the main process #577

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transpile in main thread #390

Transpile in main thread #390

jamestalmage commented Dec 30, 2015

jamestalmage commented Dec 30, 2015

jamestalmage commented Dec 30, 2015

ariporad Dec 30, 2015

jamestalmage Dec 31, 2015

sindresorhus Jan 2, 2016

jamestalmage Jan 2, 2016

ariporad commented Dec 30, 2015

sindresorhus Jan 1, 2016

sindresorhus commented Jan 1, 2016

jamestalmage commented Jan 1, 2016

sindresorhus commented Jan 1, 2016

vadimdemedes Jan 2, 2016

jamestalmage Jan 2, 2016

vadimdemedes Jan 2, 2016

vadimdemedes commented Jan 2, 2016

jamestalmage commented Jan 2, 2016

jamestalmage commented Jan 2, 2016

Transpile in main thread #390

Transpile in main thread #390

Conversation

jamestalmage commented Dec 30, 2015

jamestalmage commented Dec 30, 2015

jamestalmage commented Dec 30, 2015

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

ariporad commented Dec 30, 2015

Choose a reason for hiding this comment

sindresorhus commented Jan 1, 2016

jamestalmage commented Jan 1, 2016

sindresorhus commented Jan 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimdemedes commented Jan 2, 2016

jamestalmage commented Jan 2, 2016

jamestalmage commented Jan 2, 2016