Dramatically improve watch mode performance. #8201

scotthovestadt · 2019-03-24T08:24:56Z

Summary

Resolves #7341

This PR dramatically improves watch mode performance, bringing it in line with single run mode performance. It accomplishes that by:

Workers previously initialized a new ModuleMap and Resolver for every test in watch mode. Now, those objects are only initialized once when the worker is setup.
In the main thread, caching the conversion of ModuleMap to a JSON-friendly object.

Benchmarks

I benchmarked against Jest's own test suite, excluding e2e tests which don't provide good signal because they individually take a long time (so startup time for the test is marginalized). The numbers show that running in Watch mode previously added an extra 35%~ of runtime to the tests but that has now been reduced to almost nothing.

Watch mode should now just be paying a one-time initial cost for each worker when the haste map changes instead of paying that same cost for every test run.

branch: master

yarn jest ./packages
Run time: 15.091s

yarn jest ./packages --watch
Run time: 23.234s

branch: watch-performance

yarn jest ./packages
Run time: 14.973s

yarn jest ./packages --watch
Run time: 15.196s

Test plan

All tests pass.
Benchmarked to verify the performance wins.
Verified that when the haste map is updated, the update is propagated out to all workers.

thymikee

So excited!

packages/jest-runner/src/testWorker.ts

alloy · 2019-03-24T09:01:28Z

Nice one, thanks! ✨

SimenB · 2019-03-24T10:10:15Z

packages/jest-config/src/__tests__/getMaxWorkers.test.ts

@@ -32,7 +32,7 @@ describe('getMaxWorkers', () => {

  it('Returns based on the number of cpus', () => {
    expect(getMaxWorkers({})).toBe(3);
-    expect(getMaxWorkers({watch: true})).toBe(2);
+    expect(getMaxWorkers({watch: true})).toBe(3);


was there any reason this was 1 less? @rogeliog do you know?

Maybe the rationale for cpus/2 was that watch mode is typically used while doing other things (mostly editing files) and this was to make it less likely that the editor becomes slow / freezes?

If we want to make it 1 less that's fine, but before it was halved! Just happened to be 1 less in this case.

On my machine, it was running 6 workers instead of 11.

I know, stumbled over the formula as well some time ago and wondered if there's any experimental basis for it ^^
I'm fine with using cpus - 1 for watch mode as well. It's a good heuristic for what performs best when Jest is the only thing running, we can't really do much about sharing resources with other applications.
If you want your editor to remain responsive while running an expensive task, nice yarn jest --watch is a good idea anyway.

SimenB

This is awesome!

Could you update the changelog? 😀

SimenB · 2019-03-24T10:12:46Z

packages/jest-haste-map/src/ModuleMap.ts

  static DuplicateHasteCandidatesError: typeof DuplicateHasteCandidatesError;
+  private static nextUniqueID = 0;


static above instance variables?

Also, as mentioned somewhere else, start this at 1 so it's not falsy?

static above instance should be a lint rule / auto-fixable? done.

static above instance should be a lint rule / auto-fixable?

yeah, probably 🙂

packages/jest-haste-map/src/ModuleMap.ts

scotthovestadt · 2019-03-24T16:46:36Z

packages/jest-runner/src/index.ts

@@ -134,6 +134,9 @@ class TestRunner {
              Array.from(this._context.changedFiles),
          },
          globalConfig: this._globalConfig,
+          moduleMapUniqueID: watcher.isWatchMode()
+            ? test.context.moduleMap.uniqueID
+            : null,
          path: test.path,
          serializableModuleMap: watcher.isWatchMode()
            ? test.context.moduleMap.toJSON()


@SimenB I've been thinking about this and I'm pretty sure we're still paying a serialization cost to send this to the worker for each test, just much less of one. Is this part of the public interface? Can I change it without it being considered a breaking change?

Can always explore in further PRs since this one is obviously an improvement, but it would be nice if I was able to move the logic up into the main thread and not even send the haste map if it wasn't new. I'm not sure if it is a breaking change.

I'm not sure... That's more of a question for @rubennorte or @mjesun

The less we have to send across ipc the better, so if we can change it I'm all for it 🙂

It looks like external runners include both the worker and the orchestrator, so changing what we pass to the worker here should be safely in the realm of a non-breaking-change!

Working to further optimize this.

@rubennorte @mjesun Let me know if I'm incorrect.

@SimenB Feel free to re-review, I've refactored this to approach it from a different angle and it's now even faster. On Jest's test suite, before Watch mode was a 6%~ performance cost (in my initial PR) and now it's about 1%~.

The refactor matters even more for larger haste maps, like Facebook's.

codecov-io · 2019-03-24T17:03:17Z

Codecov Report

Merging #8201 into master will decrease coverage by <.01%.
The diff coverage is 36.36%.

@@            Coverage Diff            @@
##           master   #8201      +/-   ##
=========================================
- Coverage    62.3%   62.3%   -0.01%     
=========================================
  Files         265     265              
  Lines       10473   10483      +10     
  Branches     2542    2543       +1     
=========================================
+ Hits         6525    6531       +6     
- Misses       3366    3370       +4     
  Partials      582     582

Impacted Files	Coverage Δ
packages/jest-haste-map/src/ModuleMap.ts	`64.28% <0%> (-2.39%)`	⬇️
packages/jest-runner/src/testWorker.ts	`0% <0%> (ø)`	⬆️
packages/jest-config/src/getMaxWorkers.ts	`100% <100%> (ø)`	⬆️
packages/jest-runner/src/index.ts	`68% <100%> (+4.36%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e4e170...68c2c43. Read the comment docs.

SimenB

Oh, hell yeah! I like this even more. Happy sunday! 😀

cpojer · 2019-03-25T14:08:09Z

As @jeysal pointed out, the reason only half of the available CPUs are used in watch mode is because otherwise Jest will grind your system to a halt. Ideally, watch mode should be unobtrusive. With the change from this PR, Jest will run faster at the cost of developer experience because all of their other tooling will be slower (at Facebook consider Buck, Jest, Flow, Nuclide etc. all fighting for resources).

Initially, we started out with using all available CPUs in Jest and people complained which is why we changed it to half of the available CPUs. I feel strongly that the change in this PR should be reverted back, even if it is at the cost of watch performance.

(Apologies for not adding a code comment in the past, I thought that this could be blamed back to the original PR that most likely has an explanation).

cpojer · 2019-03-25T14:08:43Z

Alternatively, I'm definitely happy if you wanna go for an adaptive approach that checks for recent CPU utilization and adjusts CPU usage for Jest accordingly.

SimenB · 2019-03-25T14:25:30Z

Shouldn't that work the same regardless of watch mode? Why should watch mode use less cores than a normal run?

cpojer · 2019-03-25T14:29:23Z

Mainly because watch mode is something you have open as a service, often in the background, and it runs automatically while writing/saving code. When you invoke jest on the other hand, the user intentionally invokes the test runner knowingly and is more likely to be ok with the resource usage.

SimenB · 2019-03-25T14:31:26Z

Right, makes sense

scotthovestadt · 2019-03-25T14:51:47Z

@cpojer I actually checked the blame but it was lost to an earlier refactor. :)

It was only a small part of this PR anyway, if anything I thought it might have been a mistake. I’ll submit a PR to revert that piece.

SimenB · 2019-03-25T15:18:48Z

Can that PR include doc changes which explains what these values are and why? That was part of the confusion in #7341

scotthovestadt · 2019-03-25T15:21:17Z

Sure, will do.

I just validated (as is pretty obvious) that even without the CPU change this is a massive performance boost. At 300k files in the map, I’m seeing things run almost twice as fast.

I’ll work on reverting that piece with documentation today.

github-actions · 2021-05-11T21:07:12Z

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.

Dramatically improve watch mode performance.

5e16bdf

facebook-github-bot added the cla signed label Mar 24, 2019

scotthovestadt requested review from SimenB and cpojer March 24, 2019 08:29

thymikee reviewed Mar 24, 2019

View reviewed changes

packages/jest-runner/src/testWorker.ts Outdated Show resolved Hide resolved

SimenB reviewed Mar 24, 2019

View reviewed changes

SimenB approved these changes Mar 24, 2019

View reviewed changes

scotthovestadt added 3 commits March 24, 2019 09:36

Update CHANGELOG.md

551f1d7

Module map unique ID must be truthy.

e0ec425

Static above instance vars.

67ea28f

scotthovestadt commented Mar 24, 2019

View reviewed changes

thymikee approved these changes Mar 24, 2019

View reviewed changes

Further improve performance by sending module maps during worker setup.

68c2c43

SimenB approved these changes Mar 24, 2019

View reviewed changes

thymikee merged commit e75eaf0 into jestjs:master Mar 24, 2019

scotthovestadt mentioned this pull request Mar 25, 2019

Watch mode number of CPUs & documentation. #8211

Merged

snyk-bot mentioned this pull request Dec 15, 2019

[Snyk] Upgrade babel-jest from 24.0.0 to 24.9.0 jakeherp/burger-builder#14

Open

This was referenced Mar 21, 2020

[Snyk] Upgrade: jest, jest-cli lwojcik/sc2pte-panel-frontend#88

Closed

[Snyk] Upgrade jest from 24.0.0 to 24.9.0 DavidKindler/burger-builder#47

Merged

Rossh87 mentioned this pull request Feb 3, 2021

Interactive Snapshot Update Mode outside of watch mode #8135

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dramatically improve watch mode performance. #8201

Dramatically improve watch mode performance. #8201

scotthovestadt commented Mar 24, 2019 •

edited

thymikee left a comment

alloy commented Mar 24, 2019

SimenB Mar 24, 2019

jeysal Mar 24, 2019

scotthovestadt Mar 24, 2019

jeysal Mar 24, 2019

SimenB left a comment

SimenB Mar 24, 2019

scotthovestadt Mar 24, 2019

SimenB Mar 24, 2019

scotthovestadt Mar 24, 2019

SimenB Mar 24, 2019

scotthovestadt Mar 24, 2019

scotthovestadt Mar 24, 2019 •

edited

codecov-io commented Mar 24, 2019 •

edited

SimenB left a comment •

edited

cpojer commented Mar 25, 2019

cpojer commented Mar 25, 2019

SimenB commented Mar 25, 2019

cpojer commented Mar 25, 2019

SimenB commented Mar 25, 2019

scotthovestadt commented Mar 25, 2019

SimenB commented Mar 25, 2019

scotthovestadt commented Mar 25, 2019

github-actions bot commented May 11, 2021

		static DuplicateHasteCandidatesError: typeof DuplicateHasteCandidatesError;
		private static nextUniqueID = 0;

Dramatically improve watch mode performance. #8201

Dramatically improve watch mode performance. #8201

Conversation

scotthovestadt commented Mar 24, 2019 • edited

Summary

Benchmarks

branch: master

branch: watch-performance

Test plan

thymikee left a comment

Choose a reason for hiding this comment

alloy commented Mar 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scotthovestadt Mar 24, 2019 • edited

Choose a reason for hiding this comment

codecov-io commented Mar 24, 2019 • edited

Codecov Report

SimenB left a comment • edited

Choose a reason for hiding this comment

cpojer commented Mar 25, 2019

cpojer commented Mar 25, 2019

SimenB commented Mar 25, 2019

cpojer commented Mar 25, 2019

SimenB commented Mar 25, 2019

scotthovestadt commented Mar 25, 2019

SimenB commented Mar 25, 2019

scotthovestadt commented Mar 25, 2019

github-actions bot commented May 11, 2021

scotthovestadt commented Mar 24, 2019 •

edited

scotthovestadt Mar 24, 2019 •

edited

codecov-io commented Mar 24, 2019 •

edited

SimenB left a comment •

edited