From 81c78a9066d871d9be936858e25084938057947e Mon Sep 17 00:00:00 2001 From: Victor Lin Date: Mon, 3 Feb 2020 17:16:02 -0800 Subject: [PATCH 1/9] reduce the number of lstat calls --- packages/jest-haste-map/src/crawlers/node.ts | 30 ++++++++++++-------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/packages/jest-haste-map/src/crawlers/node.ts b/packages/jest-haste-map/src/crawlers/node.ts index 6e6b6b5d4c9d..fabb5f32c976 100644 --- a/packages/jest-haste-map/src/crawlers/node.ts +++ b/packages/jest-haste-map/src/crawlers/node.ts @@ -32,32 +32,38 @@ function find( function search(directory: string): void { activeCalls++; - fs.readdir(directory, (err, names) => { + fs.readdir(directory, { withFileTypes: true }, (err, entries) => { activeCalls--; if (err) { callback(result); return; } - names.forEach(file => { - file = path.join(directory, file); + entries.forEach(entry => { + const file = path.join(directory, entry.name); + if (ignore(file)) { return; } + + if (entry.isSymbolicLink()) { + return; + } + + if (entry.isDirectory()) { + search(file); + return; + } + activeCalls++; fs.lstat(file, (err, stat) => { activeCalls--; - if (!err && stat && !stat.isSymbolicLink()) { - if (stat.isDirectory()) { - search(file); - } else { - const ext = path.extname(file).substr(1); - if (extensions.indexOf(ext) !== -1) { - result.push([file, stat.mtime.getTime(), stat.size]); - } - } + const ext = path.extname(file).substr(1); + if (extensions.indexOf(ext) !== -1) { + result.push([file, stat.mtime.getTime(), stat.size]); } + if (activeCalls === 0) { callback(result); } From 5c14c7dc7651942e6601de0690183cd4d79b268b Mon Sep 17 00:00:00 2001 From: Victor Lin Date: Mon, 3 Feb 2020 17:26:59 -0800 Subject: [PATCH 2/9] update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d507a365eedf..2f0024f8e048 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ - `[jest-snapshot]` Downgrade semver to v6 to support node 8 ([#9451](https://github.com/facebook/jest/pull/9451)) - `[jest-transform]` Correct sourcemap behavior for transformed and instrumented code ([#9460](https://github.com/facebook/jest/pull/9460)) - `[pretty-format]` Export `OldPlugin` type ([#9491](https://github.com/facebook/jest/pull/9491)) +- `[jest-haste-map]` Reduce number of lstat calls in node crawler ### Chore & Maintenance From 819118c2eadb100af6e595465b3deeea1c7c8f59 Mon Sep 17 00:00:00 2001 From: Victor Lin Date: Mon, 3 Feb 2020 17:56:40 -0800 Subject: [PATCH 3/9] handle node < v10.10 --- packages/jest-haste-map/src/crawlers/node.ts | 43 +++++++++++++------- 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/packages/jest-haste-map/src/crawlers/node.ts b/packages/jest-haste-map/src/crawlers/node.ts index fabb5f32c976..d37562d10ab6 100644 --- a/packages/jest-haste-map/src/crawlers/node.ts +++ b/packages/jest-haste-map/src/crawlers/node.ts @@ -38,20 +38,27 @@ function find( callback(result); return; } - entries.forEach(entry => { - const file = path.join(directory, entry.name); + entries.forEach((entry: string | fs.Dirent) => { + let file: string; + // node < v10.10 does not support the withFileTypes option, and + // entry will be a string. + if (typeof entry === 'string') { + file = path.join(directory, entry); + } else { + file = path.join(directory, entry.name); - if (ignore(file)) { - return; - } + if (ignore(file)) { + return; + } - if (entry.isSymbolicLink()) { - return; - } + if (entry.isSymbolicLink()) { + return; + } - if (entry.isDirectory()) { - search(file); - return; + if (entry.isDirectory()) { + search(file); + return; + } } activeCalls++; @@ -59,9 +66,17 @@ function find( fs.lstat(file, (err, stat) => { activeCalls--; - const ext = path.extname(file).substr(1); - if (extensions.indexOf(ext) !== -1) { - result.push([file, stat.mtime.getTime(), stat.size]); + // This logic is unnecessary for node > v10.10, but leaving it in + // since we need it for backwards-compatibility still. + if (!err && stat && !stat.isSymbolicLink()) { + if (stat.isDirectory()) { + search(file); + } else { + const ext = path.extname(file).substr(1); + if (extensions.indexOf(ext) !== -1) { + result.push([file, stat.mtime.getTime(), stat.size]); + } + } } if (activeCalls === 0) { From 4f7109acf11a8d3a2da750c12ac7bc0cdcc34e24 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Tue, 4 Feb 2020 09:06:23 +0100 Subject: [PATCH 4/9] lint --- packages/jest-haste-map/src/crawlers/node.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/jest-haste-map/src/crawlers/node.ts b/packages/jest-haste-map/src/crawlers/node.ts index d37562d10ab6..8bc859782d44 100644 --- a/packages/jest-haste-map/src/crawlers/node.ts +++ b/packages/jest-haste-map/src/crawlers/node.ts @@ -32,7 +32,7 @@ function find( function search(directory: string): void { activeCalls++; - fs.readdir(directory, { withFileTypes: true }, (err, entries) => { + fs.readdir(directory, {withFileTypes: true}, (err, entries) => { activeCalls--; if (err) { callback(result); From 1dd833d99e0d1754510d736afbf33107b43d32be Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Tue, 4 Feb 2020 09:08:01 +0100 Subject: [PATCH 5/9] link to PR from changelog --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f0024f8e048..af88c7714c23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,6 @@ - `[jest-snapshot]` Downgrade semver to v6 to support node 8 ([#9451](https://github.com/facebook/jest/pull/9451)) - `[jest-transform]` Correct sourcemap behavior for transformed and instrumented code ([#9460](https://github.com/facebook/jest/pull/9460)) - `[pretty-format]` Export `OldPlugin` type ([#9491](https://github.com/facebook/jest/pull/9491)) -- `[jest-haste-map]` Reduce number of lstat calls in node crawler ### Chore & Maintenance @@ -22,6 +21,8 @@ ### Performance +- `[jest-haste-map]` Reduce number of `lstat` calls in node crawler ([#9514](https://github.com/facebook/jest/pull/9514)) + ## 25.1.0 ### Features From 68552339c8d0a0c4abfea246c982babfc0af4e77 Mon Sep 17 00:00:00 2001 From: Victor Lin Date: Tue, 4 Feb 2020 11:56:53 -0800 Subject: [PATCH 6/9] fix tests --- .../src/crawlers/__tests__/node.test.js | 11 +++++++++- packages/jest-haste-map/src/crawlers/node.ts | 20 +++++++++---------- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/packages/jest-haste-map/src/crawlers/__tests__/node.test.js b/packages/jest-haste-map/src/crawlers/__tests__/node.test.js index 69d3c3d6ca73..b1c994ba5112 100644 --- a/packages/jest-haste-map/src/crawlers/__tests__/node.test.js +++ b/packages/jest-haste-map/src/crawlers/__tests__/node.test.js @@ -56,7 +56,16 @@ jest.mock('fs', () => { }; return { lstat: jest.fn(stat), - readdir: jest.fn((dir, callback) => { + readdir: jest.fn((dir, options, callback) => { + // readdir has an optional `options` arg that's in the middle of the args list. + // we always provide it in practice, but let's try to handle the case where it's not + // provided too + if (typeof callback === 'undefined') { + if (typeof options === 'function') { + callback = options; + } + throw new Error('readdir: callback is not a function!'); + } if (dir === '/project/fruits') { setTimeout(() => callback(null, ['directory', 'tomato.js']), 0); } else if (dir === '/project/fruits/directory') { diff --git a/packages/jest-haste-map/src/crawlers/node.ts b/packages/jest-haste-map/src/crawlers/node.ts index 8bc859782d44..833f51f80f14 100644 --- a/packages/jest-haste-map/src/crawlers/node.ts +++ b/packages/jest-haste-map/src/crawlers/node.ts @@ -38,19 +38,19 @@ function find( callback(result); return; } + // node < v10.10 does not support the withFileTypes option, and + // entry will be a string. entries.forEach((entry: string | fs.Dirent) => { - let file: string; - // node < v10.10 does not support the withFileTypes option, and - // entry will be a string. - if (typeof entry === 'string') { - file = path.join(directory, entry); - } else { - file = path.join(directory, entry.name); + const file = path.join( + directory, + typeof entry === 'string' ? entry : entry.name, + ); - if (ignore(file)) { - return; - } + if (ignore(file)) { + return; + } + if (typeof entry !== 'string') { if (entry.isSymbolicLink()) { return; } From f077a642462a34992ba17176fa18deff9bc72725 Mon Sep 17 00:00:00 2001 From: Victor Lin Date: Tue, 4 Feb 2020 12:25:16 -0800 Subject: [PATCH 7/9] add lstat tests --- .../src/crawlers/__tests__/node.test.js | 121 +++++++++++++++++- 1 file changed, 114 insertions(+), 7 deletions(-) diff --git a/packages/jest-haste-map/src/crawlers/__tests__/node.test.js b/packages/jest-haste-map/src/crawlers/__tests__/node.test.js index b1c994ba5112..6130a2f8ffe8 100644 --- a/packages/jest-haste-map/src/crawlers/__tests__/node.test.js +++ b/packages/jest-haste-map/src/crawlers/__tests__/node.test.js @@ -31,6 +31,8 @@ jest.mock('child_process', () => ({ }), })); +let mockHasReaddirWithFileTypesSupport = false; + jest.mock('fs', () => { let mtime = 32; const size = 42; @@ -42,7 +44,7 @@ jest.mock('fs', () => { return path.endsWith('/directory'); }, isSymbolicLink() { - return false; + return path.endsWith('symlink'); }, mtime: { getTime() { @@ -66,12 +68,56 @@ jest.mock('fs', () => { } throw new Error('readdir: callback is not a function!'); } - if (dir === '/project/fruits') { - setTimeout(() => callback(null, ['directory', 'tomato.js']), 0); - } else if (dir === '/project/fruits/directory') { - setTimeout(() => callback(null, ['strawberry.js']), 0); - } else if (dir == '/error') { - setTimeout(() => callback({code: 'ENOTDIR'}, undefined), 0); + + if (mockHasReaddirWithFileTypesSupport) { + if (dir === '/project/fruits') { + setTimeout( + () => + callback(null, [ + { + isDirectory: () => true, + isSymbolicLink: () => false, + name: 'directory', + }, + { + isDirectory: () => false, + isSymbolicLink: () => false, + name: 'tomato.js', + }, + { + isDirectory: () => false, + isSymbolicLink: () => true, + name: 'symlink', + }, + ]), + 0, + ); + } else if (dir === '/project/fruits/directory') { + setTimeout( + () => + callback(null, [ + { + isDirectory: () => false, + isSymbolicLink: () => false, + name: 'strawberry.js', + }, + ]), + 0, + ); + } else if (dir == '/error') { + setTimeout(() => callback({code: 'ENOTDIR'}, undefined), 0); + } + } else { + if (dir === '/project/fruits') { + setTimeout( + () => callback(null, ['directory', 'tomato.js', 'symlink']), + 0, + ); + } else if (dir === '/project/fruits/directory') { + setTimeout(() => callback(null, ['strawberry.js']), 0); + } else if (dir == '/error') { + setTimeout(() => callback({code: 'ENOTDIR'}, undefined), 0); + } } }), stat: jest.fn(stat), @@ -305,4 +351,65 @@ describe('node crawler', () => { expect(removedFiles).toEqual(new Map()); }); }); + + describe('readdir withFileTypes support', () => { + it('calls lstat for directories and symlinks if readdir withFileTypes is not supported', () => { + nodeCrawl = require('../node'); + const fs = require('fs'); + + const files = new Map(); + return nodeCrawl({ + data: {files}, + extensions: ['js'], + forceNodeFilesystemAPI: true, + ignore: pearMatcher, + rootDir, + roots: ['/project/fruits'], + }).then(({hasteMap, removedFiles}) => { + expect(hasteMap.files).toEqual( + createMap({ + 'fruits/directory/strawberry.js': ['', 33, 42, 0, '', null], + 'fruits/tomato.js': ['', 32, 42, 0, '', null], + }), + ); + expect(removedFiles).toEqual(new Map()); + // once for /project/fruits, once for /project/fruits/directory + expect(fs.readdir).toHaveBeenCalledTimes(2); + // once for each of: + // 1. /project/fruits/directory + // 2. /project/fruits/directory/strawberry.js + // 3. /project/fruits/tomato.js + // 4. /project/fruits/symlink + // (we never call lstat on the root /project/fruits, since we know it's a directory) + expect(fs.lstat).toHaveBeenCalledTimes(4); + }); + }); + it('avoids calling lstat for directories and symlinks if readdir withFileTypes is supported', () => { + mockHasReaddirWithFileTypesSupport = true; + nodeCrawl = require('../node'); + const fs = require('fs'); + + const files = new Map(); + return nodeCrawl({ + data: {files}, + extensions: ['js'], + forceNodeFilesystemAPI: true, + ignore: pearMatcher, + rootDir, + roots: ['/project/fruits'], + }).then(({hasteMap, removedFiles}) => { + expect(hasteMap.files).toEqual( + createMap({ + 'fruits/directory/strawberry.js': ['', 33, 42, 0, '', null], + 'fruits/tomato.js': ['', 32, 42, 0, '', null], + }), + ); + expect(removedFiles).toEqual(new Map()); + // once for /project/fruits, once for /project/fruits/directory + expect(fs.readdir).toHaveBeenCalledTimes(2); + // once for strawberry.js, once for tomato.js + expect(fs.lstat).toHaveBeenCalledTimes(2); + }); + }); + }); }); From 758c76af58f9e9d41ab41f37d89f196b1140a4bc Mon Sep 17 00:00:00 2001 From: Victor Lin Date: Tue, 4 Feb 2020 12:44:26 -0800 Subject: [PATCH 8/9] skip checks in lstat callback --- packages/jest-haste-map/src/crawlers/node.ts | 21 ++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/packages/jest-haste-map/src/crawlers/node.ts b/packages/jest-haste-map/src/crawlers/node.ts index 833f51f80f14..0bcb0fcd82db 100644 --- a/packages/jest-haste-map/src/crawlers/node.ts +++ b/packages/jest-haste-map/src/crawlers/node.ts @@ -61,17 +61,26 @@ function find( } } + // If we made it here and had dirent support, we know that + // this is a normal file and can skip some checks in the lstat callback below + let passedFiletypeChecks = typeof entry !== 'string'; + activeCalls++; fs.lstat(file, (err, stat) => { activeCalls--; - // This logic is unnecessary for node > v10.10, but leaving it in - // since we need it for backwards-compatibility still. - if (!err && stat && !stat.isSymbolicLink()) { - if (stat.isDirectory()) { - search(file); - } else { + if (!err && stat) { + if (!passedFiletypeChecks) { + if (stat.isSymbolicLink()) { + // do nothing + } else if (stat.isDirectory()) { + search(file); + } else { + passedFiletypeChecks = true; + } + } + if (passedFiletypeChecks) { const ext = path.extname(file).substr(1); if (extensions.indexOf(ext) !== -1) { result.push([file, stat.mtime.getTime(), stat.size]); From 9f0aae7c79090cc5275aa7b91e2bb5a02d2c36f0 Mon Sep 17 00:00:00 2001 From: Victor Lin Date: Tue, 4 Feb 2020 13:23:28 -0800 Subject: [PATCH 9/9] Revert "skip checks in lstat callback" This reverts commit 758c76af58f9e9d41ab41f37d89f196b1140a4bc. --- packages/jest-haste-map/src/crawlers/node.ts | 21 ++++++-------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/packages/jest-haste-map/src/crawlers/node.ts b/packages/jest-haste-map/src/crawlers/node.ts index 0bcb0fcd82db..833f51f80f14 100644 --- a/packages/jest-haste-map/src/crawlers/node.ts +++ b/packages/jest-haste-map/src/crawlers/node.ts @@ -61,26 +61,17 @@ function find( } } - // If we made it here and had dirent support, we know that - // this is a normal file and can skip some checks in the lstat callback below - let passedFiletypeChecks = typeof entry !== 'string'; - activeCalls++; fs.lstat(file, (err, stat) => { activeCalls--; - if (!err && stat) { - if (!passedFiletypeChecks) { - if (stat.isSymbolicLink()) { - // do nothing - } else if (stat.isDirectory()) { - search(file); - } else { - passedFiletypeChecks = true; - } - } - if (passedFiletypeChecks) { + // This logic is unnecessary for node > v10.10, but leaving it in + // since we need it for backwards-compatibility still. + if (!err && stat && !stat.isSymbolicLink()) { + if (stat.isDirectory()) { + search(file); + } else { const ext = path.extname(file).substr(1); if (extensions.indexOf(ext) !== -1) { result.push([file, stat.mtime.getTime(), stat.size]);