Optimize buildActionsForCopy routine #2657

mourner · 2017-02-07T15:50:45Z

Summary

Makes linking faster by reducing the number of file system calls in the buildActionsForCopy routine which synchronizes existing node_modules tree with the ideal one.

In my test runs (on yarn itself, and on another big project), it saves about 400ms when running yarn without a lock file (or during upgrade).

Remove redundant fs.exists (in favor of lstat which is called later anyway)
Remove unnecessary fs.access (which does nothing — it's a placeholder in the code, so should be commented)
Remove deeper directory traversal for possibleExtraneous since it should be populated with the same values later in the queue anyway.
Avoid calling mkdirp if we know the destination folder already exists.

cc @bestander

Test plan

Not necessary — it doesn't change the results of exported functions. Let's see if this PR passes the CI tests.

mourner · 2017-02-08T08:18:34Z

In the tests, there's only one failure (resolving https://github.com/babel/babel-loader.git#greenkeeper/cross-env-3.1.4) and it seems unrelated to the PR changes because the same failure happens in other recent PRs.

bestander

That is very useful, thanks a lot for improving it.
Just a few nits

bestander · 2017-02-09T00:59:33Z

src/util/fs.js

+    let destStat;
+    try {
+      destStat = await lstat(dest);
+    } catch (e) {}


there needs to be an explanation for a muted exception.
Could you add a one line comment?

Oh, sure. Basically, fs.exists is deprecated in Node, which recommends using fs.stat with error handling instead. In this case destStat will be undefined if error occurs during lstat.

bestander · 2017-02-09T00:59:52Z

src/util/fs.js

+      // EINVAL access errors sometimes happen which shouldn't because node shouldn't be giving
+      // us modes that aren't valid. investigate this, it's generally safe to proceed.
+
+      // if (srcStat.mode !== destStat.mode) {


Forgot to remove?

I thought it was left there as something to investigate in future, so I left it as a comment. Should I remove?

mourner · 2017-02-09T12:04:05Z

@bestander addressed your first comment and rebased on master, so this PR should be good to go. Regarding the read mode leftover — it was there before my changes (as sort of a TODO to investigate), so I'm leaving it there as a comment. @kittens introduced it in 4bbc653.

mourner · 2017-02-09T12:21:44Z

The AppVeyor build failed with the following error, which seems unrelated to the PR changes:

should expose npm_config_argv environment variable to lifecycle scripts for back compatibility with npm (#684)

bestander · 2017-02-14T14:06:29Z

src/util/fs.js

+      // EINVAL access errors sometimes happen which shouldn't because node shouldn't be giving
+      // us modes that aren't valid. investigate this, it's generally safe to proceed.
+
+      /* if (srcStat.mode !== destStat.mode) {


So what should we do with this code?
No reason to keep it commented out.

If no one from the Yarn team is willing to investigate cases where it's unsafe to proceed when the file read modes don't match, I'm OK removing the code. I'd leave the text comment above though, just in case.

This commit was added by @kittens in 4bbc653.
I'll ping him to give some context

Ping me in 2 days if there is no answer, then let's just merge with this code removed.

@bestander Ping 😉

Ok, let's remove the commented out code and get back to it if we see errors reported.

mourner · 2017-03-24T10:44:55Z

Sorry for the late response — I should probably wait before master starts passing CI tests more reliably before rebasing and suggesting to merge. Most recent commits in master fail on all servers.

bestander · 2017-04-07T18:02:10Z

We have a few flaky tests but master got stable enough to get the signal if a PR is breaking something, so let's give this one one more try.

bestander · 2017-04-07T18:02:21Z

@mourner, would you rebase?

mourner · 2017-04-10T10:59:34Z

@bestander yes, will do!

mourner · 2017-04-21T12:31:56Z

@bestander sorry for getting out of the loop. I see that Travis started having a failed test after this merge — do you think this is a consequence of this PR or an unrelated intermittent failure?

FAIL  __tests__/commands/add.js (174.708s)
  ● add with new dependency should be deterministic 3

arcanis · 2017-04-21T14:13:34Z

It's probably related, previous tests weren't subject to this (it's usually timeout that kill our tests)

mourner · 2017-04-21T15:33:32Z

OK, submitting a PR to revert a few lines that caused the failure until we figure out what's going on.

sebmck · 2017-04-21T20:24:38Z

There's a bunch of lint warnings on master introduced by this PR, could you open a PR to fix them?

bestander requested changes Feb 9, 2017

View reviewed changes

mourner added 2 commits February 9, 2017 13:52

optimize buildActionsForCopy routine

c9207d8

more resilient sync in util.fs, add comments

d77af50

mourner force-pushed the optimize-sync branch from 86e5e69 to d77af50 Compare February 9, 2017 12:00

bestander reviewed Feb 14, 2017

View reviewed changes

bestander merged commit 9a6afe1 into yarnpkg:master Apr 20, 2017

mourner added a commit to mourner/yarn that referenced this pull request Apr 21, 2017

partially revert yarnpkg#2657 to fix add command test

a9a389c

mourner mentioned this pull request Apr 21, 2017

Partially revert #2657 to fix add command test #3222

Merged

mourner deleted the optimize-sync branch April 21, 2017 15:34

sebmck pushed a commit that referenced this pull request Apr 21, 2017

partially revert #2657 to fix add command test (#3222)

ba45162

This was referenced Apr 28, 2017

yarn-homebrew #4 failed #3275

Closed

yarn-homebrew #5 failed #3304

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize buildActionsForCopy routine #2657

Optimize buildActionsForCopy routine #2657

mourner commented Feb 7, 2017

mourner commented Feb 8, 2017

bestander left a comment

bestander Feb 9, 2017

mourner Feb 9, 2017

bestander Feb 9, 2017

mourner Feb 9, 2017

mourner commented Feb 9, 2017

mourner commented Feb 9, 2017

bestander Feb 14, 2017

mourner Feb 14, 2017 •

edited

Loading

bestander Feb 14, 2017

bestander Feb 14, 2017 •

edited

Loading

TimvdLippe Feb 21, 2017

bestander Feb 21, 2017 •

edited

Loading

mourner commented Mar 24, 2017

bestander commented Apr 7, 2017

bestander commented Apr 7, 2017

mourner commented Apr 10, 2017

mourner commented Apr 21, 2017

arcanis commented Apr 21, 2017

mourner commented Apr 21, 2017

sebmck commented Apr 21, 2017

Optimize buildActionsForCopy routine #2657

Optimize buildActionsForCopy routine #2657

Conversation

mourner commented Feb 7, 2017

mourner commented Feb 8, 2017

bestander left a comment

Choose a reason for hiding this comment

bestander Feb 9, 2017

Choose a reason for hiding this comment

mourner Feb 9, 2017

Choose a reason for hiding this comment

bestander Feb 9, 2017

Choose a reason for hiding this comment

mourner Feb 9, 2017

Choose a reason for hiding this comment

mourner commented Feb 9, 2017

mourner commented Feb 9, 2017

bestander Feb 14, 2017

Choose a reason for hiding this comment

mourner Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

bestander Feb 14, 2017

Choose a reason for hiding this comment

bestander Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

TimvdLippe Feb 21, 2017

Choose a reason for hiding this comment

bestander Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

mourner commented Mar 24, 2017

bestander commented Apr 7, 2017

bestander commented Apr 7, 2017

mourner commented Apr 10, 2017

mourner commented Apr 21, 2017

arcanis commented Apr 21, 2017

mourner commented Apr 21, 2017

sebmck commented Apr 21, 2017

mourner Feb 14, 2017 •

edited

Loading

bestander Feb 14, 2017 •

edited

Loading

bestander Feb 21, 2017 •

edited

Loading