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

Perf improvement to stat in jest-resolve #11738

Closed
markjm opened this issue Aug 9, 2021 · 5 comments
Closed

Perf improvement to stat in jest-resolve #11738

markjm opened this issue Aug 9, 2021 · 5 comments

Comments

@markjm
Copy link
Contributor

markjm commented Aug 9, 2021

🐛 Bug Report

jest-resolve specified its own isFile and isDirectory. stat[Sync] does some weird stuff by capturing a very large stack trace on failure (see here). Based on what we need from the errors of statSync for resolving, we dont need these stack traces (as errors when resolving are much more "expected"). My proposal below should improve resolving performance (with included rough console.time numbers):

To Reproduce

Create a "simulated" resolve environment (just forcing a lot of failed stat calls)

const fs = require("fs");
function oldStat(path) {
  let stat;

  try {
    stat = fs.statSync(path);
  } catch (e) {
    if (!(e && (e.code === "ENOENT" || e.code === "ENOTDIR"))) {
      throw e;
    }
  }
}

console.time("oldStat");
for (let i = 0; i < 100000; i++) oldStat("not-real-path");
console.timeEnd("oldStat");


# Console output
oldStat: 8.336s

Expected behavior

If we prevent capturing these extended traces, we see much improved CPU time for these stat calls.

function newStat(path) {
  let stat;
  let oldTrace = Object.getOwnPropertyDescriptors(Error).stackTraceLimit;

  try {
    Object.defineProperty(Error, "stackTraceLimit", {
      value: 0,
      writable: false,
    });
    stat = fs.statSync(path);
  } catch (e) {
    if (!(e && (e.code === "ENOENT" || e.code === "ENOTDIR"))) {
      throw e;
    }
  } finally {
    Object.defineProperty(Error, "stackTraceLimit", oldTrace);
  }
}

console.time("newStat");
for (let i = 0; i < 100000; i++) newStat("not-real-path");
console.timeEnd("newStat");

# Console output
newStat: 5.014s

Is there any interest in this change? If so, I can make a simple PR to get this going. Apologies if this doesnt dall in "bug" work item type, but didnt think it was a feature either.

@markjm
Copy link
Contributor Author

markjm commented Aug 9, 2021

@sokra I wonder if this optimization can also be added to enhanced-resolve?

@SimenB
Copy link
Member

SimenB commented Aug 10, 2021

Feel free to send a PR with this change, seems reasonable to me 🙂 Ideally there'd be a way to stat and automatically ignore "not found" errors, but there probably isn't

@sokra
Copy link
Contributor

sokra commented Aug 10, 2021

This only affects statSync. In very recent node.js versions there is an option to return undefined instead of throwing an error on not found

@SimenB
Copy link
Member

SimenB commented Jan 6, 2022

#11749

@SimenB SimenB closed this as completed Jan 6, 2022
@github-actions
Copy link

github-actions bot commented Feb 6, 2022

This issue 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 Feb 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants