generic-builder: optimize - reduce execve calls when sourcing #417132
Conversation
18fcc19 to
dba8df4
Compare
There was a problem hiding this comment.
I love this!
One request: please put this new test in tests.stdenv, as that's what I run in order to see whether all parts of the stdenv work. The generic builder is an integral part of the stdenv in my view.
For the performance test, consider using hyperfine.
For the non-performance section of the test, please extract it into its own test. It's not performance testing -- it's correctness!
This improves the implementation of `dumpVars` by removing a call to `install`. This improves performance when sourcing setup.sh by more than 10% It should also improve the performance when transitioning between build phases significantly as no process executions are issued anymore, A test is added, which ensures that no extra execve calls are issued while sourcing setup.sh.
|
Maybe having the benchmark in tree is a bit too noisy anyways. What's important is the test that counts the execve calls. I factored it out and put it in It can now be built via |
|
|
I bisected that this broke Here is the minimal reproducible nix file With this file I could run: git bisect run nix build -f docker-tools-test.nixThe hoops is there to reduce the build time during bisecting as the error occurs inside the
export 2>/dev/null > "$NIX_BUILD_TOP/env-vars"being changed from: export 2>/dev/null >| "$NIX_BUILD_TOP/env-vars"However every iteration of this takes massive amount of time. |
|
This patch solved the issue, mainly the So seems somehow export is failing and that wreaks havoc. |
|
@DavHau and @philiptaron what do you think? Would this fix be correct, or does this need more investigation? |
|
I don't think it's correct. I don't have enough information from your problem report to understand why it's failing...
I don't think I'd merge a change to |
if enableFakechroot then
''
proot -r $PWD/old_out ${bind-paths} --pwd=/ fakeroot bash -e -c '
if [ -e "$NIX_ATTRS_SH_FILE" ]; then . "$NIX_ATTRS_SH_FILE"; fi
source $stdenv/setup
eval "$fakeRootCommands"
tar \
--sort name \
--exclude=./dev \
--exclude=./proc \
--exclude=./sys \
--exclude=.${builtins.storeDir} \
--numeric-owner --mtime "@$SOURCE_DATE_EPOCH" \
--hard-dereference \
-cf $out/layer.tar .
'
''
else
''
fakeroot bash -e -c '
if [ -e "$NIX_ATTRS_SH_FILE" ]; then . "$NIX_ATTRS_SH_FILE"; fi
source $stdenv/setup
cd old_out
eval "$fakeRootCommands"
tar \
--sort name \
--numeric-owner --mtime "@$SOURCE_DATE_EPOCH" \
--hard-dereference \
-cf $out/layer.tar .
'
''
Adding a Alternatively add it to |
|
Assuming I bisected correctly, for some reason this breaks nix-shell for me, maybe I have an old version of Nix? |
This broke with NixOS#417132 where the previous behaviour of suppressing errors of the export into $NIX_BUILD_TOP/env-vars when dumping env vars. This error seems to stem from the non-existance of $NIX_BUILD_TOP. This fix ensures the NIX_BUILD_TOP propagates into the proot environment.
|
@deliciouslytyped Your nix-shell probably also doesn't have Edit: I don't seem to be able to reproduce an error with your command. What error do you get? |
This broke with NixOS#417132 where the previous behaviour of suppressing errors of the export into $NIX_BUILD_TOP/env-vars when dumping env vars. This error seems to stem from the non-existance of $NIX_BUILD_TOP. This fix ensures the NIX_BUILD_TOP propagates into the proot environment.
|
There is no visible error, what I see is what I posted. |
|
I forgot NIX_DEBUG can have a higher value set and thought it wasn't working. Indeed the failure hook seems to be triggered in dumpVars at
I don't understand what you mean by this. |
|
Ok yeah, @philiptaron @DavHau the code before the patch had an Edit: It exists since 2015 nixpkgs/pkgs/stdenv/generic/setup.sh Line 453 in 774f74b |
|
@terlar I was probably unclear about this, the issue is that the failure hook is called and nix-shell immediately exits, the shell environment isn't entered. |
This resolves the spate of user reports about strange failures in various derivations. See NixOS#417132 (comment) and related.
This resolves the spate of user reports about strange failures in various derivations. See NixOS#417132 (comment) and related.
|
I proposed a fix for |
This improves the implementation of
dumpVarsby removing a call toinstall.This improves performance when sourcing setup.sh by more than 10%
It should also improve the performance when transitioning between build phases significantly as no process executions are issued anymore,
A test is added, which ensures that no extra execve calls are issued while sourcing setup.sh.
This test can also be used to benchmark the sourcing of different implementations of setup.sh.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.