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 2 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
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
118 changes: 73 additions & 45 deletions packages/jest-haste-map/src/index.ts
View file
Open in desktop
Expand Up @@ -339,30 +339,42 @@ 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;
return this._watch(hasteMap).then(() => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a quick observation, since this._buildPromise is an async function, is there a need to call then on this._watch? Shouldn't this be an await call followed by a return of the object?

Copy link
Member

Choose a reason for hiding this comment

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

This didn't used to use async/await in the past, so this is probably why it looks this way. It can be changed to await.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change to await. Thanks for the feedback.

__hasteMapForTest,
hasteFS,
moduleMap,
}));
})();
}
return this._buildPromise;
}
Expand Down Expand Up @@ -395,16 +407,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 +633,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 +678,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