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

Add fs.copyFile for copying files, using uv_fs_copyfile #14906

Closed
Daniel15 opened this issue Aug 17, 2017 · 9 comments · Fixed by #15034
Closed

Add fs.copyFile for copying files, using uv_fs_copyfile #14906

Daniel15 opened this issue Aug 17, 2017 · 9 comments · Fixed by #15034
Assignees
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding.

Comments

@Daniel15
Copy link

Daniel15 commented Aug 17, 2017

A new uv_fs_copyfile function recently landed in libuv in order to allow more efficient copying of files (in the future, it could allow copy-on-write semantics on file systems that support it). A copyFile function that uses this should be added to Node.js. 😃

Depends on upgrade to libuv 1.14.0 (#14866)

@Daniel15 Daniel15 changed the title Add fs.copyFile for copying files Add fs.copyFile for copying files, using uv_fs_copyfile Aug 17, 2017
@vsemozhetbyt vsemozhetbyt added feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding. labels Aug 17, 2017
@cjihrig
Copy link
Contributor

cjihrig commented Aug 17, 2017

I'll be working on this once libuv can be updated. Right now, it's blocked on a broken test.

@cjihrig cjihrig self-assigned this Aug 17, 2017
@Daniel15
Copy link
Author

Thank you @cjihrig! I just wanted to create an issue for tracking purposes 😃

@Fishrock123
Copy link
Contributor

Could someone detail the advantages of uv_fs_copyfile?

Do note that we make no guarentees to expose every libuv API. 😉

@Daniel15
Copy link
Author

Daniel15 commented Aug 18, 2017

Could someone detail the advantages of uv_fs_copyfile?

Two main advantages:

  1. It's significantly faster than a JS implementation, as it uses native system calls where available (eg. CopyFile on Windows, copyfile(3) on Mac OS). See some benchmark results on use fcopy for faster file copying yarnpkg/yarn#3290 and [WIP] Use the windows CopyFile API for copying files yarnpkg/yarn#2960 for example.
  2. It allows us to take advantage of copy-on-write semantics on file systems that support it, such as ZFS, BTRFS, ReFS, APFS. Copy-on-write means that creating a copy of a file reuses the same data on disk, similar to a hardlink. It uses very very little additional disk space, until the file is modified (at which point it's actually physically copied). This is hugely beneficial for apps that deal with lots of file copies, like Yarn and npm. It allows the benefits of hardlinks/symlinks (very fast installation, reusing the same data on the disk) without any of the disadvantages (changing one of the copies does not modify any of the other ones).

Copy-on-write needs native system calls to function, as the hacky mechanism used in Node.js projects at the moment (read the input file into a buffer and write it to the new location) is not recognised as a file copy by the OS. The file system just sees it as creation of a brand new file.

@MylesBorins
Copy link
Contributor

One thing to keep an eye out for. Replacing a JS implementation with a native API for file system related stuff will end up changing the underlying syscall failures if anything goes wrong. We saw this happen with realpath, and things got weird quickly.

This is in no way saying we shouldn't do this, but we should be pretty thorough with smoke testing if we do decide to change things. Either way it will likely be semver major, even if not obviously so

@cjihrig
Copy link
Contributor

cjihrig commented Aug 18, 2017

We aren't replacing anything here though. This would be purely semver minor for core. The applications using it can pick their own level of semver.

@Daniel15
Copy link
Author

Daniel15 commented Aug 18, 2017

. Replacing a JS implementation with a native API

There is no core JS implementation at the moment. This would add a brand new function, so there's no breaking change. Currently, apps are implementing their own JS versions of it.

@richardlau
Copy link
Member

Existing #12902 requests the same feature.

@tniessen
Copy link
Member

I am in favor of implementing this on core 👍

cjihrig added a commit to cjihrig/node that referenced this issue Sep 8, 2017
Fixes: nodejs#14906
PR-URL: nodejs#15034
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
cjihrig added a commit to cjihrig/node that referenced this issue Sep 10, 2017
Fixes: nodejs#14906
PR-URL: nodejs#15034
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Sep 10, 2017
Fixes: #14906
PR-URL: #15034
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Sep 11, 2017
Fixes: #14906
PR-URL: #15034
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Sep 11, 2017
Fixes: #14906
PR-URL: #15034
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Sep 12, 2017
Fixes: #14906
PR-URL: #15034
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
addaleax pushed a commit to addaleax/node that referenced this issue Sep 13, 2017
Fixes: nodejs#14906
PR-URL: nodejs#15034
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants