This implements a special back-compat shim to specifically allow
unbracketed IPv6 addresses in store references. This is something
that is relied upon in the wild and the old parsing logic accepted
both ways (brackets were optional). This patch restores this behavior.
As always, we didn't have any tests for this.
Addresses #13937.
`perf c2c` shows a lot of cacheline conflicts between purely read-only
Store methods (like `parseStorePath()`) and the Sync classes. So
allocate pathInfoCache separately to avoid that.
Calling `drainFD()` will hang if another process has the write side
open, since then the child won't get an EOF. This can happen if we
have multiple threads doing a build, since in that case another thread
may fork a child process that inherits the write side of the first
thread.
We could set O_CLOEXEC on the write side (using pipe2()) but it won't
help here since we don't always do an exec() in the child, e.g. in the
case of builtin builders. (We need a "close-on-fork", not a
"close-on-exec".)
This is relied upon (specifically the `local` store) by existing
tooling [1] and we broke this in 3e7879e6df (which
was first released in 2.31).
To lessen the scope of the breakage we should not normalize "auto" references
and explicitly specified references like "local" or "daemon". It also makes
sense to canonicalize local://,daemon:// to be more compatible with prior
behavior.
[1]: 05e1b3cba2/lib/NOM/Builds.hs (L60-L64)
Exactly why is is correct is a little subtle, because sometimes the
worker is owned by the worker. But the commit message in
e437b08250 explained the situation well
enough: I made that commit message part of the ABI docs, and now it
should be understandable to the next person.
Do this with a new `useHook` boolean we carefully make sure is set in
all cases. This change isn't really worthwhile by itself, but it allows
us to make further refactors (see later commits) which are
well-motivated.
When useMaster is true, startMaster() acquires the state lock, then
calls isMasterRunning(), which calls addCommonSSHOpts(), which tries
to acquire the state lock again, causing a deadlock.
The solution is to move tmpDir out of the state. It doesn't need to be
there in the first place because it never changes.
The URL should not be normalized before handing it off to cURL, because
builtin fetchers like fetchTarball/fetchurl are expected to work with
arbitrary URLs, that might not be RFC3986 compliant. For those cases
Nix should not normalize URLs, though validation is fine. ParseURL and
cURL are supposed to match the set of acceptable URLs, since they implement
the same RFC.
Looking at perf:
0.21 │ push %rbp
0.99 │ mov %rsp,%rbp
│ push %r15
0.25 │ push %r14
│ push %r13
0.49 │ push %r12
0.66 │ push %rbx
1.23 │ lea -0x10000(%rsp),%r11
0.23 │ 15: sub $0x1000,%rsp
1.01 │ orq $0x0,(%rsp)
59.12 │ cmp %r11,%rsp
0.27 │ ↑ jne 15
Seems like 64K is too much to have on the stack for each invocation, considering
that only a minuscule number of allocations are actually larger than 4K.
There's actually no good reason this function should use so much stack space. Or
use small_string at all. Everything can be done in small chunks that don't require
any memory allocations and use up 2K bytes on the stack.
This patch also adds a microbenchmark for tracking the unparsing performance. Here
are the results for this change:
(Before)
BM_UnparseRealDerivationFile/hello 7275 ns 7247 ns 96093 bytes_per_second=232.136Mi/s
BM_UnparseRealDerivationFile/firefox 40538 ns 40376 ns 17327 bytes_per_second=378.534Mi/s
(After)
BM_UnparseRealDerivationFile/hello 3228 ns 3218 ns 215671 bytes_per_second=522.775Mi/s
BM_UnparseRealDerivationFile/firefox 39724 ns 39584 ns 17617 bytes_per_second=386.101Mi/s
This translates into nice evaluation performance improvements (compared to 18c3d2348f):
Benchmark 1: GC_INITIAL_HEAP_SIZE=8G old-nix/bin/nix-instantiate ../nixpkgs -A nixosTests.gnome --readonly-mode
Time (mean ± σ): 3.111 s ± 0.021 s [User: 2.513 s, System: 0.580 s]
Range (min … max): 3.083 s … 3.143 s 10 runs
Benchmark 2: GC_INITIAL_HEAP_SIZE=8G result/bin/nix-instantiate ../nixpkgs -A nixosTests.gnome --readonly-mode
Time (mean ± σ): 3.037 s ± 0.038 s [User: 2.461 s, System: 0.558 s]
Range (min … max): 2.960 s … 3.086 s 10 runs
It is only done in the `force = true` case, and the only
`cleanupBuild(true)` call is right after where it used to be, so this
has the exact same behavior as before.
Calling `reset` on this `std::optional` field of `DerivationBuilderImpl`
is also what the (automatically created) destructor of
`DerivationBuilderImpl` will do. We should be making sure that the
derivation builder is cleaned up by the goal anyways, and if we do that,
then this `Finally` is no longer needed.
Before, had a very ugly `appendLogTailErrorMsg` callback. Now, we
instead have a `fixupBuilderFailureErrorMessage` that is just used by
`DerivationBuildingGoal`, and `DerivationBuilder` just returns the raw
data needed by this.
