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

Cache babel-transpiled code #352

Merged
merged 1 commit into from
Dec 25, 2015
Merged

Cache babel-transpiled code #352

merged 1 commit into from
Dec 25, 2015

Conversation

vadimdemedes
Copy link
Contributor

Originated from #351, inspired from #189.

This PR was created to avoid retranspiling test files on every run and even require('babel-core'). MD5 hash of test file gets calculated and if stored hash equals current hash, AVA loads transpiled code from cache.

@vadimdemedes
Copy link
Contributor Author

Compared to master branch (see results in #351), now test runs take ~0.7s, given the same test files.

@vadimdemedes vadimdemedes force-pushed the cache-babel branch 2 times, most recently from d4d4377 to 844fad9 Compare December 22, 2015 16:48
@jamestalmage
Copy link
Contributor

I don't think we should put cached files in the home dir. Users will not be able to find and clear them. For nyc we settled on node_modules/.cache/nyc, the equivalent for use would be node_modules/.cache/ava. This means users will clear the cache themselves if they trash node_modules and reinstall (even if they don't know the cache is there). If we convince enough modules to adapt that pattern, it also makes trash node_modules/.cache a standard way to clear caches.

If you are storing in the home dir, then you need to take extra care salting the hash with the versions of Babel and every plugin used. That would still be a good idea in a local cache, but less critical since it's easier to clear the cache.

@sindresorhus
Copy link
Member

I like the node_modules/.cache convention.

@sindresorhus
Copy link
Member

// @floatdrop

@vadimdemedes
Copy link
Contributor Author

Good suggestion! Update is coming.

@jamestalmage
Copy link
Contributor

also, I wrote caching-transform for nyc. My intent was to use it here as well. Similar to cacha, but intended specifically for transforms.

@vadimdemedes
Copy link
Contributor Author

@sindresorhus @jamestalmage PR updated to store cache in node_modules/.cache/ava. Do you have other things in mind or we can merge it in?

appendPaths: module.paths
});
var cachePath = hasha(testPath);
var hashPath = hasha(testPath) + '_hash';
Copy link
Contributor

Choose a reason for hiding this comment

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

cachePath + '_hash'
instead of invoking hasha twice

@vadimdemedes vadimdemedes force-pushed the cache-babel branch 2 times, most recently from 74fc5a9 to 6e4c5b2 Compare December 22, 2015 18:43
@sindresorhus
Copy link
Member

LGTM. @jamestalmage ?

Tried it on got (6.0.0 branch). First uncached run 17 sec, and second cached run 7 sec.

@jamestalmage
Copy link
Contributor

The cache is currently saved in ./node_modules/ava/node_modules/.cache/ava, whereas I think it should be in ./node_modules/.cache/ava. That is how I did it in nyc, and my hope was we would all store our cache content in a single directory that was easy to delete.

I think the solution to this is to do:

pkgDir.sync(path.dirname(filename))

However, there is a chance they are using a global install, and do not have a package.json anywhere. I think we just disable caching in that situation, I do not like the idea of users having to hunt down were we are hiding the global cache.

nyc is starting with caching disabled by default for the time being. It is enabled with a --cache argument. I think we should do the opposite. Enable by default and provide a --no-cache argument. That could help eliminate the cache as the source of some difficult problem down the road.

We should probably salt the hash with babel.version as well (maybe even with the version of every preset/plugin, but that may be overkill).

@vadimdemedes
Copy link
Contributor Author

Why don't we just fallback to xdg-basedir (with os-tmpdir as a backup)? It is a stable approach.

Regarding cache, when do you think users would want to clear it?

@jamestalmage
Copy link
Contributor

Regarding cache, when do you think users would want to clear it?

Because caches cause problems. The one CSE joke I know is about cache invalidation. I can't think of a single piece of software I use that is so absolutely confident in its caching strategy that it does not provide a means for me to clear it. My browser, my IDE, npm, all of these give me an option to clear the cache.

As a practical example, the first thing I can think of is babel-plugin-xxx publishes a regression and the result of that regression makes its way into the cache. I am sure that is just the tip of the iceberg.

Perhaps, when we can't resolve their node modules folder, we do put it with the global AVA install and just provide a --clear-cache flag.

@sindresorhus
Copy link
Member

The cache is currently saved in ./node_modules/ava/node_modules/.cache/ava, whereas I think it should be in ./node_modules/.cache/ava.

Not sure about this. It would mean when the user upgrades to a new AVA version, that might change how the cache works, the existing cache is still kept around. Should at least then be ./node_modules/.cache/ava-0.8.0.

However, there is a chance they are using a global install, and do not have a package.json anywhere. I think we just disable caching in that situation

👍

We should probably salt the hash with babel.version

👍 Actually, we should make the salt based on the versions of the various Babel related packages. Upgrades to any of them should invalidate the cache.

Why don't we just fallback to xdg-basedir (with os-tmpdir as a backup)? It is a stable approach.

We now have multiple ways the cache can go wrong and when in xdg-basedir it's never emptied so cache errors will live forever unless the user does something manually to solve it, which I really don't want.

Perhaps, when we can't resolve their node modules folder, we do put it with the global AVA install and just provide a --clear-cache flag.

I don't see the point of a flag if we only put the cache in ./node_modules/.cache. A flag comes with a lot of hidden overhead.

@jamestalmage
Copy link
Contributor

Should at least then be ./node_modules/.cache/ava-0.8.0.

That makes it (only a little) harder to script removing the directory. I was going to suggest we include the AVA version in the salt. I'm fine with this approach though. I just really want it in their root node_modules folder so it is visible.

Actually, we should make the salt based on the versions of the various Babel related packages

I think we are going to find that difficult and costly. Babel doesn't do that in it's own cache implementation. To actually do that right, you would need to resolve the package.json of all 20 (twenty!) dependencies of babel-preset-es2015, fetch and parse those to extract pkg.version, and hash them all together. Same for babel-preset-stage-2 (another 5 dependencies when you walk the full graph), and again for power-assert, and transform-runtime

--clear-cache ... A flag comes with a lot of hidden overhead.

Agreed. That was offered as a workaround for my concerns with global installs. If we disable caching for global installs, then this is moot. I still think --no-cache has value, the cache introduces so many moving parts, it would be nice to be able to disable it and eliminate it as the source when troubleshooting a problem with users.

@sindresorhus
Copy link
Member

I was going to suggest we include the AVA version in the salt.

👍 AVA version in the salt would be even better.

I think we are going to find that difficult and costly. Babel doesn't do that in it's own cache implementation.

Ok, lets not do it then.


var opts = JSON.parse(process.argv[2]);
var testPath = opts.file;

var cache = cacha(path.join(pkgDir(path.dirname(testPath)), 'node_modules', '.cache', 'ava'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, the cache is being stored in node_modules/.cache/ava, but later down this file versions from ava & babel-* packages are being included in the cache key.

That way, when users rm -rf node_modules/.cache/ava, they can be sure they removed all the cache. And they won't need to get the AVA version to remove ava-0.8.0 (for example).

@vadimdemedes
Copy link
Contributor Author

So, are we good on this? Would really like to get it in before the release, so that users don't experience bad performance.

sindresorhus added a commit that referenced this pull request Dec 25, 2015
@sindresorhus sindresorhus merged commit 865cc05 into master Dec 25, 2015
@sindresorhus sindresorhus deleted the cache-babel branch December 25, 2015 15:33
@sindresorhus
Copy link
Member

👍

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.

4 participants