From 0ee1c55fd55f3863a7544e40a774162764d29536 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Burak=20Yi=C4=9Fit=20Kaya?= Date: Mon, 18 Sep 2017 13:47:31 +0100 Subject: [PATCH] Update: use fs.copyFile when available (#4486) **Summary** Fixes #4331. Supersedes #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. --- .travis.yml | 11 ++++ appveyor.yml | 1 + circle.yml | 2 +- src/util/fs.js | 154 ++++++++++++++++++++++++++++--------------------- 4 files changed, 101 insertions(+), 67 deletions(-) diff --git a/.travis.yml b/.travis.yml index 6f4b0655ba..797e18713d 100644 --- a/.travis.yml +++ b/.travis.yml @@ -10,6 +10,7 @@ git: language: node_js node_js: + - "8" - "6" - "4" @@ -38,10 +39,20 @@ matrix: node_js: "4" - env: TEST_TYPE="lint" node_js: "4" + - env: TEST_TYPE="build-dist" + node_js: "6" + - env: TEST_TYPE="check-lockfile" + node_js: "6" + - env: TEST_TYPE="lint" + node_js: "6" + include: - os: osx node_js: "6" env: TEST_TYPE="test-only" + - os: osx + node_js: "8" + env: TEST_TYPE="test-only" before_install: - if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then diff --git a/appveyor.yml b/appveyor.yml index d84c0d4656..abca8a06f0 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -1,5 +1,6 @@ environment: matrix: + - node_version: "8" - node_version: "6" - node_version: "4" diff --git a/circle.yml b/circle.yml index 93f57af395..bfde41fddc 100644 --- a/circle.yml +++ b/circle.yml @@ -7,7 +7,7 @@ general: machine: node: - version: 6 + version: 8 dependencies: cache_directories: diff --git a/src/util/fs.js b/src/util/fs.js index 0a280a7be2..054e556fe1 100644 --- a/src/util/fs.js +++ b/src/util/fs.js @@ -40,8 +40,37 @@ export const chmod: (path: string, mode: number | string) => Promise = pro export const link: (src: string, dst: string) => Promise = promisify(fs.link); export const glob: (path: string, options?: Object) => Promise> = promisify(globModule); -const CONCURRENT_QUEUE_ITEMS = 4; - +// fs.copyFile uses the native file copying instructions on the system, performing much better +// than any JS-based solution and consumes fewer resources. Repeated testing to fine tune the +// concurrency level revealed 128 as the sweet spot on a quad-core, 16 CPU Intel system with SSD. +const CONCURRENT_QUEUE_ITEMS = fs.copyFile ? 128 : 4; + +const open: (path: string, flags: string | number, mode: number) => Promise = promisify(fs.open); +const close: (fd: number) => Promise = promisify(fs.close); +const write: ( + fd: number, + buffer: Buffer, + offset: ?number, + length: ?number, + position: ?number, +) => Promise = promisify(fs.write); +const futimes: (fd: number, atime: number, mtime: number) => Promise = promisify(fs.futimes); +const copyFile: (src: string, dest: string, flags: number, data: CopyFileAction) => Promise = fs.copyFile + ? // Don't use `promisify` to avoid passing the last, argument `data`, to the native method + (src, dest, flags, data) => + new Promise((resolve, reject) => fs.copyFile(src, dest, flags, err => (err ? reject(err) : resolve(err)))) + : async (src, dest, flags, data) => { + // Use open -> write -> futimes -> close sequence to avoid opening the file twice: + // one with writeFile and one with utimes + const fd = await open(dest, 'w', data.mode); + try { + const buffer = await readFileBuffer(src); + await write(fd, buffer, 0, buffer.length); + await futimes(fd, data.atime, data.mtime); + } finally { + await close(fd); + } + }; const fsSymlink: (target: string, path: string, type?: 'dir' | 'file' | 'junction') => Promise = promisify( fs.symlink, ); @@ -61,7 +90,6 @@ export type CopyQueueItem = { type CopyQueue = Array; type CopyFileAction = { - type: 'file', src: string, dest: string, atime: number, @@ -70,19 +98,21 @@ type CopyFileAction = { }; type LinkFileAction = { - type: 'link', src: string, dest: string, removeDest: boolean, }; type CopySymlinkAction = { - type: 'symlink', dest: string, linkname: string, }; -type CopyActions = Array; +type CopyActions = { + file: Array, + symlink: Array, + link: Array, +}; type CopyOptions = { onProgress: (dest: string) => void, @@ -154,7 +184,11 @@ async function buildActionsForCopy( events.onStart(queue.length); // start building actions - const actions: CopyActions = []; + const actions: CopyActions = { + file: [], + symlink: [], + link: [], + }; // custom concurrency logic as we're always executing stacks of CONCURRENT_QUEUE_ITEMS queue items // at a time due to the requirement to push items onto the queue @@ -180,7 +214,7 @@ async function buildActionsForCopy( return actions; // - async function build(data): Promise { + async function build(data: CopyQueueItem): Promise { const {src, dest, type} = data; const onFresh = data.onFresh || noop; const onDone = data.onDone || noop; @@ -196,8 +230,7 @@ async function buildActionsForCopy( if (type === 'symlink') { await mkdirp(path.dirname(dest)); onFresh(); - actions.push({ - type: 'symlink', + actions.symlink.push({ dest, linkname: src, }); @@ -288,10 +321,9 @@ async function buildActionsForCopy( if (srcStat.isSymbolicLink()) { onFresh(); const linkname = await readlink(src); - actions.push({ + actions.symlink.push({ dest, linkname, - type: 'symlink', }); onDone(); } else if (srcStat.isDirectory()) { @@ -326,8 +358,7 @@ async function buildActionsForCopy( } } else if (srcStat.isFile()) { onFresh(); - actions.push({ - type: 'file', + actions.file.push({ src, dest, atime: srcStat.atime, @@ -352,18 +383,20 @@ async function buildActionsForHardlink( // initialise events for (const item of queue) { - const onDone = item.onDone; + const onDone = item.onDone || noop; item.onDone = () => { events.onProgress(item.dest); - if (onDone) { - onDone(); - } + onDone(); }; } events.onStart(queue.length); // start building actions - const actions: CopyActions = []; + const actions: CopyActions = { + file: [], + symlink: [], + link: [], + }; // custom concurrency logic as we're always executing stacks of CONCURRENT_QUEUE_ITEMS queue items // at a time due to the requirement to push items onto the queue @@ -389,7 +422,7 @@ async function buildActionsForHardlink( return actions; // - async function build(data): Promise { + async function build(data: CopyQueueItem): Promise { const {src, dest} = data; const onFresh = data.onFresh || noop; const onDone = data.onDone || noop; @@ -474,8 +507,7 @@ async function buildActionsForHardlink( if (srcStat.isSymbolicLink()) { onFresh(); const linkname = await readlink(src); - actions.push({ - type: 'symlink', + actions.symlink.push({ dest, linkname, }); @@ -510,8 +542,7 @@ async function buildActionsForHardlink( } } else if (srcStat.isFile()) { onFresh(); - actions.push({ - type: 'link', + actions.link.push({ src, dest, removeDest: destExists, @@ -527,6 +558,23 @@ export function copy(src: string, dest: string, reporter: Reporter): Promise jest). + * It also calls a cleanup function once it is done. + * + * `data` contains target file attributes like mode, atime and mtime. Built-in copyFile copies these + * automatically but our polyfill needs the do this manually, thus needs the info. + */ +const safeCopyFile = async function(data: CopyFileAction, cleanup: () => mixed): Promise { + try { + await unlink(data.dest); + await copyFile(data.src, data.dest, 0, data); + } finally { + cleanup(); + } +}; + export async function copyBulk( queue: CopyQueue, reporter: Reporter, @@ -547,57 +595,31 @@ export async function copyBulk( }; const actions: CopyActions = await buildActionsForCopy(queue, events, events.possibleExtraneous, reporter); - events.onStart(actions.length); + events.onStart(actions.file.length + actions.symlink.length + actions.link.length); - const fileActions: Array = (actions.filter(action => action.type === 'file'): any); + const fileActions: Array = actions.file; - const currentlyWriting: {[dest: string]: Promise} = {}; + const currentlyWriting: Map> = new Map(); await promise.queue( fileActions, - async (data): Promise => { - let writePromise: Promise; - while ((writePromise = currentlyWriting[data.dest])) { - await writePromise; + (data: CopyFileAction): Promise => { + const writePromise = currentlyWriting.get(data.dest); + if (writePromise) { + return writePromise; } - const cleanup = () => delete currentlyWriting[data.dest]; reporter.verbose(reporter.lang('verboseFileCopy', data.src, data.dest)); - return (currentlyWriting[data.dest] = readFileBuffer(data.src) - .then(async d => { - // we need to do this because of case-insensitive filesystems, which wouldn't properly - // change the file name in case of a file being renamed - await unlink(data.dest); - - return writeFile(data.dest, d, {mode: data.mode}); - }) - .then(() => { - return new Promise((resolve, reject) => { - fs.utimes(data.dest, data.atime, data.mtime, err => { - if (err) { - reject(err); - } else { - resolve(); - } - }); - }); - }) - .then( - () => { - events.onProgress(data.dest); - cleanup(); - }, - err => { - cleanup(); - throw err; - }, - )); + const copier = safeCopyFile(data, () => currentlyWriting.delete(data.dest)); + currentlyWriting.set(data.dest, copier); + events.onProgress(data.dest); + return copier; }, CONCURRENT_QUEUE_ITEMS, ); // we need to copy symlinks last as they could reference files we were copying - const symlinkActions: Array = (actions.filter(action => action.type === 'symlink'): any); + const symlinkActions: Array = actions.symlink; await promise.queue(symlinkActions, (data): Promise => { const linkname = path.resolve(path.dirname(data.dest), data.linkname); reporter.verbose(reporter.lang('verboseFileSymlink', data.dest, linkname)); @@ -624,9 +646,9 @@ export async function hardlinkBulk( }; const actions: CopyActions = await buildActionsForHardlink(queue, events, events.possibleExtraneous, reporter); - events.onStart(actions.length); + events.onStart(actions.file.length + actions.symlink.length + actions.link.length); - const fileActions: Array = (actions.filter(action => action.type === 'link'): any); + const fileActions: Array = actions.link; await promise.queue( fileActions, @@ -641,7 +663,7 @@ export async function hardlinkBulk( ); // we need to copy symlinks last as they could reference files we were copying - const symlinkActions: Array = (actions.filter(action => action.type === 'symlink'): any); + const symlinkActions: Array = actions.symlink; await promise.queue(symlinkActions, (data): Promise => { const linkname = path.resolve(path.dirname(data.dest), data.linkname); reporter.verbose(reporter.lang('verboseFileSymlink', data.dest, linkname)); @@ -836,7 +858,7 @@ export async function writeFilePreservingEol(path: string, data: string): Promis if (eol !== '\n') { data = data.replace(/\n/g, eol); } - await promisify(fs.writeFile)(path, data); + await writeFile(path, data); } export async function hardlinksWork(dir: string): Promise {