The behavior of passing NIX_BUILD_CORES=0 leads to a performance penalty when sourcing nixpkgs' generic builder's setup.sh, as setup.sh has to execute nproc.

This is rather surprising, since nproc is instantaneous for me:

$ command time nproc
24
0.00user 0.00system 0:00.00elapsed 100%CPU (0avgtext+0avgdata 1716maxresident)k

This patch (288 lines) seems overly complicated for such a trivial change. I would just do something like:

env["NIX_BUILD_CORES"] = fmt("%d", settings.buildCores ? settings.buildCores : Settings::getDefaultCores());

In particular, having both a unit test and a functional test seems overkill. (Functional tests are almost always preferred over unit tests.)

This is rather surprising, since nproc is instantaneous for me

It is in the millisecond range, as is sourcing setup.sh. It slowed it down by roughly 10-15% if I remember correctly.
The performance thing might be minor, but I guess we want nix anyways to determine the cores on most computers, not the builder.

seems overly complicated for such a trivial change

I applied your suggestion, which removed most code. Also I removed the unit tests.

but I guess we want nix anyways to determine the cores on most computers, not the builder.

So the issue that is addressed here is that the "builder" (a.k.a. the shell script) shouldn't need to "guess" the level of parallelism?

Also the special semantics of 0 should probably be documented. But what's the reasoning for changing the default value to 0? Doesn't that behave in the exact same way as getDefaultCores() with your other changes?

So the issue that is addressed here is that the "builder" (a.k.a. the shell script) shouldn't need to "guess" the level of parallelism?

Correct!

Also the special semantics of 0 should probably be documented

Done

But what's the reasoning for changing the default value to 0? Doesn't that behave in the exact same way as getDefaultCores() with your other changes?

Yes it behaves exactly the same, I just thought that having 0 as a default is easier to follow. But I can change it back if you prefer.

I just thought that having 0 as a default is easier to follow

It just seems that this does extra unnecessary work (getDefaultCores doesn't seem to cache the result). Though I'm not sure it would make a difference.

It also looks like getDefaultCores uses hardware_concurrency when it can't read cgroupfs. I wonder if that is correct, because:

https://en.cppreference.com/w/cpp/thread/thread/hardware_concurrency.html

Returns the number of concurrent threads supported by the implementation. The value should be considered only a hint.

So this really doesn't correspond to the number of logical cores. Do you think this could be a problem? Since nix itself would be the sole source of truth for the level of concurrency it's important to get the default right.

So this really doesn't correspond to the number of logical cores

On my system it does correctly return the number of logical cores trying the snippet of the reference you linked:

$ cat > main.c
#include <iostream>
#include <thread>

int main()
{
    unsigned int n = std::thread::hardware_concurrency();
    std::cout << n << " concurrent threads are supported.\n";
}

$ g++ main.c && ./main
16 concurrent threads are supported.

I have 8 physical cores and 16 logical ones, so it seems correct.

so it seems correct

Yeah, I suspect it does the right thing for most cases, though there might be some nuances.
At least libstdc++ does one of the following things:

https://github.com/gcc-mirror/gcc/blob/dd92d6acb416e138b21f00f34df54cb740e40e4c/libstdc%2B%2B-v3/src/c%2B%2B11/thread.cc#L50-L81

So I guess it's fine.

I have to correct one of my statements made in the original description above:

This affects all nixos machines since the introduction of the nix settings module a couple releases ago which sets build-cores to 0 be default.

This is not true, nixos was setting cores to 0 since quite a long time already. This means, so far only people who installed nix on foreign distros were actually using nix' builtin detection of cores.

Also this needs a release note.

Also this needs a release note.

Done

@DavHau, thanks! Also would you be interested in coordinating with @emilazy to push #11143 over the line? Now that the question of the semantics of cores = 0 has been further clarified.

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-2-31-0-released/68465/1