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

fix(jest-resolve): cache package.json lookups #11076

Merged
merged 4 commits into from
Feb 18, 2021

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Feb 11, 2021

Summary

See #11034.

@lencioni would you be able to test this instead of your custom resolver?

Test plan

Green CI

@lencioni
Copy link
Contributor

@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?

@SimenB
Copy link
Member Author

SimenB commented Feb 11, 2021

I usually just do ../jest/jest my-test (possibly uninstalling any local install of jest first). 😅 In this case though it should be enough to update your local installation of resolve to 1.20 and copy over a built packages/jest-resolve/src/defaultResolver.ts (i.e. packages/jest-resolve/build/defaultResolver.js) into your node_modules from this branch

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);
}

@lencioni
Copy link
Contributor

Profiling looks good. I ran 4 different scenarios.

  1. No custom resolver without this PR's changes: ~160s
  2. My custom resolver without this PR's changes: ~35s
  3. No custom resolver with this PR's changes: ~38s
  4. My custom resolver with this PR's changes: ~28s

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:

image

And here's the bottom up from my custom resolver + this PR's changes (my custom resolver shows up as "pineappleResolver" here BTW):

image

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!

@SimenB
Copy link
Member Author

SimenB commented Feb 12, 2021

Thanks so much for checking @lencioni!

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.

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

@SimenB
Copy link
Member Author

SimenB commented Feb 12, 2021

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.

Yup, the "find closest package.json" is probably something we should look into

@lencioni
Copy link
Contributor

Happy to see this improves the perf in your resolver as well, tho, that's sweet

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.

@ljharb
Copy link
Contributor

ljharb commented Feb 12, 2021

@SimenB if jest can safely assume that package.json files won't be modified during a test run, why wouldn't it be safe to assume that all package.json files that aren't in node_modules are present at the start of the test run?

@lencioni
Copy link
Contributor

@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.

@SimenB
Copy link
Member Author

SimenB commented Feb 12, 2021

@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.

Right, that's what I meant.

@SimenB SimenB force-pushed the resolve-readpackage branch from f671ccd to 902eacd Compare February 18, 2021 17:40
@@ -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;
Copy link
Member Author

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

@SimenB SimenB merged commit aec8573 into jestjs:master Feb 18, 2021
@SimenB SimenB deleted the resolve-readpackage branch February 18, 2021 18:41
@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants