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

Move Babel transforms to the main process. #349

Closed

Conversation

jamestalmage
Copy link
Contributor

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.


Speedup is dramatic.
Using the 6.0.0 branch of got:

ava master branch

$ time ava
# npm 2
ava  85.51s user 7.19s system 592% cpu 15.650 total
# npm 3
ava  24.87s user 1.91s system 384% cpu 6.957 total

this branch using --no-cache

$ time ava --no-cache
# npm 2
ava --no-cache  11.19s user 1.05s system 130% cpu 9.379 total
# npm 3
ava --no-cache  7.59s user 0.75s system 134% cpu 6.217 total

This branch with caching enabled

$ time ava
# npm 2
ava  7.86s user 0.81s system 193% cpu 4.484 total
# npm 3
ava  6.64s user 0.72s system 173% cpu 4.237 total

A couple points of interest here:

  • This drastically reduces the penalty for sticking with npm 2 even on your first run.
  • You absolutely should upgrade to npm 3 for the best performance.

@vadimdemedes
Copy link
Contributor

Rebase went wrong here.

@jamestalmage
Copy link
Contributor Author

Rebase went wrong here.

It wasn't like that last night.

@jamestalmage jamestalmage force-pushed the move-babel-to-main-thread branch 2 times, most recently from 60c0cbf to c738c78 Compare December 22, 2015 19:07
"test": "xo && tap --coverage --reporter=spec --timeout=150 test/*.js",
"test-win": "tap --reporter=spec --timeout=150 test/*.js",
"coverage": "tap --coverage-report=lcov"
"pretest": "rm -rf ./node_modules/.cache",
Copy link
Member

Choose a reason for hiding this comment

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

Use https://github.com/sindresorhus/del-cli to make it work on Windwos too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I tried trash-cli first but it fails if the directory doesn't exist. Was just about to push rimraf, does del-cli accept missing directories?

Copy link
Member

Choose a reason for hiding this comment

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

@jamestalmage Re trash-cli, that's a bug sindresorhus/trash#43, but no point in having the overhead of moving it to the trash, not something you'd ever care about restoring.

@jamestalmage
Copy link
Contributor Author

@sindresorhus I used rimraf over del-cli, because sindresorhus/del-cli#3

@jamestalmage
Copy link
Contributor Author

Hmm, it seems implementing caching fixed my Windows errors. I previously saw some errors that made me suspicious the problem was in temp-write. I'm even more suspicious now.

@sindresorhus
Copy link
Member

@jamestalmage del-cli now supports Node.js 0.10.

@jamestalmage
Copy link
Contributor Author

OK, cool.
I'm heading out for ~4 hours. I will apply all the suggested changes later tonight.

There are many failing tests, but it basically works.
The latest `nyc` uses `append-transform`, which means multiple require extensions can be installed on top of the one `nyc` installs. Previous versions of nyc only allowed one additional transform before breaking. This allows us to use `require-precompiled` and users can still add `babel/register` (whenever we get those working).
`tap` automatically uploads coverage to coveralls for branches, but not PR's.

That required an ugly workaround in .travis.yml to manually upload only for PR's.

Since we now use `nyc` directly, and pass the the `--no-cov` flag in our `tap` commands, this is no longer a problem.
Does not yet handle missing `package.json`
This implements the `--no-cache` flag, but in this commit it's always set.
I want to see how well `--no-cache` works on Windows...
I need to decide on a good testing strategy for this.
I have updated it to use `write-file-atomic` to avoid race conditions
@jamestalmage jamestalmage changed the title [WIP] Move Babel transforms to the main process. Move Babel transforms to the main process. Dec 24, 2015
@jamestalmage
Copy link
Contributor Author

OK, This is ready for review.

// @vdemedes @sindresorhus

@@ -77,7 +78,8 @@ if (cli.flags.tap) {
var api = new Api(cli.input, {
failFast: cli.flags.failFast,
serial: cli.flags.serial,
require: arrify(cli.flags.require)
require: arrify(cli.flags.require),
noCache: cli.flags.cache === false
Copy link
Member

Choose a reason for hiding this comment

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

noCache => cache. I don't like double negatives.

.update(code, 'utf8')
.update(filename || 'unknown file', 'utf8')
.update(salt || '', 'utf8')
.digest('hex');
Copy link
Member

Choose a reason for hiding this comment

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

@jamestalmage
Copy link
Contributor Author

closed in favor of #390

@jamestalmage jamestalmage deleted the move-babel-to-main-thread branch April 6, 2016 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants