jestjs / jest

Delightful JavaScript Testing.
https://jestjs.io
MIT License
44.12k stars 6.44k forks source link

[Bug]: `findRepos` works slowly with a large number of `projects` #14818

Open voronin-ivan opened 9 months ago

voronin-ivan commented 9 months ago

Version

29.7.0

Steps to reproduce

  1. Clone the demo repo
  2. nvm use 20.10.0
  3. npm ci
  4. npm test packages/package-1/example.test.js -- --watch

You can reproduce this issue on any Node.js version starting at 18.13.0 by having a large number of projects.

Expected behavior

--watch mode doesn't consume large amounts of resources and works fast.

Actual behavior

--watch mode is resource-intensive, works slowly and slows down the whole system.

Screenshot 2023-12-30 at 11 09 49

Additional context

Why it's slow

findRepos is sluggish because of running a ton of childProcess.spawn under the hood (three processes per each project). It worked fast enough until Node.js version 18.13.0 was released, and this issue still persists even in the latest Node version (21.5.0).

Performance footprint for 30s

node --inspect-brk ./node_modules/.bin/jest packages/package-1/example.test.js --watch

Node.js 18.12.1

Screenshot 2023-12-30 at 11 35 58

Node.js 18.13.0 (and any later ones)

Screenshot 2023-12-30 at 11 37 56

Possible ways to solve it

  1. Update execa, the current version that you use (5.0.0) is quite outdated, the latest is 8.0.1, there were some optimisation tweaks
  2. Don't get repos for all projects if there is only one repo: in my monorepos there is always only one git repo, so findRepos works for nothing 99% of the time. This can be solved by introducing not required jest config fields like onlyOneRepo: boolean or repo: string
  3. ...

Environment

System:
    OS: macOS 14.1.1
    CPU: (10) arm64 Apple M1 Pro
Binaries:
    Node: 20.10.0 - ~/.nvm/versions/node/v20.10.0/bin/node
    Yarn: 1.22.17 - /opt/homebrew/bin/yarn
    npm: 10.2.3 - ~/.nvm/versions/node/v20.10.0/bin/npm
    pnpm: 8.6.2 - /opt/homebrew/bin/pnpm
npmPackages:
    jest: ^29.7.0 => 29.7.0
SimenB commented 9 months ago

Interesting! Do we know why it regressed so much in 18.13?

As for mitigations, easiest is probably just to avoid shelling out at all. Maybe walking the file system looking for .git? And whatever the equivalent is for Mercurial and Sapling

voronin-ivan commented 9 months ago

Do we know why it regressed so much in 18.13?

I'm not sure, at first sight, only https://github.com/nodejs/node/commit/7b68c06988 is related to child_process in that release.

easiest is probably just to avoid shelling out at all

Yes, that could be an option!

In my project, to mitigate this issue quickly knowing there is only one repo, I've made changes via patch-package πŸ˜„

Screenshot 2024-01-02 at 09 40 45
SimenB commented 9 months ago

FWIW, #14826 helps a bit before the findRepos calls.

I found https://github.com/antongolub/git-root - I wonder if we should use that? I'd assume just fixing git is enough as that's most used πŸ˜…

voronin-ivan commented 9 months ago

I found https://github.com/antongolub/git-root - I wonder if we should use that? I'd assume just fixing git is enough as that's most used πŸ˜…

Using git-root sounds good to me, but I'm not following how changing only findGitRoot solves the issue, findHgRoot and findSlRoot are still expensive and always run. So it looks like we need to tackle them all or find a way to say to Jest that project(s) contain only git repo(s)

SimenB commented 9 months ago

bah, of course... Mixing VCS-es sounds horrible, but I bet we'd break somebody if we looked for only looked for Mercurial and Sapling if we didn't find git roots.

I see that we find the VSC roots for every test run. I bet we could at least resolve those when normalizing config instead, so at least we'd only get the hit at the start of runs rather than every re-run.


https://github.com/jestjs/jest/pull/14826 should help a bit, btw

voronin-ivan commented 8 months ago

https://github.com/jestjs/jest/pull/14826 should help a bit, btw

Thank you, but I don't see much difference tbh because the most expensive operation is getRoute, tried on commit e334898 that contains your changes. node --inspect-brk ../jest/packages/jest/bin/jest packages/package-1/example.test.js --watch

Screenshot 2024-01-08 at 14 24 17
github-actions[bot] commented 7 months ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.