-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Perf improvements - avoid persisting haste map / processing files when not changed. #8153
Changes from 2 commits
0d0d2e1
477d9ee
d9b37df
97703c0
9522026
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(() => ({ | ||
__hasteMapForTest, | ||
hasteFS, | ||
moduleMap, | ||
})); | ||
})(); | ||
} | ||
return this._buildPromise; | ||
} | ||
|
@@ -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); | ||
} | ||
|
||
/** | ||
|
@@ -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) | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could do } finally {
this._cleanup();
} instead of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the |
||
this._cleanup(); | ||
throw error; | ||
} | ||
} | ||
|
||
private _cleanup() { | ||
|
There was a problem hiding this comment.
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 callthen
onthis._watch
? Shouldn't this be an await call followed by a return of the object?There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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.