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

Dramatically improve watch mode performance. #8201

Merged
merged 5 commits into from
Mar 24, 2019

Conversation

scotthovestadt
Copy link
Contributor

@scotthovestadt scotthovestadt commented Mar 24, 2019

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.

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So excited!

packages/jest-runner/src/testWorker.ts Outdated Show resolved Hide resolved
@alloy
Copy link

alloy commented Mar 24, 2019

Nice one, thanks! ✨

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome!

Could you update the changelog? 😀

static DuplicateHasteCandidatesError: typeof DuplicateHasteCandidatesError;
private static nextUniqueID = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static above instance variables?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

yeah, probably 🙂

packages/jest-haste-map/src/ModuleMap.ts Show resolved Hide resolved
@@ -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()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@scotthovestadt scotthovestadt Mar 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link

codecov-io commented Mar 24, 2019

Codecov Report

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

Impacted file tree graph

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

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@thymikee thymikee merged commit e75eaf0 into jestjs:master Mar 24, 2019
@cpojer
Copy link
Member

cpojer commented Mar 25, 2019

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
Copy link
Member

cpojer commented Mar 25, 2019

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
Copy link
Member

SimenB commented Mar 25, 2019

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

@cpojer
Copy link
Member

cpojer commented Mar 25, 2019

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
Copy link
Member

SimenB commented Mar 25, 2019

Right, makes sense

@scotthovestadt
Copy link
Contributor Author

@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
Copy link
Member

SimenB commented Mar 25, 2019

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

@scotthovestadt
Copy link
Contributor Author

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
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 11, 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.

Watch mode ~2 times slower than regular test run.
8 participants