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.

Sign up for GitHub

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
Merged

Dramatically improve watch mode performance. #8201

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5e16bdf
Dramatically improve watch mode performance.
scotthovestadt Mar 24, 2019
551f1d7
Update CHANGELOG.md
scotthovestadt Mar 24, 2019
e0ec425
Module map unique ID must be truthy.
scotthovestadt Mar 24, 2019
67ea28f
Static above instance vars.
scotthovestadt Mar 24, 2019
68c2c43
Further improve performance by sending module maps during worker setup.
scotthovestadt Mar 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/jest-config/src/__tests__/getMaxWorkers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

});

describe('% based', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/jest-config/src/getMaxWorkers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,6 @@ export default function getMaxWorkers(
return parsed > 0 ? parsed : 1;
} else {
const cpus = os.cpus() ? os.cpus().length : 1;
return Math.max(argv.watch ? Math.floor(cpus / 2) : cpus - 1, 1);
return Math.max(cpus - 1, 1);
}
}
20 changes: 14 additions & 6 deletions packages/jest-haste-map/src/ModuleMap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,15 @@ export type SerializableModuleMap = {
};

export default class ModuleMap {
public readonly uniqueID: number;
private readonly _raw: RawModuleMap;
private json: SerializableModuleMap | undefined;
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 🙂


constructor(raw: RawModuleMap) {
this.uniqueID = ModuleMap.nextUniqueID;
ModuleMap.nextUniqueID++;
this._raw = raw;
}

Expand Down Expand Up @@ -84,12 +89,15 @@ export default class ModuleMap {
}

toJSON(): SerializableModuleMap {
return {
duplicates: Array.from(this._raw.duplicates),
map: Array.from(this._raw.map),
mocks: Array.from(this._raw.mocks),
rootDir: this._raw.rootDir,
};
if (!this.json) {
SimenB marked this conversation as resolved.
Show resolved Hide resolved
this.json = {
duplicates: Array.from(this._raw.duplicates),
map: Array.from(this._raw.map),
mocks: Array.from(this._raw.mocks),
rootDir: this._raw.rootDir,
};
}
return this.json;
}

static fromJSON(serializableModuleMap: SerializableModuleMap) {
Expand Down
2 changes: 2 additions & 0 deletions packages/jest-runner/src/__tests__/testRunner.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ test('does not inject the serializable module map in serial mode', () => {
config,
context: runContext,
globalConfig,
moduleMapUniqueID: null,
path: './file.test.js',
serializableModuleMap: null,
},
Expand All @@ -98,6 +99,7 @@ test('does not inject the serializable module map in serial mode', () => {
config,
context: runContext,
globalConfig,
moduleMapUniqueID: null,
path: './file2.test.js',
serializableModuleMap: null,
},
Expand Down
3 changes: 3 additions & 0 deletions packages/jest-runner/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

Expand Down
42 changes: 27 additions & 15 deletions packages/jest-runner/src/testWorker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@

import {Config} from '@jest/types';
import {SerializableError, TestResult} from '@jest/test-result';
import HasteMap, {SerializableModuleMap, ModuleMap} from 'jest-haste-map';
import HasteMap, {ModuleMap, SerializableModuleMap} from 'jest-haste-map';
import exit from 'exit';
import {separateMessageFromStack} from 'jest-message-util';
import Runtime from 'jest-runtime';
import Resolver from 'jest-resolve';
import {ErrorWithCode, TestRunnerSerializedContext} from './types';
import runTest from './runTest';

Expand All @@ -20,6 +21,7 @@ type WorkerData = {
globalConfig: Config.GlobalConfig;
path: Config.Path;
serializableModuleMap: SerializableModuleMap | null;
moduleMapUniqueID: number | null;
context?: TestRunnerSerializedContext;
};

Expand Down Expand Up @@ -47,7 +49,7 @@ const formatError = (error: string | ErrorWithCode): SerializableError => {
};
};

const resolvers = Object.create(null);
const resolvers = new Map<string, Resolver>();
const getResolver = (
config: Config.ProjectConfig,
moduleMap: ModuleMap | null,
Expand All @@ -56,31 +58,41 @@ const getResolver = (
// the test runner to the watch command. This is because jest-haste-map's
// watch mode does not persist the haste map on disk after every file change.
// To make this fast and consistent, we pass it from the TestRunner.
if (moduleMap) {
return Runtime.createResolver(config, moduleMap);
} else {
const name = config.name;
if (!resolvers[name]) {
resolvers[name] = Runtime.createResolver(
const name = config.name;
if (moduleMap || !resolvers.has(name)) {
resolvers.set(
name,
Runtime.createResolver(
config,
Runtime.createHasteMap(config).readModuleMap(),
);
}
return resolvers[name];
moduleMap || Runtime.createHasteMap(config).readModuleMap(),
),
);
}
return resolvers.get(name)!;
};

const deserializedModuleMaps = new Map<string, number>();
export async function worker({
config,
globalConfig,
path,
serializableModuleMap,
moduleMapUniqueID,
context,
}: WorkerData): Promise<TestResult> {
try {
const moduleMap = serializableModuleMap
? HasteMap.ModuleMap.fromJSON(serializableModuleMap)
: null;
// If the module map ID does not match what is currently being used by the
// config's resolver was passed, deserialize it and update the resolver.
let moduleMap: ModuleMap | null = null;
if (
serializableModuleMap &&
moduleMapUniqueID &&
scotthovestadt marked this conversation as resolved.
Show resolved Hide resolved
deserializedModuleMaps.get(config.name) !== moduleMapUniqueID
) {
deserializedModuleMaps.set(config.name, moduleMapUniqueID);
moduleMap = HasteMap.ModuleMap.fromJSON(serializableModuleMap);
}

return await runTest(
path,
globalConfig,
Expand Down