Default concurrency

[sindresorhus]

In #791, we added a flag to control the concurrency. It turned out operating systems are really bad at doing their job properly, and they couldn't handle the amount of processes we tried to start up when there's a lot of test files. We also have the overhead starting up node and requiring dependencies in each process. This was a big problem in React projects where you do one test file per component, and you usually have a lot of components.

We should consider doing a default concurrency limit. Users should still be able to set it to unlimited, but I think it would be in our best interest to adapt to reality. I've previously had good success with (os.cpus().length || 1) * 2, but we should get some real numbers on it from various systems.

Everyone, please run your test suite with the --concurrency flag and try out different numbers for it and post back the results. Also include the number of test files in the suite and the output of:

Edit: Let's wait on this until #945 is merged and released, per #966 (comment).

node -p "require('os').cpus().length"

@avajs/core Thoughts?

[jamestalmage]

I think this is blocked by #923 - but yeah, seems reasonable to me.

Also, I think we should get #945 merged and released before spending a lot of time collecting data on this. I think it will have a dramatic impact on those using --babel-require, and possibly shift the ideal concurrency higher. I think disk and memory contention are equally as likely to be the culprit as CPU contention.

Use time ava on OSX / Linux and post all timings

[sindresorhus]

@jamestalmage Agreed. I've updated the issue text.

[doug-wade]

dougwade redfin.npm/core-ui ‹dbw-core-ui-80-percent-coverage› » time ava -c 10

   116 passed
ava -c 10  113.98s user 7.87s system 590% cpu 20.639 total
dougwade redfin.npm/core-ui ‹dbw-core-ui-80-percent-coverage› » time ava -c 15

   116 passed
ava -c 15  117.13s user 8.00s system 727% cpu 17.197 total
dougwade redfin.npm/core-ui ‹dbw-core-ui-80-percent-coverage› » time ava -c 5

   116 passed
ava -c 5  76.16s user 6.22s system 395% cpu 20.853 total
dougwade redfin.npm/core-ui ‹dbw-core-ui-80-percent-coverage› » time ava

   116 passed
ava  99.11s user 7.69s system 706% cpu 15.107 total
dougwade redfin.npm/core-ui ‹dbw-core-ui-80-percent-coverage› » time ava -c 7

   116 passed
ava -c 7  83.85s user 6.78s system 548% cpu 16.520 total
dougwade redfin.npm/core-ui ‹dbw-core-ui-80-percent-coverage› » time ava -c 6

   116 passed
ava -c 6  79.69s user 6.51s system 522% cpu 16.486 total

looks like 5 is a pretty good default; maybe 6 is better? Adding concurrency: 6 to my package.json is ~18% faster

dougwade redfin.npm/core-ui ‹dbw-core-ui-80-percent-coverage› » time ava

   116 passed
ava  94.35s user 7.38s system 715% cpu 14.213 total
dougwade redfin.npm/core-ui ‹dbw-core-ui-80-percent-coverage*› » vim package.json
dougwade redfin.npm/core-ui ‹dbw-core-ui-80-percent-coverage*› » time ava

   116 passed
ava  79.91s user 6.57s system 534% cpu 16.188 total

My intuition is that the best default is likely Math.floor((os.cpus().length || 1) * 1.5)

[dananichev]

Yeah, #945 is deal-breaker. Right now babel-require consumes a lot of resources. I believe it would drastically improve performance once shipped. Concurrency usually is not that bad (comparing to #945)

[jonathanrdelgado]

Is this still being blocked? (#945 was closed). I'm happy to submit a PR to get some sane concurrency defaults.

[sindresorhus]

@jonathandelgado It has the downside of making test.only() only work per file and not globally. See caveat in #791.

@novemberborn Do you still think we shouldn't do this? What is your current opinion?

[novemberborn]

We should do this, yes. It just completely breaks test.only(). I've been musing about how to "fix" that and it roughly comes down to:

• Refactor reporters so they keep a full buffer of their output, and can rewrite that buffer (kinda Write reporter updates on a fixed interval #939)
• When exclusive tests are detected, remove previous results from the output
• Ensure stdout/stderr from workers is routed through the reporters
• Cache tests which in the last run had exclusive tests, and start those first
• The above but better for watch mode

This would give us the experience of exclusive tests while not having to load all test files in advance (which then gives us other scheduling benefits).

It's not easy though… perhaps what we should do in the mean time is show an instruction when t.only() is used with multiple tests, and encourage users to run just that test file instead. And then we can run all tests within a worker pool and remove the current behavior in its entirety.

[sindresorhus]

This would give us the experience of exclusive tests while not having to load all test files in advance (which then gives us other scheduling benefits).

It's a cool idea, but I think it's also too clever and not worth all the work and potential downsides. I'd rather spend energy on making it easier to filter down tests.

[novemberborn]

It's a cool idea, but I think it's also too clever and not worth all the work and potential downsides. I'd rather spend energy on making it easier to filter down tests.

Agreed.