Skip to content

Update: use fs.copyFile when available #4486

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.

Sign up for GitHub

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

Merged
BYK merged 8 commits into from
Sep 18, 2017
Merged

Update: use fs.copyFile when available #4486

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
110d43a
Update: use fs.copyFile when available
BYK Sep 16, 2017
c7c0072
Don't wait on writePromise
BYK Sep 16, 2017
c38b00f
More readable code
BYK Sep 17, 2017
58f6438
Fix bug in Node < 7
BYK Sep 17, 2017
a42adbd
Increase copy concurrency and add comment about where it came from
BYK Sep 17, 2017
b84289a
Merge branch 'master' into copyFile
BYK Sep 17, 2017
06ce7f5
Use better function flow type
BYK Sep 18, 2017
3a57109
Fix lint
BYK Sep 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ git:
language: node_js

node_js:
- "8"
- "6"
- "4"

Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions appveyor.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
environment:
matrix:
- node_version: "8"
- node_version: "6"
- node_version: "4"

Expand Down
2 changes: 1 addition & 1 deletion circle.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ general:

machine:
node:
version: 6
version: 8

dependencies:
cache_directories:
Expand Down
138 changes: 73 additions & 65 deletions src/util/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,31 @@ export const chmod: (path: string, mode: number | string) => Promise<void> = pro
export const link: (src: string, dst: string) => Promise<fs.Stats> = promisify(fs.link);
export const glob: (path: string, options?: Object) => Promise<Array<string>> = promisify(globModule);

const CONCURRENT_QUEUE_ITEMS = 4;

const CONCURRENT_QUEUE_ITEMS = fs.copyFile ? 16 : 4;
Copy link
Contributor

@cpojer cpojer Sep 17, 2017

Choose a reason for hiding this comment

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

How did you pick these numbers?

Copy link
Member Author

@BYK BYK Sep 17, 2017

Choose a reason for hiding this comment

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

I borrowed them from #3290. I did some testing myself with various numbers going as high as 64 and as low as 1. 1 definitely hurts but anything more than 8 doesn't make a huge difference. I think coupling this with the CPU count would be a better idea. What do you think?

Copy link
Contributor

@cpojer cpojer Sep 17, 2017

Choose a reason for hiding this comment

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

How about an elaborate code comment on how you came up with this truth? Numbers tend to be arbitrary in code unless we give explicit meaning to them.

Copy link
Member Author

@BYK BYK Sep 17, 2017

Choose a reason for hiding this comment

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

I'll experiment a bit more and then see if I can come up with a better logic. I think simply tying this tp process count should be fine and self-documenting. Otherwise, I'll add the comment as you suggested.

Copy link
Member Author

@BYK BYK Sep 17, 2017

Choose a reason for hiding this comment

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

Glad you brought this up @cpojer! I did some repeated testing for powers of 2 and found that 128 is consistently the best concurrency level. Updated the code and added a comment about this


const open: (path: string, flags: string | number, mode: number) => Promise<number> = promisify(fs.open);
const close: (fd: number) => Promise<void> = promisify(fs.close);
const write: (
fd: number,
buffer: Buffer,
offset: ?number,
length: ?number,
position: ?number,
) => Promise<void> = promisify(fs.write);
const futimes: (fd: number, atime: number, mtime: number) => Promise<void> = promisify(fs.futimes);
const copyFile: (src: string, dest: string, flags: number, data: CopyFileAction) => Promise<void> = 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) => {
const fd = await open(dest, 'w', data.mode);
try {
await write(fd, await readFileBuffer(src));
Copy link
Member

@arcanis arcanis Sep 16, 2017

Choose a reason for hiding this comment

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

Why not writeFile?

Copy link
Member Author

@BYK BYK Sep 16, 2017

Choose a reason for hiding this comment

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

writeFile and utimes expect file paths and use open() and close() behind the scenes. Using open, write and close manually here along with futimes which expects a file descriptor, makes the operation more efficient by avoiding an extra file open/close.

I think this is why the patch is faster than master even when fs.copyFile is not used.

Should I add a comment about this in the code?

await futimes(fd, data.atime, data.mtime);
} finally {
await close(fd);
}
};
const fsSymlink: (target: string, path: string, type?: 'dir' | 'file' | 'junction') => Promise<void> = promisify(
fs.symlink,
);
Expand All @@ -61,7 +84,6 @@ export type CopyQueueItem = {
type CopyQueue = Array<CopyQueueItem>;

type CopyFileAction = {
type: 'file',
src: string,
dest: string,
atime: number,
Expand All @@ -70,19 +92,21 @@ type CopyFileAction = {
};

type LinkFileAction = {
type: 'link',
src: string,
dest: string,
removeDest: boolean,
};

type CopySymlinkAction = {
type: 'symlink',
dest: string,
linkname: string,
};

type CopyActions = Array<CopyFileAction | CopySymlinkAction | LinkFileAction>;
type CopyActions = {
file: Array<CopyFileAction>,
symlink: Array<CopySymlinkAction>,
link: Array<LinkFileAction>,
};

type CopyOptions = {
onProgress: (dest: string) => void,
Expand Down Expand Up @@ -154,7 +178,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
Expand All @@ -180,7 +208,7 @@ async function buildActionsForCopy(
return actions;

//
async function build(data): Promise<void> {
async function build(data: CopyQueueItem): Promise<void> {
const {src, dest, type} = data;
const onFresh = data.onFresh || noop;
const onDone = data.onDone || noop;
Expand All @@ -196,8 +224,7 @@ async function buildActionsForCopy(
if (type === 'symlink') {
await mkdirp(path.dirname(dest));
onFresh();
actions.push({
type: 'symlink',
actions.symlink.push({
dest,
linkname: src,
});
Expand Down Expand Up @@ -288,10 +315,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()) {
Expand Down Expand Up @@ -326,8 +352,7 @@ async function buildActionsForCopy(
}
} else if (srcStat.isFile()) {
onFresh();
actions.push({
type: 'file',
actions.file.push({
src,
dest,
atime: srcStat.atime,
Expand All @@ -352,18 +377,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
Expand All @@ -389,7 +416,7 @@ async function buildActionsForHardlink(
return actions;

//
async function build(data): Promise<void> {
async function build(data: CopyQueueItem): Promise<void> {
const {src, dest} = data;
const onFresh = data.onFresh || noop;
const onDone = data.onDone || noop;
Expand Down Expand Up @@ -474,8 +501,7 @@ async function buildActionsForHardlink(
if (srcStat.isSymbolicLink()) {
onFresh();
const linkname = await readlink(src);
actions.push({
type: 'symlink',
actions.symlink.push({
dest,
linkname,
});
Expand Down Expand Up @@ -510,8 +536,7 @@ async function buildActionsForHardlink(
}
} else if (srcStat.isFile()) {
onFresh();
actions.push({
type: 'link',
actions.link.push({
src,
dest,
removeDest: destExists,
Expand All @@ -527,6 +552,15 @@ export function copy(src: string, dest: string, reporter: Reporter): Promise<voi
return copyBulk([{src, dest}], reporter);
}

const copyFileWithAttributes = async function(data: CopyFileAction, cleanup: Function): Promise<void> {
Copy link
Member Author

@BYK BYK Sep 16, 2017

Choose a reason for hiding this comment

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

This is a terrible name, I should probably change this to safeCopyFile with an explanation of unlink and cleanup parts. What do you think?

Copy link
Member

@arcanis arcanis Sep 16, 2017

Choose a reason for hiding this comment

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

The name looks fine, but a comment to explain the difference with copyFile would be useful.

I also feel like the arguments are also kinda obfuscated because of the custom CopyFileAction type.

try {
await unlink(data.dest);
await copyFile(data.src, data.dest, 0, data);
} finally {
cleanup();
}
};

export async function copyBulk(
queue: CopyQueue,
reporter: Reporter,
Expand All @@ -547,57 +581,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<CopyFileAction> = (actions.filter(action => action.type === 'file'): any);
const fileActions: Array<CopyFileAction> = actions.file;

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

await promise.queue(
fileActions,
async (data): Promise<void> => {
let writePromise: Promise<void>;
while ((writePromise = currentlyWriting[data.dest])) {
async (data: CopyFileAction): Promise<void> => {
const writePromise = currentlyWriting.get(data.dest);
if (writePromise) {
Copy link
Member

@arcanis arcanis Sep 16, 2017

Choose a reason for hiding this comment

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

You can't replace the while by a if, otherwise multiple waiting instances will all get released all at once. The while ensures that a single one will ever be able to cross the condition.

Copy link
Member Author

@BYK BYK Sep 16, 2017

Choose a reason for hiding this comment

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

I thought about this but here's my reasoning:

  • Event loop is single-threaded
  • If I do await promise the rest of the code is essentially gets into promise.then()
  • Since all code after this point is synchronous and since the event loop is single threaded, there can't be a new entry in the map between when the promise get's released and we add a new promise there.

Makes sense, or am I missing something?

Copy link
Member

@arcanis arcanis Sep 16, 2017

Choose a reason for hiding this comment

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

Even if the event loop is single threaded, because all iterations will wait on the same promise, all the promises will be released at once (but sequentially) because they won't re-check to make sure they are the first promise to wake up.

That being said, I just saw that this call is inside a promise.queue call, so maybe this utility will already take care of this (but then why await at all?).

Copy link
Member Author

@BYK BYK Sep 16, 2017

Choose a reason for hiding this comment

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

Ah, I see! Yes, you're right. For the promise queue part, I don't know why we simply don't return the existing promise. Doesn't make much sense to me since right now we are essentially writing the same file more than 1 time once the promsie is released. I'll change the code accordingly.

await 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 = copyFileWithAttributes(data, () => currentlyWriting.delete(data.dest));
currentlyWriting.set(data.dest, copier);
events.onProgress(data.dest);
return copier;
Copy link
Contributor

@cpojer cpojer Sep 17, 2017

Choose a reason for hiding this comment

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

I read: cpojer. lol

},
CONCURRENT_QUEUE_ITEMS,
);

// we need to copy symlinks last as they could reference files we were copying
const symlinkActions: Array<CopySymlinkAction> = (actions.filter(action => action.type === 'symlink'): any);
const symlinkActions: Array<CopySymlinkAction> = actions.symlink;
await promise.queue(symlinkActions, (data): Promise<void> => {
const linkname = path.resolve(path.dirname(data.dest), data.linkname);
reporter.verbose(reporter.lang('verboseFileSymlink', data.dest, linkname));
Expand All @@ -624,9 +632,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<LinkFileAction> = (actions.filter(action => action.type === 'link'): any);
const fileActions: Array<LinkFileAction> = actions.link;

await promise.queue(
fileActions,
Expand All @@ -641,7 +649,7 @@ export async function hardlinkBulk(
);

// we need to copy symlinks last as they could reference files we were copying
const symlinkActions: Array<CopySymlinkAction> = (actions.filter(action => action.type === 'symlink'): any);
const symlinkActions: Array<CopySymlinkAction> = actions.symlink;
await promise.queue(symlinkActions, (data): Promise<void> => {
const linkname = path.resolve(path.dirname(data.dest), data.linkname);
reporter.verbose(reporter.lang('verboseFileSymlink', data.dest, linkname));
Expand Down Expand Up @@ -836,7 +844,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<boolean> {
Expand Down