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

Make "Linking dependencies" faster on Windows (in some cases) #2958

Merged
merged 1 commit into from
Apr 3, 2017

Conversation

vbfox
Copy link
Contributor

@vbfox vbfox commented Mar 20, 2017

Summary

Motivation: Yarn is slow on one of my windows machines (but not others) with Linking dependencies... that can take minutes, so I tried to find the cause.

Turns out that Yarn was sometimes always copying every file because it's hiting this node bug: nodejs/node#2069 that is windows specific.

In fs.js, fs.utimes is used after the file copy in copyBulk to set the mtime and checked to avoid extra copies in buildActionsForCopy but due to the bug the dates won't match even for files that were just copied by Yarn.

The bug is subtle because it doesn't reproduce on a clean system as Yarn would have downloaded the packages and copied the extracted version to the cache. In this case it would use the same buggy utimes version and will set the milliseconds of mtime for the source in the cache. But if the file modification time is changed for any reason (Antivirus maybe ?), Yarn will never be able to determine that they are the same again and will always do the copy.

Test plan

  • Installed nodejs v6.9.5 (To be sure as it was released before the fix could be present anywhere, but 6.10 has the problem too)
  • Ensured that the modification times for all files in my yarn cache was fresh and with a milliseconds part that was non-zero (From the bash on windows command line: find . -type f -exec touch {} +)
  • Created multiple copies of a repository for test containing a real project.json and it's yarn.lock (Simple react/redux/gulp project)
  • Ran yarn add lodash using the released version (0.21.3) in one dir and measured the time over a few runs : ~14s, the progress bar in Linking dependencies... dominates the time
    2017-03-20 22_39_09-vs 2015
  • Ran yarn add lodash using my branch in one dir and measured the time over a few runs: ~6s, the progress bar in Linking dependencies... starts & finish near immediately
    2017-03-20 22_38_20-vs 2015

@vbfox vbfox changed the title Avoid node issue with utimes on Windows Make "Linking dependencies" faster on Windows (in some cases) Mar 20, 2017
@vbfox
Copy link
Contributor Author

vbfox commented Mar 28, 2017

If anyone with the bug want to make it's yarn install temporarily faster here is a powershell to "fix" all files in the cache.

It doesn't completly solve the problem but it's a lot faster after :)

$epoch = (Get-Date -Date "1970-01-01 00:00:00Z").ToUniversalTime()
Function GetJsTime
{
    Param ([datetime]$date)
    [long](($date -$epoch).TotalMilliseconds)
}

Function TimeFromJs
{
    Param ([long]$date)
    $epoch + [System.TimeSpan]::FromMilliseconds($date)
}

Function ZeroMs
{
    Param ([long]$date)
    ([long]($date / 1000)) * 1000
}

$cache = [string](&yarn cache dir)
Get-ChildItem $cache -Recurse -File | Foreach-Object { $_.LastWriteTimeUtc = TimeFromJs (ZeroMs (GetJsTime ($_.LastWriteTimeUtc))) }

And if you aren't limited to LTS just upgrade to nodejs 7.1.0+ 😉 still the easiest solution.

Lot of nodejs versions are buggy on windows regarding utime : It's not
setting the milliseconds part.

Issue: nodejs/node#2069

With this change file date comparison is now done with only seconds
precision on windows when copying files. It make linking fast again under
windows.
@Daniel15
Copy link
Member

Daniel15 commented Apr 3, 2017

This looks good to me! Is it possible to add a unit test for this? You could mock os.platform in the test by overriding it (os.platform = () => 'win32')

@Daniel15 Daniel15 merged commit bc128d1 into yarnpkg:master Apr 3, 2017
@vbfox
Copy link
Contributor Author

vbfox commented Apr 3, 2017

@Daniel15 yep, i'll add it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants