-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
fix(jest-resolve): cache package.json lookups #11076
Conversation
@SimenB I followed the instructions here: https://github.com/facebook/jest/blob/master/CONTRIBUTING.md#how-to-try-a-development-build-of-jest-in-another-project However, the profiling didn't seem to change much--I noticed that the profiler was saying that it was using packages like jest-resolve and resolve from my project's node_modules and not from the version I cloned and built. Is there a better way to try this out? |
I usually just do packages/jest-resolve/build/defaultResolver.js'use strict';
Object.defineProperty(exports, '__esModule', {
value: true
});
exports.default = defaultResolver;
exports.clearDefaultResolverCache = clearDefaultResolverCache;
function fs() {
const data = _interopRequireWildcard(require('graceful-fs'));
fs = function () {
return data;
};
return data;
}
function _jestPnpResolver() {
const data = _interopRequireDefault(require('jest-pnp-resolver'));
_jestPnpResolver = function () {
return data;
};
return data;
}
function _resolve() {
const data = require('resolve');
_resolve = function () {
return data;
};
return data;
}
function _jestUtil() {
const data = require('jest-util');
_jestUtil = function () {
return data;
};
return data;
}
function _interopRequireDefault(obj) {
return obj && obj.__esModule ? obj : {default: obj};
}
function _getRequireWildcardCache() {
if (typeof WeakMap !== 'function') return null;
var cache = new WeakMap();
_getRequireWildcardCache = function () {
return cache;
};
return cache;
}
function _interopRequireWildcard(obj) {
if (obj && obj.__esModule) {
return obj;
}
if (obj === null || (typeof obj !== 'object' && typeof obj !== 'function')) {
return {default: obj};
}
var cache = _getRequireWildcardCache();
if (cache && cache.has(obj)) {
return cache.get(obj);
}
var newObj = {};
var hasPropertyDescriptor =
Object.defineProperty && Object.getOwnPropertyDescriptor;
for (var key in obj) {
if (Object.prototype.hasOwnProperty.call(obj, key)) {
var desc = hasPropertyDescriptor
? Object.getOwnPropertyDescriptor(obj, key)
: null;
if (desc && (desc.get || desc.set)) {
Object.defineProperty(newObj, key, desc);
} else {
newObj[key] = obj[key];
}
}
}
newObj.default = obj;
if (cache) {
cache.set(obj, newObj);
}
return newObj;
}
/**
* Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/
function defaultResolver(path, options) {
// Yarn 2 adds support to `resolve` automatically so the pnpResolver is only
// needed for Yarn 1 which implements version 1 of the pnp spec
if (process.versions.pnp === '1') {
return (0, _jestPnpResolver().default)(path, options);
}
const result = (0, _resolve().sync)(path, {
basedir: options.basedir,
extensions: options.extensions,
isDirectory,
isFile,
moduleDirectory: options.moduleDirectory,
packageFilter: options.packageFilter,
paths: options.paths,
preserveSymlinks: false,
readPackageSync,
realpathSync
}); // Dereference symlinks to ensure we don't create a separate
// module instance depending on how it was referenced.
return realpathSync(result);
}
function clearDefaultResolverCache() {
checkedPaths.clear();
checkedRealpathPaths.clear();
packageContents.clear();
}
var IPathType;
(function (IPathType) {
IPathType[(IPathType['FILE'] = 1)] = 'FILE';
IPathType[(IPathType['DIRECTORY'] = 2)] = 'DIRECTORY';
IPathType[(IPathType['OTHER'] = 3)] = 'OTHER';
})(IPathType || (IPathType = {}));
const checkedPaths = new Map();
function statSyncCached(path) {
const result = checkedPaths.get(path);
if (result !== undefined) {
return result;
}
let stat;
try {
stat = fs().statSync(path);
} catch (e) {
if (!(e && (e.code === 'ENOENT' || e.code === 'ENOTDIR'))) {
throw e;
}
}
if (stat) {
if (stat.isFile() || stat.isFIFO()) {
checkedPaths.set(path, IPathType.FILE);
return IPathType.FILE;
} else if (stat.isDirectory()) {
checkedPaths.set(path, IPathType.DIRECTORY);
return IPathType.DIRECTORY;
}
}
checkedPaths.set(path, IPathType.OTHER);
return IPathType.OTHER;
}
const checkedRealpathPaths = new Map();
function realpathCached(path) {
let result = checkedRealpathPaths.get(path);
if (result !== undefined) {
return result;
}
result = (0, _jestUtil().tryRealpath)(path);
checkedRealpathPaths.set(path, result);
if (path !== result) {
// also cache the result in case it's ever referenced directly - no reason to `realpath` that as well
checkedRealpathPaths.set(result, result);
}
return result;
}
const packageContents = new Map();
function readPackageCached(path) {
let result = packageContents.get(path);
if (result !== undefined) {
return result;
}
result = JSON.parse(fs().readFileSync(path, 'utf8'));
packageContents.set(path, result);
return result;
}
/*
* helper functions
*/
function isFile(file) {
return statSyncCached(file) === IPathType.FILE;
}
function isDirectory(dir) {
return statSyncCached(dir) === IPathType.DIRECTORY;
}
function realpathSync(file) {
return realpathCached(file);
}
function readPackageSync(_, file) {
return readPackageCached(file);
} |
Profiling looks good. I ran 4 different scenarios.
So it looks like this change makes a big improvement, but there is still probably value in me keeping my custom resolver around after this lands. I think the difference is mostly due to my custom resolver avoiding a bunch of stat and realpath calls when looking for package.json for files outside of node_modules. Here's the bottom up from no custom resolver + this PR's changes: And here's the bottom up from my custom resolver + this PR's changes (my custom resolver shows up as "pineappleResolver" here BTW): One way to make this even faster might be to do a fast glob up front for package.json files and then use that information to shortcut that lookup everywhere, since that should avoid a bunch of stat calls. I believe this would require the other change to resolve that we discussed elsewhere. In any case, this PR seems to be a good perf improvement already, so I say ship it! |
Thanks so much for checking @lencioni!
That makes sense. I don't think that's an optimization we can make in Jest, but certainly one you can make in your own app 👍 160 to 38 is super awesome with something I believe is a safe optimization for everybody Happy to see this improves the perf in your resolver as well, tho, that's sweet |
Yup, the "find closest package.json" is probably something we should look into |
Yeah! BTW, this is because my resolver delegates to Jest's default resolver for anything in node_modules, so any improvements made to the default resolver will also improve my custom resolver. |
@SimenB if jest can safely assume that |
@ljharb I think Simen meant that the optimization I made in my custom resolver of assuming that there is only one package.json file in the whole repo for everything outside of node_modules is not an assumption that the default resolver can make. I suspect that if we implement the package.json glob-up-front optimization that my custom resolver won't provide much meaningful additional speed boost and I could probably just switch back to the default resolver entirely. |
Right, that's what I meant. |
f671ccd
to
902eacd
Compare
@@ -19,7 +19,7 @@ type ResolverOptions = { | |||
moduleDirectory?: Array<string>; | |||
paths?: Array<Config.Path>; | |||
rootDir?: Config.Path; | |||
packageFilter?: ResolveOpts['packageFilter']; | |||
packageFilter?: (pkg: any, pkgfile: string) => any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change so resolve
's types are not in our published types
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
See #11034.
@lencioni would you be able to test this instead of your custom resolver?
Test plan
Green CI