Update: use fs.copyFile when available #4486

BYK · 2017-09-16T23:12:50Z

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:

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.

**Summary** Fixes #4331. Supercedes #3290. Uses the newly added `fs.copyFile` on Node 8.5 hen available and falls back to 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 fsCopy | w/ fsCopy | | - | - | - | | ~23s | ~19s | ~15s | This is with `yarn.lock` in place and w/o `node_modules`. **Test plan** CI should pass.

BYK · 2017-09-16T23:17:48Z

src/util/fs.js

@@ -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> {


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

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.

buildsize · 2017-09-16T23:26:05Z

This change will increase the build size from 9.69 MB to 9.69 MB, an increase of 2.27 KB (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	834.67 KB	834.92 KB	258 bytes (0%)
yarn-[version].js	3.69 MB	3.69 MB	748 bytes (0%)
yarn-legacy-[version].js	3.74 MB	3.74 MB	874 bytes (0%)
yarn-v[version].tar.gz	838.74 KB	838.96 KB	227 bytes (0%)
yarn_[version]all.deb	634.81 KB	635.03 KB	222 bytes (0%)

arcanis · 2017-09-16T23:18:06Z

src/util/fs.js

+  : async (src, dest, flags, data) => {
+      const fd = await open(dest, 'w', data.mode);
+      try {
+        await write(fd, await readFileBuffer(src));


Why not writeFile?

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?

arcanis · 2017-09-16T23:27:25Z

src/util/fs.js

@@ -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> {


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.

arcanis · 2017-09-16T23:29:52Z

src/util/fs.js

-      while ((writePromise = currentlyWriting[data.dest])) {
+    async (data: CopyFileAction): Promise<void> => {
+      const writePromise = currentlyWriting.get(data.dest);
+      if (writePromise) {


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.

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?

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?).

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.

BYK · 2017-09-17T00:41:31Z

Oh boy, dat speed:

cpojer · 2017-09-17T12:03:43Z

src/util/fs.js

@@ -40,8 +40,34 @@ 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;


How did you pick these numbers?

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?

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.

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.

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

cpojer · 2017-09-17T12:04:56Z

src/util/fs.js

+      const copier = safeCopyFile(data, () => currentlyWriting.delete(data.dest));
+      currentlyWriting.set(data.dest, copier);
+      events.onProgress(data.dest);
+      return copier;


I read: cpojer. lol

cpojer · 2017-09-17T12:05:39Z

Wow, awesome speedup for Yarn!

langpavel · 2017-09-17T12:34:49Z

Does node support shallow copy for copy on write filesystems like btrfs? https://btrfs.wiki.kernel.org/index.php/Deduplication
It can not only speed up operation but also save disk space

BYK · 2017-09-17T12:36:58Z

@langpavel I think the new fs.copyFile will be able to leverage that, yeah.

BYK · 2017-09-17T13:37:44Z

Another record after setting concurrency to 128 on Linux & Node 8:

vjeux · 2017-09-17T15:35:59Z

I'm curious, for node < 8.5 could you shell out to "cp" and if you do, do you get the same wins?

BYK · 2017-09-17T16:33:09Z

I'm curious, for node < 8.5 could you shell out to "cp" and if you do, do you get the same wins?

Why you hate cross-platform? :( (Windows)

BYK · 2017-09-17T16:43:56Z

@vjeux also "shelling out" would probably come with its own costs which may make this option a lot less desirable.

sciolist · 2017-09-17T19:37:22Z

I did give shelling out to cp a look, and you could get similar speed benefits.. but you'd pretty much need to prepare one massive bash (+ powershell for windows) script with all the copies you want to run and then execute that, to offset the startup cost.. especially on Windows where it's very, very slow. Terribly ugly.

Daniel15 · 2017-09-17T22:57:04Z

@langpavel

Does node support shallow copy for copy on write filesystems like btrfs?

Not yet, but coming soon! See libuv/libuv#1491 and the discussion on libuv/libuv#1465

Daniel15 · 2017-09-17T23:09:46Z

src/util/fs.js

+ * `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: Function): Promise<void> {


Use () => void instead of Function for the type of cleanup.

Daniel15 · 2017-09-17T23:10:36Z

src/util/fs.js

+// 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;


I wonder if 128 will essentially lock up the system on slower hard drives (particularly if the user is multitasking)

It shouldn't since this is still single threaded and we leave the scheduling job to the OS and Node as far as I understand. The reason for having a much lower limit for the fallback is mostly old versions of Node having buffering issues with pipe sometimes and possible memory pressure after #3539 since it now reads the whole file contents into memory.

If this becomes a problem we can always make this a configurable variable.

cpojer · 2017-09-18T12:49:10Z

🎉

bestander · 2017-09-18T16:48:19Z

441: 2017-09-19のJS: Node.js 8.5.0、MSEdge 16の変更点、CoffeeScript 2 r=azu a=azu Node - [Node v8.5.0 (Current) | Node.js](https://nodejs.org/en/blog/release/v8.5.0/) - [Update: use fs.copyFile when available by BYK · Pull Request #4486 · yarnpkg/yarn](yarnpkg/yarn#4486) Edge - [What's Coming in EdgeHTML 16 - Microsoft Edge Development | Microsoft Docs](https://docs.microsoft.com/en-us/microsoft-edge/dev-guide/whats-new/edgehtml-16) - [Making the web smoother with independent rendering - Microsoft Edge Dev BlogMicrosoft Edge Dev Blog](https://blogs.windows.com/msedgedev/2017/08/17/making-web-smoother-independent-rendering/#d8sy0bGIZkZ6gF01.97) - [Microsoft Edge at Build 2017 - Microsoft Edge Dev BlogMicrosoft Edge Dev Blog](https://blogs.windows.com/msedgedev/2017/05/19/microsoft-edge-at-build-2017/#CwDc45Kwq1K8M2VW.97) - [独立レンダリングパイプラインを大幅改良：Windows 10 Fall Creators Updateの「Microsoft Edge」は「EdgeHTML 16」アップデートで何がどれだけ速くなる？ - ＠IT](http://www.atmarkit.co.jp/ait/articles/1708/22/news068.html) Coffee - [Announcing CoffeeScript 2](http://coffeescript.org/announcing-coffeescript-2/)

**Summary** Follow up to #4486 which reverted the while loop that waits on potential multiple copies of the same file. This seems to have some random breakages and needs more investigation for optimizing. **Test plan** N/A

**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.

**Summary** Follow up to yarnpkg#4486 which reverted the while loop that waits on potential multiple copies of the same file. This seems to have some random breakages and needs more investigation for optimizing. **Test plan** N/A

langpavel · 2018-03-05T22:58:15Z

For CoW file systems support see #5456

droslean · 2023-08-23T15:20:05Z

@BYK I found this PR while trying to understand the memory usage of yarn. Is there any way to disable this feature and not use any cache at all? Instead, I would assume yarn to download the packages directly to the node_modules directory instead of using the default cache folder and then copy-paste millions of files.

BYK requested review from cpojer, arcanis, bestander and kaylie-alexa September 16, 2017 23:12

BYK changed the title ~~Update: use fs.copyFile when awailable~~ Update: use fs.copyFile when available Sep 16, 2017

BYK force-pushed the copyFile branch from 8ac7c00 to 110d43a Compare September 16, 2017 23:14

BYK mentioned this pull request Sep 16, 2017

use fcopy for faster file copying #3290

Closed

BYK commented Sep 16, 2017

View reviewed changes

arcanis reviewed Sep 16, 2017

View reviewed changes

BYK added 3 commits September 17, 2017 00:51

Don't wait on writePromise

c7c0072

More readable code

c38b00f

Fix bug in Node < 7

58f6438

cpojer reviewed Sep 17, 2017

View reviewed changes

BYK added 2 commits September 17, 2017 14:24

Increase copy concurrency and add comment about where it came from

a42adbd

Merge branch 'master' into copyFile

b84289a

zkat mentioned this pull request Sep 17, 2017

unpack: open file handle only once isaacs/node-tar#142

Closed

Daniel15 reviewed Sep 17, 2017

View reviewed changes

Daniel15 approved these changes Sep 17, 2017

View reviewed changes

BYK added 2 commits September 18, 2017 10:25

Use better function flow type

06ce7f5

Fix lint

3a57109

BYK merged commit 0ee1c55 into master Sep 18, 2017

BYK deleted the copyFile branch September 18, 2017 12:47

azu mentioned this pull request Sep 19, 2017

2017-09-19のJS: Node.js 8.5.0、MSEdge 16の変更点、CoffeeScript 2 jser/jser.github.io#441

Merged

segayuu mentioned this pull request Sep 19, 2017

[perfomance] Support native copyFile. hexojs/hexo-fs#8

Closed

OlegLustenko mentioned this pull request Sep 19, 2017

[FEATURE] [PERFORMANCE] Adaptation fs.copyFile npm/npm#18579

Open

serima mentioned this pull request Sep 26, 2017

ci: Migrate to CircleCI 2.0 with Workflows #4271

Merged

BYK mentioned this pull request Sep 28, 2017

Fix: bring back the aggressive concurrency protection #4573

Merged

This was referenced Sep 29, 2017

Use fs.copyFileSync if available broccolijs/node-copy-dereference#3

Closed

Use fs.copyFileSync if available chadhietala/broccoli-rollup#32

Merged

zkat mentioned this pull request Oct 2, 2017

feat: add benchmark with lots of files pnpm/benchmarks-of-javascript-package-managers#11

Merged

bestander mentioned this pull request Dec 7, 2017

[in docker] yarn (linking) touches module's files without need #4764

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: use fs.copyFile when available #4486

Update: use fs.copyFile when available #4486

BYK commented Sep 16, 2017 •

edited

Loading

BYK Sep 16, 2017

arcanis Sep 16, 2017

buildsize bot commented Sep 16, 2017 •

edited

Loading

arcanis Sep 16, 2017

BYK Sep 16, 2017

arcanis Sep 16, 2017

arcanis Sep 16, 2017

BYK Sep 16, 2017

arcanis Sep 16, 2017

BYK Sep 16, 2017

BYK commented Sep 17, 2017 •

edited

Loading

cpojer Sep 17, 2017

BYK Sep 17, 2017

cpojer Sep 17, 2017

BYK Sep 17, 2017

BYK Sep 17, 2017

cpojer Sep 17, 2017

cpojer commented Sep 17, 2017

langpavel commented Sep 17, 2017

BYK commented Sep 17, 2017

BYK commented Sep 17, 2017

vjeux commented Sep 17, 2017

BYK commented Sep 17, 2017

BYK commented Sep 17, 2017

sciolist commented Sep 17, 2017 •

edited

Loading

Daniel15 commented Sep 17, 2017

Daniel15 Sep 17, 2017

Daniel15 Sep 17, 2017

BYK Sep 18, 2017

BYK Sep 18, 2017

cpojer commented Sep 18, 2017

bestander commented Sep 18, 2017

langpavel commented Mar 5, 2018

droslean commented Aug 23, 2023

Update: use fs.copyFile when available #4486

Update: use fs.copyFile when available #4486

Conversation

BYK commented Sep 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buildsize bot commented Sep 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BYK commented Sep 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Sep 17, 2017

langpavel commented Sep 17, 2017

BYK commented Sep 17, 2017

BYK commented Sep 17, 2017

vjeux commented Sep 17, 2017

BYK commented Sep 17, 2017

BYK commented Sep 17, 2017

sciolist commented Sep 17, 2017 • edited Loading

Daniel15 commented Sep 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Sep 18, 2017

bestander commented Sep 18, 2017

langpavel commented Mar 5, 2018

droslean commented Aug 23, 2023

BYK commented Sep 16, 2017 •

edited

Loading

buildsize bot commented Sep 16, 2017 •

edited

Loading

BYK commented Sep 17, 2017 •

edited

Loading

sciolist commented Sep 17, 2017 •

edited

Loading