Fixes resolution concurrency problem

[arcanis]

Should fix #3023 for sure, possibly #3364, #3490, and #3489 as well.

Two notes:

• I thought this would make perfs worse by a large factor, but actually it barely had any impact at all, even when running without lockfile. On a project requiring react-native, react, react-dom and webpack, over a hundred tries, it went from Avg:11s to Avg:12s overall.

• I tried to write an integration test, but unfortunately couldn't automatically reproduce from inside the test environment get different version dependencies between before there is a yarn.lock file and after. #3023 - for some reason I always ended up getting 2.7.5 instead of the bogus 2.8.0 :( However, I was able to systematically reproduce it manually, so I'm confident that this PR should fix this issue.

[bestander]

That is bold :)

[arcanis]

That makes me think - maybe it could be interesting to have "perf regression tests", to make sure that we don't unknowingly introduce big regressions.

[bestander]

Definitely a good idea to have some performance regression tracking.

@trueadm, do you track performance regressions on OSS React with some framework?

[arcanis]

On Yoga we just run a script with some stats. Maybe we could do this kind of thing with a simple bash script?

[bestander]

@arcanis, this change removes parallelism from top level resolutions, but parallelism still exists for second and deeper layer of dependencies https://github.com/yarnpkg/yarn/blob/master/src/package-request.js#L348.

You'll probably want to make them in sequence as well.
In that case I expect to see an increased resolution time especially for large projects with 100s of dependencies to resolve but it may depend on your network latency.

Can you add data to Test Plan even for the manual tests you perform?
For a true test you'd want to add an artificial network latency to your system.

[bestander]

I've had this test for #3023 https://github.com/yarnpkg/yarn/pull/3563/files#diff-c2dc3bd2d0bdc7b8d87c36ed3039c2d4R300

[arcanis]

You'll probably want to make them in sequence as well.

Right!

In that case I expect to see an increased resolution time especially for large projects with 100s of dependencies to resolve but it may depend on your network latency.

I'd expect it too, but apparently there's little change. It even seems faster, weird observation I don't completely understand (the following numbers are obtained with a build that includes the fix inside package-request):

Sync resolution (master):

❯ [mael-mbp?] /tmp/foolol ❯ python -m timeit -v -n 1 -r 10 -s 'import os' 'os.system("(export PATH=~/yarn/bin:$PATH; rm -rf node_modules yarn.lock && yarn) >& /dev/null")'

raw times: 56.8 27.3 24.6 26.1 24.7 26.6 25.5 25.4 57.7 26.2
1 loops, best of 10: 24.6 sec per loop

Async resolution (0.24.3):

❯ [mael-mbp?] /tmp/foolol ❯ python -m timeit -v -n 1 -r 10 -s 'import os' 'os.system("(export PATH=~/not-yarn/bin:$PATH; rm -rf node_modules yarn.lock && yarn) >& /dev/null")'

raw times: 28.9 30.5 29.8 32.9 33.9 30.4 32.6 27.5 30.8 30.5
1 loops, best of 10: 27.5 sec per loop

[bestander]

That looks all right when you are in corporate network, what if you are on a 3G grade network with significant latency (500ms)?
You can test it with Network Link Conditioner http://nshipster.com/network-link-conditioner/ on mac.

I think we should do it a bit smarter and go sequential only if there is a lockfile already with all patterns resolved.
Otherwise we can allow some randomness on first-come-first-serve basis although with smarter post processing we could eliminate it too.

[arcanis]

The following numbers are from a custom build with fetching / linking disabled. We can see there's some slowdown on very slow networks (+- 4s in average, +17%), but it looks quite small, and probably insignificant compared to the time required to run the fetching + linking steps.

I'd like to avoid complexifying the code by adding another parameter "is the lockfile new?", that would only be a workaround that would eventually become legacy. Ideally, I think the tree should be resolved asynchronously without any regard for any kind of context-sensitive optimizations (optimization passes being then ran on this tree), and I plan to address that in a followup PR. It should fix your concerns. WDYT?

3G sync:

raw times: 27.2 27.5 27.6 27.3 27.6 28.3 28 28.3 27.5 27
1 loops, best of 10: 27 sec per loop

3G async:

raw times: 23.4 24.1 23.6 23.5 23.9 23.5 23.3 23.7 23.5 48.6
1 loops, best of 10: 23.3 sec per loop

[trueadm]

@bestander in regards to how we track performance regressions on React, we don't have a formal process in place yet, more an ad-hoc one. We've implemented some benchmarking tools that allow us to do this using Lighthouse: https://github.com/facebook/react/tree/master/scripts/bench

[arcanis]

I'll merge this PR to fix the concurrency issues that make the install unstable, and try to figure out a more secure way to improve this next week.

[bestander]

@arcanis, you need to add a test too otherwise we won't notice regressions later
Can you copy https://github.com/yarnpkg/yarn/pull/3563/files#diff-c2dc3bd2d0bdc7b8d87c36ed3039c2d4R300?