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.

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

[jest-haste-map] reduce the number of lstat calls in node crawler #9514

Merged
merged 9 commits into from Feb 5, 2020
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
Expand Up @@ -21,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
Expand Down
132 changes: 124 additions & 8 deletions packages/jest-haste-map/src/crawlers/__tests__/node.test.js
Expand Up @@ -31,6 +31,8 @@ jest.mock('child_process', () => ({
}),
}));

let mockHasReaddirWithFileTypesSupport = false;

jest.mock('fs', () => {
let mtime = 32;
const size = 42;
Expand All @@ -42,7 +44,7 @@ jest.mock('fs', () => {
return path.endsWith('/directory');
},
isSymbolicLink() {
return false;
return path.endsWith('symlink');
},
mtime: {
getTime() {
Expand All @@ -56,13 +58,66 @@ jest.mock('fs', () => {
};
return {
lstat: jest.fn(stat),
readdir: jest.fn((dir, callback) => {
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);
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 (mockHasReaddirWithFileTypesSupport) {
if (dir === '/project/fruits') {
setTimeout(
() =>
callback(null, [
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case you're wondering - I mirrored the existing mocks to look like a fake fs.Dirent object here. I thought about coming up with something fancy to avoid this duplication (e.g defining the mocks in a map and then reduceing it to get strings/dirents as needed) but it seemed more complicated than it was worth.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, let's try to keep it simple here

isDirectory: () => true,
isSymbolicLink: () => false,
name: 'directory',
},
{
isDirectory: () => false,
isSymbolicLink: () => false,
name: 'tomato.js',
},
{
isDirectory: () => false,
isSymbolicLink: () => true,
name: 'symlink',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also noticed that there weren't any existing tests for symlinks that I could find, so I added one here. It doesn't change the result of any existing tests (since we ignore symlinks) but will affect the lstat call-counting tests I added.

Copy link
Member

Choose a reason for hiding this comment

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

very nice, thank you! we'll hopefully be landing some symlink support soonish, so these tests should come in handy

},
]),
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),
Expand Down Expand Up @@ -296,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);
});
});
});
});
44 changes: 37 additions & 7 deletions packages/jest-haste-map/src/crawlers/node.ts
Expand Up @@ -32,32 +32,62 @@ 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);
// node < v10.10 does not support the withFileTypes option, and
// entry will be a string.
entries.forEach((entry: string | fs.Dirent) => {
const file = path.join(
directory,
typeof entry === 'string' ? entry : entry.name,
);

if (ignore(file)) {
return;
}

if (typeof entry !== 'string') {
if (entry.isSymbolicLink()) {
return;
}

if (entry.isDirectory()) {
search(file);
return;
}
}
SimenB marked this conversation as resolved.
Show resolved Hide resolved

// 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 && !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]);
}
}
}

if (activeCalls === 0) {
callback(result);
}
Expand Down