use close instead of finish event for file copy completion

[sciolist]

Currently performance of yarn on Windows with Windows Defender is not great, especially for large projects. One of the causes of this is fs.copyBulk invoking fs.utimes prior to closing the file being copied.

Windows updates a files mtime when its handle closes, so if yarns utime call completes before the os has had time to actually close the file, yarns times will be overwritten. Windows Defender makes this issue far more noticeable since it runs its scan before the handle is closed causing nearly every file to lose yarns mtime. The end result is that most files in node_modules will be recopied on every single yarn command if Windows Defender is enabled. (If WD is disabled the issue remains, but it does stabilize as yarn gets lucky and updates mtimes after windows.)

By using the close event of writeStream rather than finish, the utimes-call occurs after Windows has had time to wrap things up.

[bestander]

Oh, nice.
Did you have a chance to measure speed impact?

[sciolist]

Hey. I did run quite a lot of tests, but threw away the data. :)

Ran a few more basic tests (just averaging over 5 runs per test).. also all these tests include #3235 and are run on yarn v0.23.2.

package.json has.. ~100 deps, ~50000 files (200mb) in node_modules after install.

using FINISH, node_modules SCANNED by windows defender:
    yarn --offline             - 180s # fresh install, with yarn.lock, empty node_modules
    yarn add leftpad --offline - 164s # add a new dependency
    yarn add leftpad --offline - 156s # re-add an already installed dependency
    yarn remove leftpad        - 171s # remove an installed dependency

using FINISH, node_modules IGNORED by windows defender:
    yarn --offline             - 103s
    yarn add leftpad --offline -  55s
    yarn add leftpad --offline -  49s 
    yarn remove leftpad        -  46s

using CLOSE, node_modules SCANNED by windows defender:
    yarn --offline             - 158s
    yarn add leftpad --offline -  15s
    yarn add leftpad --offline -  15s
    yarn remove leftpad        -  16s

using CLOSE, node_modules IGNORED by windows defender:
    yarn --offline             -  71s
    yarn add leftpad --offline -  15s
    yarn add leftpad --offline -  15s
    yarn remove leftpad        -  16s

in the 'using FINISH, node_modules IGNORED by windows defender' test there was a lot of variance depending on luck, if some other process started hitting disk it'd fail at setting more of the dates which would slow it down.

Windows Defender still messes with the initial install, and I doubt there's much that can be done about that.. But working with large projects after initial install is a lot faster. I may look into what's making it still take 15 seconds, but, stat is pretty slow on windows so it's not all that surprising. For comparison I ran the same test on os x:

    yarn --offline             -  37s
    yarn add leftpad --offline -   7s
    yarn add leftpad --offline -   7s
    yarn remove leftpad        -   8s

[bestander]

Wow, that is a huge help to the project. Thanks a lot for doing it!

…

On Wed, 26 Apr 2017 at 22:53, Daniel Pihlstrom ***@***.***> wrote: Hey. I did run quite a lot of tests, but threw away the data. :) Ran a few more basic tests (just averaging over 5 runs per test).. also all these tests include #3235 <#3235> and are run on yarn v0.23.2. package.json has.. ~100 deps, ~50000 files (200mb) in node_modules after install. using FINISH, node_modules SCANNED by windows defender: yarn --offline - 180s # fresh install, with yarn.lock, empty node_modules yarn add leftpad --offline - 164s # add a new dependency yarn add leftpad --offline - 156s # re-add an already installed dependency yarn remove leftpad - 171s # remove an installed dependency using FINISH, node_modules IGNORED by windows defender: yarn --offline - 103s yarn add leftpad --offline - 55s yarn add leftpad --offline - 49s yarn remove leftpad - 46s using CLOSE, node_modules SCANNED by windows defender: yarn --offline - 158s yarn add leftpad --offline - 15s yarn add leftpad --offline - 15s yarn remove leftpad - 16s using CLOSE, node_modules IGNORED by windows defender: yarn --offline - 71s yarn add leftpad --offline - 15s yarn add leftpad --offline - 15s yarn remove leftpad - 16s in the 'using FINISH, node_modules IGNORED by windows defender' test there was a lot of variance depending on luck, if some other process started hitting disk it'd fail at setting more of the dates which would slow it down. Windows Defender still messes with the initial install, and I doubt there's much that can be done about that.. But working with large projects after initial install is a lot faster. I may look into what's making it still take 15 seconds, but, stat is pretty slow on windows so it's not all that surprising. For comparison I ran the same test on os x: yarn --offline - 37s yarn add leftpad --offline - 7s yarn add leftpad --offline - 7s yarn remove leftpad - 8s — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#3234 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWDkx_MWQIjvL-DEK3Q3JxDtf1j6Bks5rz7zVgaJpZM4NFVGy> .

[sciolist]

Not a problem.

It is possible to add windows defender exclusions through their PS cmdlets.. but that may feel a bit outside the scope of yarn. I made a quick test of listing, adding and removing exclusion paths over here: https://github.com/sciolist/node-undefender

Tracking antivirus software doesn't seem like something yarn should get involved in, but it is an issue that's likely to affect every single windows user, leading them to either think that yarn is slower than it could be, or searching and hopefully finding one of the github issues that explains the problem.

[bestander]

Yeah, this probably should not be inside Yarn codebase.
Although I keep dreaming of ecosystem of plugins around Yarn that could enable such things

[bestander]

On the other hand, the code to add exclusions is so tiny https://github.com/sciolist/node-undefender/blob/master/exclusions.js that maybe it is worth adding to Yarn.

[bestander]

So, anyone reading this, send a PR