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

Jump to bottom

Perf improvements - avoid persisting haste map / processing files when not changed. #8153

Merged
merged 5 commits into from Mar 19, 2019
Merged

Perf improvements - avoid persisting haste map / processing files when not changed. #8153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0d0d2e1
Haste map - avoid persisting haste map if not changed / processing fi…
scotthovestadt Mar 18, 2019
477d9ee
Fix lint error.
scotthovestadt Mar 18, 2019
d9b37df
Use await for watch to keep code readable and consistent.
scotthovestadt Mar 19, 2019
97703c0
Update CHANGELOG.md
scotthovestadt Mar 19, 2019
9522026
Add assertions for watchman changed files.
scotthovestadt Mar 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -15,6 +15,8 @@

### Performance

- `[jest-haste-map]` Avoid persisting haste map or processing files when not changed ([#8153](https://github.com/facebook/jest/pull/8153))

## 24.5.0

### Features
Expand Down
27 changes: 21 additions & 6 deletions packages/jest-haste-map/src/crawlers/__tests__/watchman.test.js
View file
Open in desktop
Expand Up @@ -128,7 +128,7 @@ describe('watchman watch', () => {
ignore: pearMatcher,
rootDir: ROOT_MOCK,
roots: ROOTS,
}).then(({hasteMap, removedFiles}) => {
}).then(({changedFiles, hasteMap, removedFiles}) => {
const client = watchman.Client.mock.instances[0];
const calls = client.command.mock.calls;

Expand Down Expand Up @@ -165,6 +165,8 @@ describe('watchman watch', () => {
}),
);

expect(changedFiles).toEqual(undefined);

expect(hasteMap.files).toEqual(mockFiles);

expect(removedFiles).toEqual(new Map());
Expand Down Expand Up @@ -210,7 +212,8 @@ describe('watchman watch', () => {
: null,
rootDir: ROOT_MOCK,
roots: ROOTS,
}).then(({hasteMap, removedFiles}) => {
}).then(({changedFiles, hasteMap, removedFiles}) => {
expect(changedFiles).toEqual(undefined);
expect(hasteMap.files).toEqual(
createMap({
[path.join(DURIAN_RELATIVE, 'foo.1.js')]: ['', 33, 43, 0, [], null],
Expand Down Expand Up @@ -265,7 +268,7 @@ describe('watchman watch', () => {
ignore: pearMatcher,
rootDir: ROOT_MOCK,
roots: ROOTS,
}).then(({hasteMap, removedFiles}) => {
}).then(({changedFiles, hasteMap, removedFiles}) => {
// The object was reused.
expect(hasteMap.files).toBe(mockFiles);

Expand All @@ -275,6 +278,12 @@ describe('watchman watch', () => {
}),
);

expect(changedFiles).toEqual(
createMap({
[KIWI_RELATIVE]: ['', 42, 40, 0, [], null],
}),
);

expect(hasteMap.files).toEqual(
createMap({
[KIWI_RELATIVE]: ['', 42, 40, 0, [], null],
Expand Down Expand Up @@ -349,7 +358,7 @@ describe('watchman watch', () => {
ignore: pearMatcher,
rootDir: ROOT_MOCK,
roots: ROOTS,
}).then(({hasteMap, removedFiles}) => {
}).then(({changedFiles, hasteMap, removedFiles}) => {
// The file object was *not* reused.
expect(hasteMap.files).not.toBe(mockFiles);

Expand All @@ -359,6 +368,8 @@ describe('watchman watch', () => {
}),
);

expect(changedFiles).toEqual(undefined);

// strawberry and melon removed from the file list.
expect(hasteMap.files).toEqual(
createMap({
Expand Down Expand Up @@ -443,14 +454,16 @@ describe('watchman watch', () => {
ignore: pearMatcher,
rootDir: ROOT_MOCK,
roots: ROOTS,
}).then(({hasteMap, removedFiles}) => {
}).then(({changedFiles, hasteMap, removedFiles}) => {
expect(hasteMap.clocks).toEqual(
createMap({
[FRUITS_RELATIVE]: 'c:fake-clock:3',
[VEGETABLES_RELATIVE]: 'c:fake-clock:4',
}),
);

expect(changedFiles).toEqual(undefined);

expect(hasteMap.files).toEqual(
createMap({
[KIWI_RELATIVE]: ['', 42, 52, 0, [], null],
Expand Down Expand Up @@ -506,7 +519,7 @@ describe('watchman watch', () => {
ignore: pearMatcher,
rootDir: ROOT_MOCK,
roots: [...ROOTS, ROOT_MOCK],
}).then(({hasteMap, removedFiles}) => {
}).then(({changedFiles, hasteMap, removedFiles}) => {
const client = watchman.Client.mock.instances[0];
const calls = client.command.mock.calls;

Expand Down Expand Up @@ -538,6 +551,8 @@ describe('watchman watch', () => {
}),
);

expect(changedFiles).toEqual(new Map());

expect(hasteMap.files).toEqual(new Map());

expect(removedFiles).toEqual(new Map());
Expand Down
5 changes: 5 additions & 0 deletions packages/jest-haste-map/src/crawlers/watchman.ts
View file
Open in desktop
Expand Up @@ -33,6 +33,7 @@ function WatchmanError(error: Error): Error {
export = async function watchmanCrawl(
options: CrawlerOptions,
): Promise<{
changedFiles?: FileData;
removedFiles: FileData;
hasteMap: InternalHasteMap;
}> {
Expand Down Expand Up @@ -148,6 +149,7 @@ export = async function watchmanCrawl(

let files = data.files;
let removedFiles = new Map();
const changedFiles = new Map();
let watchmanFiles: Map<string, any>;
let isFresh = false;
try {
Expand Down Expand Up @@ -243,17 +245,20 @@ export = async function watchmanCrawl(
absoluteVirtualFilePath,
);
files.set(relativeVirtualFilePath, nextData);
changedFiles.set(relativeVirtualFilePath, nextData);
}
}
} else {
files.set(relativeFilePath, nextData);
changedFiles.set(relativeFilePath, nextData);
}
}
}
}

data.files = files;
return {
changedFiles: isFresh ? undefined : changedFiles,
hasteMap: data,
removedFiles,
};
Expand Down
119 changes: 74 additions & 45 deletions packages/jest-haste-map/src/index.ts
View file
Open in desktop
Expand Up @@ -339,30 +339,43 @@ class HasteMap extends EventEmitter {

build(): Promise<InternalHasteMapObject> {
if (!this._buildPromise) {
this._buildPromise = this._buildFileMap()
.then(data => this._buildHasteMap(data))
.then(hasteMap => {
this._buildPromise = (async () => {
const data = await this._buildFileMap();

// Persist when we don't know if files changed (changedFiles undefined)
// or when we know a file was changed or deleted.
let hasteMap: InternalHasteMap;
if (
data.changedFiles === undefined ||
data.changedFiles.size > 0 ||
data.removedFiles.size > 0
) {
hasteMap = await this._buildHasteMap(data);
this._persist(hasteMap);
} else {
hasteMap = data.hasteMap;
}

const rootDir = this._options.rootDir;
const hasteFS = new HasteFS({
files: hasteMap.files,
rootDir,
});
const moduleMap = new HasteModuleMap({
duplicates: hasteMap.duplicates,
map: hasteMap.map,
mocks: hasteMap.mocks,
rootDir,
});
const __hasteMapForTest =
(process.env.NODE_ENV === 'test' && hasteMap) || null;
return this._watch(hasteMap).then(() => ({
__hasteMapForTest,
hasteFS,
moduleMap,
}));
const rootDir = this._options.rootDir;
const hasteFS = new HasteFS({
files: hasteMap.files,
rootDir,
});
const moduleMap = new HasteModuleMap({
duplicates: hasteMap.duplicates,
map: hasteMap.map,
mocks: hasteMap.mocks,
rootDir,
});
const __hasteMapForTest =
(process.env.NODE_ENV === 'test' && hasteMap) || null;
await this._watch(hasteMap);
return {
__hasteMapForTest,
hasteFS,
moduleMap,
};
})();
}
return this._buildPromise;
}
Expand Down Expand Up @@ -395,16 +408,19 @@ class HasteMap extends EventEmitter {
/**
* 2. crawl the file system.
*/
private _buildFileMap(): Promise<{
private async _buildFileMap(): Promise<{
removedFiles: FileData;
changedFiles?: FileData;
hasteMap: InternalHasteMap;
}> {
const read = this._options.resetCache ? this._createEmptyMap : this.read;

return Promise.resolve()
.then(() => read.call(this))
.catch(() => this._createEmptyMap())
.then(hasteMap => this._crawl(hasteMap));
let hasteMap: InternalHasteMap;
try {
const read = this._options.resetCache ? this._createEmptyMap : this.read;
hasteMap = await read.call(this);
} catch {
hasteMap = this._createEmptyMap();
}
return this._crawl(hasteMap);
}

/**
Expand Down Expand Up @@ -618,20 +634,34 @@ class HasteMap extends EventEmitter {
.then(workerReply, workerError);
}

private _buildHasteMap(data: {
private async _buildHasteMap(data: {
removedFiles: FileData;
changedFiles?: FileData;
hasteMap: InternalHasteMap;
}): Promise<InternalHasteMap> {
const {removedFiles, hasteMap} = data;
const map = new Map();
const mocks = new Map();
const promises = [];
const {removedFiles, changedFiles, hasteMap} = data;

// If any files were removed or we did not track what files changed, process
// every file looking for changes. Otherwise, process only changed files.
let map: ModuleMapData;
let mocks: MockData;
let filesToProcess: FileData;
if (changedFiles === undefined || removedFiles.size) {
map = new Map();
mocks = new Map();
filesToProcess = hasteMap.files;
} else {
map = hasteMap.map;
mocks = hasteMap.mocks;
filesToProcess = changedFiles;
}

for (const [relativeFilePath, fileMetadata] of removedFiles) {
this._recoverDuplicates(hasteMap, relativeFilePath, fileMetadata[H.ID]);
}

for (const relativeFilePath of hasteMap.files.keys()) {
const promises = [];
for (const relativeFilePath of filesToProcess.keys()) {
if (
this._options.skipPackageJson &&
relativeFilePath.endsWith(PACKAGE_JSON)
Expand All @@ -649,17 +679,16 @@ class HasteMap extends EventEmitter {
}
}

return Promise.all(promises)
.then(() => {
this._cleanup();
hasteMap.map = map;
hasteMap.mocks = mocks;
return hasteMap;
})
.catch(error => {
this._cleanup();
return Promise.reject(error);
});
try {
await Promise.all(promises);
this._cleanup();
hasteMap.map = map;
hasteMap.mocks = mocks;
return hasteMap;
} catch (error) {
Copy link
Member

Choose a reason for hiding this comment

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

could do

} finally {
  this._cleanup();
}

instead of the catch (and remove it from the happy path as well). Not sure if it's better or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the catch actually throws the error, it wouldn't make it to the finally block in this case. But yeah, the code duplication (even just calling the method) annoys me a little too. I don't see a way around it, though.

this._cleanup();
throw error;
}
}

private _cleanup() {
Expand Down