-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
[WIP] Use the windows CopyFile API for copying files #2960
Conversation
This is an interesting idea! Does node.js not have a built-in way to invoke |
I don't think so - there's no |
I searched in node source code but they don't reference I'll cleanup this branch, add test & get some clean benchmarks to measure the difference exactly. PS: My original hope was finding a way to get NTFS to do CopyOnWrite, but that's only possible with ReFS variant on 2016 server and as it's a block operation it has some constraints ( https://msdn.microsoft.com/en-us/library/windows/desktop/mt590820(v=vs.85).aspx ) |
0bde4ef
to
80bb164
Compare
I rebased, cleaned up a little & fixed long paths. Did a few measurements (& Added to PR text) On the same hardware and project as the measurements done in #990 (comment) running
Removed one third of the execution time (In this very specific sample). Lot of tests are currently failing on appveyor due to stack overflows because it's not picking up the binary binding on the |
For what it's worth, I think it would be pretty cool to release the file copying code as a separate npm package so that other projects could take advantage of it too. 😃 For the test failures, try running the tests on your computer and see whether they pass.
Nice!
I'm looking forward to ReFS being available on consumer versions of Windows (hopefully one day!). If you want copy-on-write today, it's available on Linux through zfs and btrfs. I suspect the code would need to either do a syscall to whatever Linux uses for copying files or shell out to |
80bb164
to
20f9d57
Compare
I rebased the PR & see 2 problems with the tests here :
|
Now that I think about it, wouldn't merging this PR mean that we wouldn't be able to ship Yarn as a single js file anymore? Since |
@arcanis I didn't know that it was distributed as such but I think it can be detected with a fallback to the nodejs code if the native module isn't there. As there is no feature change it would just be a little slower on windows in single-js form How can I generate a single-js version to test it ? |
* Exclude 'ffi' and 'ref-wchar' from webpack builds * Handle their possible absence in the code
|
The current gains on my PC :
The basline tests were done from master so they include @sciolist #3234 fix |
@Daniel15 I just tried COW on btrfs and got a pretty large speedup: const ioctl = require('ioctl');
const FICLONE = 0x40049409; // _IOW (0x94, 9, int)
const source_fd = fs.openSync(data.src, 'r'); // sync for readability only
const dest_fs = fs.openSync(data.dest, 'w');
ioctl(dest_fd, FICLONE, source_fd);
fs.closeSync(dest_fd);
fs.closeSync(source_fd); Problem is this magic constant requires the kernel headers to be determined reliably, so for Linux support splitting this functionality into a separate (native) package would really help. |
You could also try using libuv's sendfile, which copies files about 70% faster than the WriteStreams in yarn on platforms where sendfile works for file copy (linux and osx, some other unixes).. Otherwise it falls back to a more naive read+write loop that runs about 20% faster than yarns current copying. Unfortunately sendfile was removed from node's fs a few years ago, because it was a bit broken at the time. on my reasonably large test project sendfile improves initial install times by about 50% on linux/osx, 10-15% on windows. Sounds like making a general 'copyfile' library that could optimize better for each platform wouldn't be a bad idea. |
So, what is the next step? |
This is a great idea. @vbfox what do you think about putting the ffi stuff into your own separate package (maybe called "fast-file-copy" or something like that) and then just including that as a dependency in Yarn? I imagine it's very useful outside of Yarn and could be used separately by other projects 😃 |
Nice! Do you think we should close this PR then, as it likely won't be merged in its current state? It'd be good to have one single "fast copy ALL THE THINGS" module that we can use. |
Yup closing it |
Summary
I create this PR more to open a discussion on what to do than to suggest this exact code.
ffi
is an annoying dependency but on win32 using the OS CopyFile provide a nice speedup in file copy operations versus doing it by hand.On the same hardware as the measurements done in #990 (comment) running
yarn install
10 times the results are :CopyFileW
Removed one third of the execution time (In this very specific sample).
Test plan
WIP ;-)