Limit concurrency to 2 in a CI environment and 8 otherwise

[lukechilds]

The new default of using CPU cores for concurrency works well most of the time, however when running tests in Docker (Travis) it will use the CPU count of the host machine, not what's been allotted to the container.

Discovered when the Got tests where timing out for no apparent reason. I ran os.cpus().length on the container and it returned 32. Capping at 4 resolved the issue.

This PR changes AVA to use whichever is lower out of os.cpus().length or 4 for concurrency by default. This seems like a sensible soft limit. You can still manually specify a concurrency that is larger than 4.

[sindresorhus]

Relevant Travis issue: travis-ci/travis-ci#4696

I think we should only cap it to 4 when process.env.CI exists. (Maybe even lower? CIs can be pretty slow) When not on a CI, we could cap it at maybe 6?

[lukechilds]

So maybe 2 on CI? That way we don't hold everything up for one slow test file. Even one CPU should be fine swapping between two threads.

And when not on a CI I'd say go even further. Maybe cap at 8. Majority of the time if this is run locally it'll be on bare metal so os.cpus().length will be safe to use and limit to machine thread count anyway.

Although we'll encounter the same issue on local machines if you're running your tests in docker. But then users are less likely to have a large number of threads (most likely 4 - 8) so that's unlikely to cause issues.

Alternatively, as this seems mostly related to Docker, we could set the limits with an is-docker check. Otherwise, trust os.cpus().length`.

[sindresorhus]

Alright, so let's cap it at 2 for CI and 8 for normal.

@novemberborn Thoughts?

[lukechilds]

Done, can be rewritten as

let concurrency = Math.min(os.cpus().length, process.env.CI ? 2 : 8);

but thought maybe that's too much for one line?

[sindresorhus]

You're not assigning this to anything.

[lukechilds]

Woops, sorry running on 2 hours sleep 😬

[sindresorhus]

but thought maybe that's too much for one line?

It looks fine. That's how I would have written it.

[sindresorhus]

Can you also mention the capping behavior in the readme?

[lukechilds]

Yeah, done.

Also in the docs:

By default, AVA will use as many processes as there are CPU cores in your machine.

Technically this should be threads not cores. Do you want me to change that?

[sindresorhus]

Technically this should be threads not cores. Do you want me to change that?

No, it's CPU cores:

The os.cpus() method returns an array of objects containing information about each CPU/core installed. - https://nodejs.org/api/os.html#os_os_cpus

[lukechilds]

Node.js docs are misleading, it's definitely threads:

$ sysctl -n machdep.cpu.brand_string
Intel(R) Core(TM) i5-5257U CPU @ 2.70GHz

$ sysctl -n machdep.cpu.core_count
2

$ sysctl -n machdep.cpu.thread_count
4

$ node -e "console.log(require('os').cpus().length)"
4

[sindresorhus]

I see. Alright, update it to threads.

[sindresorhus]

Node.js issue: nodejs/node#16279

[sindresorhus]

Let's go with logical cores per Node.js issue discussion and make logical cores link to https://superuser.com/questions/1105654/logical-vs-physical-cpu-performance

[lukechilds]

Good idea, done 👍

[novemberborn]

👍 on the CI change, but I don't understand why we'd need to cap this at all for non-CI?

[sindresorhus]

but I don't understand why we'd need to cap this at all for non-CI?

For cases where Docker is used locally since it reports the incorrect count. And, from experience, anything more than 4-5 doesn't improve the performance. But yes, it's pretty arbitrary, so not sure whether it makes sense.

[lukechilds]

@novemberborn Just to clarify, it only caps the default from os.cpus().length. You can still manually specify whatever concurrency you want for CI or local.

But yeah, like I mentioned in an above comment, it's unlikely to cause issues locally unless they have a really high thread count.

Majority of the time if this is run locally it'll be on bare metal so os.cpus().length will be safe to use and limit to machine thread count anyway.

Although we'll encounter the same issue on local machines if you're running your tests in docker. But then users are less likely to have a large number of threads (most likely 4 - 8) so that's unlikely to cause issues.

[novemberborn]

@lukechilds @sindresorhus OK, I'm still leaning towards not capping this outside of a CI environment. Just seems like unnecessary logic.

[sindresorhus]

Ok, let's drop it. We can add it back if it's an actual proven problem.

[novemberborn]

@lukechilds I've taken the liberty to push these changes. Now also using is-ci rather than directly checking the environment.

[lukechilds]

@novemberborn was not aware of is-ci. Looks like a more robust solution 👍

[novemberborn]

Thanks @lukechilds!