Skip to content

Commit 11be389

Browse files
BYKjoaolucasl
authored andcommitted
Update: use fs.copyFile when available (yarnpkg#4486)
**Summary** Fixes yarnpkg#4331. Supersedes yarnpkg#3290. Uses the newly added `fs.copyFile` on Node 8.5 hen available and falls back to the old buffer based method otherwise. This patch also refactors the file copy code a bit making it more efficient. Here are the durations on my computer with [this package.json](https://github.com/sciolist/fcopy/blob/c0ded8b82bbaa3f4cdb0a67e951eb3f8b0f97fb7/perf/project/alottafiles/package.json): | master | w/o copyFile | w/ copyFile | | - | - | - | | ~23s | ~19s | ~14s | This is with `yarn.lock` in place and w/o `node_modules`. **Test plan** CI should pass.
1 parent 9ce00cb commit 11be389

File tree

4 files changed

+101
-67
lines changed

4 files changed

+101
-67
lines changed

.travis.yml

+11
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ git:
1010
language: node_js
1111

1212
node_js:
13+
- "8"
1314
- "6"
1415
- "4"
1516

@@ -38,10 +39,20 @@ matrix:
3839
node_js: "4"
3940
- env: TEST_TYPE="lint"
4041
node_js: "4"
42+
- env: TEST_TYPE="build-dist"
43+
node_js: "6"
44+
- env: TEST_TYPE="check-lockfile"
45+
node_js: "6"
46+
- env: TEST_TYPE="lint"
47+
node_js: "6"
48+
4149
include:
4250
- os: osx
4351
node_js: "6"
4452
env: TEST_TYPE="test-only"
53+
- os: osx
54+
node_js: "8"
55+
env: TEST_TYPE="test-only"
4556

4657
before_install:
4758
- if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then

appveyor.yml

+1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
environment:
22
matrix:
3+
- node_version: "8"
34
- node_version: "6"
45
- node_version: "4"
56

circle.yml

+1-1
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ general:
77

88
machine:
99
node:
10-
version: 6
10+
version: 8
1111

1212
dependencies:
1313
cache_directories:

src/util/fs.js

+88-66
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,37 @@ export const chmod: (path: string, mode: number | string) => Promise<void> = pro
4040
export const link: (src: string, dst: string) => Promise<fs.Stats> = promisify(fs.link);
4141
export const glob: (path: string, options?: Object) => Promise<Array<string>> = promisify(globModule);
4242

43-
const CONCURRENT_QUEUE_ITEMS = 4;
44-
43+
// fs.copyFile uses the native file copying instructions on the system, performing much better
44+
// than any JS-based solution and consumes fewer resources. Repeated testing to fine tune the
45+
// concurrency level revealed 128 as the sweet spot on a quad-core, 16 CPU Intel system with SSD.
46+
const CONCURRENT_QUEUE_ITEMS = fs.copyFile ? 128 : 4;
47+
48+
const open: (path: string, flags: string | number, mode: number) => Promise<number> = promisify(fs.open);
49+
const close: (fd: number) => Promise<void> = promisify(fs.close);
50+
const write: (
51+
fd: number,
52+
buffer: Buffer,
53+
offset: ?number,
54+
length: ?number,
55+
position: ?number,
56+
) => Promise<void> = promisify(fs.write);
57+
const futimes: (fd: number, atime: number, mtime: number) => Promise<void> = promisify(fs.futimes);
58+
const copyFile: (src: string, dest: string, flags: number, data: CopyFileAction) => Promise<void> = fs.copyFile
59+
? // Don't use `promisify` to avoid passing the last, argument `data`, to the native method
60+
(src, dest, flags, data) =>
61+
new Promise((resolve, reject) => fs.copyFile(src, dest, flags, err => (err ? reject(err) : resolve(err))))
62+
: async (src, dest, flags, data) => {
63+
// Use open -> write -> futimes -> close sequence to avoid opening the file twice:
64+
// one with writeFile and one with utimes
65+
const fd = await open(dest, 'w', data.mode);
66+
try {
67+
const buffer = await readFileBuffer(src);
68+
await write(fd, buffer, 0, buffer.length);
69+
await futimes(fd, data.atime, data.mtime);
70+
} finally {
71+
await close(fd);
72+
}
73+
};
4574
const fsSymlink: (target: string, path: string, type?: 'dir' | 'file' | 'junction') => Promise<void> = promisify(
4675
fs.symlink,
4776
);
@@ -61,7 +90,6 @@ export type CopyQueueItem = {
6190
type CopyQueue = Array<CopyQueueItem>;
6291

6392
type CopyFileAction = {
64-
type: 'file',
6593
src: string,
6694
dest: string,
6795
atime: number,
@@ -70,19 +98,21 @@ type CopyFileAction = {
7098
};
7199

72100
type LinkFileAction = {
73-
type: 'link',
74101
src: string,
75102
dest: string,
76103
removeDest: boolean,
77104
};
78105

79106
type CopySymlinkAction = {
80-
type: 'symlink',
81107
dest: string,
82108
linkname: string,
83109
};
84110

85-
type CopyActions = Array<CopyFileAction | CopySymlinkAction | LinkFileAction>;
111+
type CopyActions = {
112+
file: Array<CopyFileAction>,
113+
symlink: Array<CopySymlinkAction>,
114+
link: Array<LinkFileAction>,
115+
};
86116

87117
type CopyOptions = {
88118
onProgress: (dest: string) => void,
@@ -154,7 +184,11 @@ async function buildActionsForCopy(
154184
events.onStart(queue.length);
155185

156186
// start building actions
157-
const actions: CopyActions = [];
187+
const actions: CopyActions = {
188+
file: [],
189+
symlink: [],
190+
link: [],
191+
};
158192

159193
// custom concurrency logic as we're always executing stacks of CONCURRENT_QUEUE_ITEMS queue items
160194
// at a time due to the requirement to push items onto the queue
@@ -180,7 +214,7 @@ async function buildActionsForCopy(
180214
return actions;
181215

182216
//
183-
async function build(data): Promise<void> {
217+
async function build(data: CopyQueueItem): Promise<void> {
184218
const {src, dest, type} = data;
185219
const onFresh = data.onFresh || noop;
186220
const onDone = data.onDone || noop;
@@ -196,8 +230,7 @@ async function buildActionsForCopy(
196230
if (type === 'symlink') {
197231
await mkdirp(path.dirname(dest));
198232
onFresh();
199-
actions.push({
200-
type: 'symlink',
233+
actions.symlink.push({
201234
dest,
202235
linkname: src,
203236
});
@@ -288,10 +321,9 @@ async function buildActionsForCopy(
288321
if (srcStat.isSymbolicLink()) {
289322
onFresh();
290323
const linkname = await readlink(src);
291-
actions.push({
324+
actions.symlink.push({
292325
dest,
293326
linkname,
294-
type: 'symlink',
295327
});
296328
onDone();
297329
} else if (srcStat.isDirectory()) {
@@ -326,8 +358,7 @@ async function buildActionsForCopy(
326358
}
327359
} else if (srcStat.isFile()) {
328360
onFresh();
329-
actions.push({
330-
type: 'file',
361+
actions.file.push({
331362
src,
332363
dest,
333364
atime: srcStat.atime,
@@ -352,18 +383,20 @@ async function buildActionsForHardlink(
352383

353384
// initialise events
354385
for (const item of queue) {
355-
const onDone = item.onDone;
386+
const onDone = item.onDone || noop;
356387
item.onDone = () => {
357388
events.onProgress(item.dest);
358-
if (onDone) {
359-
onDone();
360-
}
389+
onDone();
361390
};
362391
}
363392
events.onStart(queue.length);
364393

365394
// start building actions
366-
const actions: CopyActions = [];
395+
const actions: CopyActions = {
396+
file: [],
397+
symlink: [],
398+
link: [],
399+
};
367400

368401
// custom concurrency logic as we're always executing stacks of CONCURRENT_QUEUE_ITEMS queue items
369402
// at a time due to the requirement to push items onto the queue
@@ -389,7 +422,7 @@ async function buildActionsForHardlink(
389422
return actions;
390423

391424
//
392-
async function build(data): Promise<void> {
425+
async function build(data: CopyQueueItem): Promise<void> {
393426
const {src, dest} = data;
394427
const onFresh = data.onFresh || noop;
395428
const onDone = data.onDone || noop;
@@ -474,8 +507,7 @@ async function buildActionsForHardlink(
474507
if (srcStat.isSymbolicLink()) {
475508
onFresh();
476509
const linkname = await readlink(src);
477-
actions.push({
478-
type: 'symlink',
510+
actions.symlink.push({
479511
dest,
480512
linkname,
481513
});
@@ -510,8 +542,7 @@ async function buildActionsForHardlink(
510542
}
511543
} else if (srcStat.isFile()) {
512544
onFresh();
513-
actions.push({
514-
type: 'link',
545+
actions.link.push({
515546
src,
516547
dest,
517548
removeDest: destExists,
@@ -527,6 +558,23 @@ export function copy(src: string, dest: string, reporter: Reporter): Promise<voi
527558
return copyBulk([{src, dest}], reporter);
528559
}
529560

561+
/**
562+
* Unlinks the destination to force a recreation. This is needed on case-insensitive file systems
563+
* to force the correct naming when the filename has changed only in charater-casing. (Jest -> jest).
564+
* It also calls a cleanup function once it is done.
565+
*
566+
* `data` contains target file attributes like mode, atime and mtime. Built-in copyFile copies these
567+
* automatically but our polyfill needs the do this manually, thus needs the info.
568+
*/
569+
const safeCopyFile = async function(data: CopyFileAction, cleanup: () => mixed): Promise<void> {
570+
try {
571+
await unlink(data.dest);
572+
await copyFile(data.src, data.dest, 0, data);
573+
} finally {
574+
cleanup();
575+
}
576+
};
577+
530578
export async function copyBulk(
531579
queue: CopyQueue,
532580
reporter: Reporter,
@@ -547,57 +595,31 @@ export async function copyBulk(
547595
};
548596

549597
const actions: CopyActions = await buildActionsForCopy(queue, events, events.possibleExtraneous, reporter);
550-
events.onStart(actions.length);
598+
events.onStart(actions.file.length + actions.symlink.length + actions.link.length);
551599

552-
const fileActions: Array<CopyFileAction> = (actions.filter(action => action.type === 'file'): any);
600+
const fileActions: Array<CopyFileAction> = actions.file;
553601

554-
const currentlyWriting: {[dest: string]: Promise<void>} = {};
602+
const currentlyWriting: Map<string, Promise<void>> = new Map();
555603

556604
await promise.queue(
557605
fileActions,
558-
async (data): Promise<void> => {
559-
let writePromise: Promise<void>;
560-
while ((writePromise = currentlyWriting[data.dest])) {
561-
await writePromise;
606+
(data: CopyFileAction): Promise<void> => {
607+
const writePromise = currentlyWriting.get(data.dest);
608+
if (writePromise) {
609+
return writePromise;
562610
}
563611

564-
const cleanup = () => delete currentlyWriting[data.dest];
565612
reporter.verbose(reporter.lang('verboseFileCopy', data.src, data.dest));
566-
return (currentlyWriting[data.dest] = readFileBuffer(data.src)
567-
.then(async d => {
568-
// we need to do this because of case-insensitive filesystems, which wouldn't properly
569-
// change the file name in case of a file being renamed
570-
await unlink(data.dest);
571-
572-
return writeFile(data.dest, d, {mode: data.mode});
573-
})
574-
.then(() => {
575-
return new Promise((resolve, reject) => {
576-
fs.utimes(data.dest, data.atime, data.mtime, err => {
577-
if (err) {
578-
reject(err);
579-
} else {
580-
resolve();
581-
}
582-
});
583-
});
584-
})
585-
.then(
586-
() => {
587-
events.onProgress(data.dest);
588-
cleanup();
589-
},
590-
err => {
591-
cleanup();
592-
throw err;
593-
},
594-
));
613+
const copier = safeCopyFile(data, () => currentlyWriting.delete(data.dest));
614+
currentlyWriting.set(data.dest, copier);
615+
events.onProgress(data.dest);
616+
return copier;
595617
},
596618
CONCURRENT_QUEUE_ITEMS,
597619
);
598620

599621
// we need to copy symlinks last as they could reference files we were copying
600-
const symlinkActions: Array<CopySymlinkAction> = (actions.filter(action => action.type === 'symlink'): any);
622+
const symlinkActions: Array<CopySymlinkAction> = actions.symlink;
601623
await promise.queue(symlinkActions, (data): Promise<void> => {
602624
const linkname = path.resolve(path.dirname(data.dest), data.linkname);
603625
reporter.verbose(reporter.lang('verboseFileSymlink', data.dest, linkname));
@@ -624,9 +646,9 @@ export async function hardlinkBulk(
624646
};
625647

626648
const actions: CopyActions = await buildActionsForHardlink(queue, events, events.possibleExtraneous, reporter);
627-
events.onStart(actions.length);
649+
events.onStart(actions.file.length + actions.symlink.length + actions.link.length);
628650

629-
const fileActions: Array<LinkFileAction> = (actions.filter(action => action.type === 'link'): any);
651+
const fileActions: Array<LinkFileAction> = actions.link;
630652

631653
await promise.queue(
632654
fileActions,
@@ -641,7 +663,7 @@ export async function hardlinkBulk(
641663
);
642664

643665
// we need to copy symlinks last as they could reference files we were copying
644-
const symlinkActions: Array<CopySymlinkAction> = (actions.filter(action => action.type === 'symlink'): any);
666+
const symlinkActions: Array<CopySymlinkAction> = actions.symlink;
645667
await promise.queue(symlinkActions, (data): Promise<void> => {
646668
const linkname = path.resolve(path.dirname(data.dest), data.linkname);
647669
reporter.verbose(reporter.lang('verboseFileSymlink', data.dest, linkname));
@@ -836,7 +858,7 @@ export async function writeFilePreservingEol(path: string, data: string): Promis
836858
if (eol !== '\n') {
837859
data = data.replace(/\n/g, eol);
838860
}
839-
await promisify(fs.writeFile)(path, data);
861+
await writeFile(path, data);
840862
}
841863

842864
export async function hardlinksWork(dir: string): Promise<boolean> {

0 commit comments

Comments
 (0)