setup-hooks/strip: parallelise stripping #207101

lheckemann · 2022-12-21T13:59:48Z

Description of changes

This makes bootstrapping to GNU hello ~1-2% faster on an 8-core machine and ~3-4% faster on a 64-core machine (unscientific comparison, total of 4 samples or something).

Things done

alyssais · 2022-12-21T14:01:43Z

pkgs/build-support/setup-hooks/strip.sh

@@ -62,7 +62,12 @@ stripDirs() {
    if [ -n "${paths}" ]; then
        echo "stripping (with command $cmd and flags $stripFlags) in $paths"
        # Do not strip lib/debug. This is a directory used by setup-hooks/separate-debug-info.sh.
-        find $paths -type f -a '!' -wholename "$prefix/lib/debug/*" -exec $cmd $stripFlags '{}' \; 2>/dev/null
+        find $paths -type f -a '!' -path "$prefix/lib/debug/*" -print0 |
+            xargs -r -0 -n1 -P "$NIX_BUILD_CORES" $cmd $stripFlags || exit_code=$?


This could be simplified to just this, right?

xargs -r -0 -n1 -P "$NIX_BUILD_CORES" $cmd $stripFlags || [[ $? -eq 123 ]]

I wonder even if we care about the exit code at all: do we care if all the files failed to strip? The old behavior did not:

$ find . -exec false \;; echo $? 0

I think we can || true this.

What Linus has here (and my suggestion also) does match the old behaviour. A 123 exit means that strip failed, and we are indeed ignoring that.

But any other error code from xargs means that xargs itself failed. We don't want to ignore that, as it likely means we're holding it wrong. Similarly, we weren't ignoring failures from find itself before.

can't push

diff --git a/pkgs/build-support/setup-hooks/strip.sh b/pkgs/build-support/setup-hooks/strip.sh index 8ee212be686..675cf2310a4 100644 --- a/pkgs/build-support/setup-hooks/strip.sh +++ b/pkgs/build-support/setup-hooks/strip.sh @@ -63,11 +63,12 @@ stripDirs() { echo "stripping (with command $cmd and flags $stripFlags) in $paths" # Do not strip lib/debug. This is a directory used by setup-hooks/separate-debug-info.sh. find $paths -type f -a '!' -path "$prefix/lib/debug/*" -print0 | - xargs -r -0 -n1 -P "$NIX_BUILD_CORES" $cmd $stripFlags || exit_code=$? - # xargs exits with status code 123 if some but not all of the - # processes fail. We don't care if some of the files couldn't - # be stripped, so ignore specifically this code. - [[ "$exit_code" = 123 || -z "$exit_code" ]] + xargs -r -0 -n1 -P "$NIX_BUILD_CORES" $cmd $stripFlags || [[ $? -eq 123 ]] + # ^ + # xargs exits with status code 123 if some but not all of the + # processes fail. We don't care if some of the files couldn't + # be stripped, so ignore specifically this code. + # 'strip' does not normally preserve archive index in .a files. # This usually causes linking failures against static libs like: # ld: ...-i686-w64-mingw32-stage-final-gcc-13.0.0-lib/i686-w64-mingw32/lib/libstdc++.dll.a:

I intentionally didn't apply this because I find it a little clearer and the comment feels less awkward this way, sorry I forgot to write that.

grahamc · 2022-12-21T14:29:52Z

Do we have another 1-4% improvement a few lines below?:

        find $paths -name '*.a' -type f -exec $ranlibCmd '{}' \; 2>/dev/null

lheckemann · 2022-12-21T17:03:10Z

@grahamc probably not, since there's a lot less to do:

[linus@geruest:~/scratch]$ nix-store -qR $(nix-instantiate '<nixpkgs>' -A hello) | xargs nix build
warning: you did not specify '--add-root'; the result might be removed by the garbage collector

[linus@geruest:~/scratch]$ find result*/ -name \*.a | wc -l
49

[linus@geruest:~/scratch]$ find result*/{bin,lib,libexec,lib64,sbin} -type f | wc -l
11357

We could do it as a matter of principle though.

EDIT: we can probably make savings like this on patchShebangs and patchELF though, I'll look into this.

pkgs/build-support/setup-hooks/strip.sh

SuperSandro2000 · 2023-06-29T14:34:23Z

Can we merge this or are there still things to do? 👀

lheckemann · 2023-06-30T14:33:37Z

@SuperSandro2000 I rebased it the other day and remove the extra parallelisation in hopes that that would make this mergeable as is :) I don't see any further things to address.

pkgs/build-support/setup-hooks/strip.sh

This makes bootstrapping to GNU hello ~1-2% faster on an 8-core machine and ~3-4% faster on a 64-core machine.

trofi · 2023-07-19T18:15:45Z

Bisect claims that 70945eb setup-hooks/strip: parallelise stripping broke config.system.build.extraUtils build in nixos (or a nixosTests.systemd-boot.basic, but it has bigger closure):

$ nix build -f. nixosTests.systemd-boot.basic -L
...
error: builder for '/nix/store/z49b3d7mj099bnggk224fpxsnsi6w9g7-extra-utils.drv' failed with exit code 1;
       last 10 log lines:
       > Copying libs for executable /nix/store/m1lq76v3jp321nyvf02x09v69xm97prh-extra-utils/bin/lvm
       > '/nix/store/ikvpyzyygbdvb5p5a27136r7glwf6rqm-systemd-minimal-253.6/lib/libsystemd.so.0.36.0' -> '/nix/store/m1lq76v3jp321nyvf02x09v69xm97prh-extra-utils/lib/libsystemd.so.0'
       > '/nix/store/51xaj8ksganbv0lj23df2yp6b79fsfhb-libaio-0.3.113/lib/libaio.so.1.0.2' -> '/nix/store/m1lq76v3jp321nyvf02x09v69xm97prh-extra-utils/lib/libaio.so.1'
       > Copying libs for executable /nix/store/m1lq76v3jp321nyvf02x09v69xm97prh-extra-utils/bin/scsi_id
       > Copying libs for executable /nix/store/m1lq76v3jp321nyvf02x09v69xm97prh-extra-utils/bin/systemd-sysctl
       > Copying libs for executable /nix/store/m1lq76v3jp321nyvf02x09v69xm97prh-extra-utils/bin/tune2fs
       > Copying libs for executable /nix/store/m1lq76v3jp321nyvf02x09v69xm97prh-extra-utils/bin/udevadm
       > Copying libs for executable /nix/store/m1lq76v3jp321nyvf02x09v69xm97prh-extra-utils/bin/v4l_id
       > Copying libs for executable /nix/store/m1lq76v3jp321nyvf02x09v69xm97prh-extra-utils/lib/ld-linux-x86-64.so.2
       > stripping (with command  and flags -s) in  /nix/store/m1lq76v3jp321nyvf02x09v69xm97prh-extra-utils/lib /nix/store/m1lq76v3jp321nyvf02x09v69xm97prh-extra-utils/bin
       For full logs, run 'nix log /nix/store/z49b3d7mj099bnggk224fpxsnsi6w9g7-extra-utils.drv'.

Might have something to do with multiple symlinks on the same binary.

Artturin · 2023-07-19T19:40:19Z

#244400 will print the error to make issues easier to debug

trofi · 2023-07-19T21:33:14Z

Thank you! That prints the following error:

extra-utils> stripping (with command  and flags -s) in  /nix/store/2gfapyy8nldpm7sj0mw88454fwzb995m-extra-utils/lib /nix/store/2gfapyy8nldpm7sj0mw88454fwzb995m-extra-utils/bin
extra-utils> find: 'standard output': Broken pipe
extra-utils> find: write error
extra-utils> xargs: option requires an argument -- 's'
extra-utils> Try 'xargs --help' for more information.

It looks like runCommand there uses stdenvNoCC rather than stdenv and probably uses busybox for find / xargs and friends.

trofi · 2023-07-19T21:34:07Z

$ nix-shell -p busybox

[nix-shell:~]$ busybox sh
[nix-shell:~]$ xargs --help
BusyBox v1.36.1 () multi-call binary.

Usage: xargs [OPTIONS] [PROG ARGS]

Run PROG on every item given by stdin

        -0      NUL terminated input
        -a FILE Read from FILE instead of stdin
        -o      Reopen stdin as /dev/tty
        -r      Don't run command if input is empty
        -t      Print the command on stderr before execution
        -p      Ask user whether to run each command
        -E STR,-e[STR]  STR stops input processing
        -I STR  Replace STR within PROG ARGS with input line
        -n N    Pass no more than N args to PROG
        -s N    Pass command line of no more than N bytes
        -P N    Run up to N PROGs in parallel
        -x      Exit if size is exceeded

Artturin · 2023-07-20T03:01:07Z

pkgs/build-support/setup-hooks/strip.sh

@@ -62,7 +62,12 @@ stripDirs() {
    if [ -n "${paths}" ]; then
        echo "stripping (with command $cmd and flags $stripFlags) in $paths"
        # Do not strip lib/debug. This is a directory used by setup-hooks/separate-debug-info.sh.
-        find $paths -type f -a '!' -wholename "$prefix/lib/debug/*" -exec $cmd $stripFlags '{}' \; 2>/dev/null
+        find $paths -type f -a '!' -path "$prefix/lib/debug/*" -print0 |
+            xargs -r -0 -n1 -P "$NIX_BUILD_CORES" $cmd $stripFlags 2>/dev/null || exit_code=$?


@trofi maybe a -- before cmd is needed?

Added to my PR, building.. nixosTests.systemd-boot.basic.config.nodes.machine.system.build.extraUtils

Now it fails with xargs: -s: No such file or directory

Aha there's no $cmd
stripping (with command and flags -s) in
echo "stripping (with command $cmd and flags $stripFlags) in $paths"

On current master(3ec081d) it has the same issue
extra-utils> stripping (with command and flags -s) in

so this issue was just being ignored!

trofi · 2023-07-30T08:03:58Z

I don't have a strong evidence, but I have a suspiction that we still manage to run multiple strip commands against a single binary if it's available through multiple symlinks: #246147 (comment)

I wonder if it would be safer to do realpath | uniq first to at least decrease workload on strip for heavily symlinked trees.

trofi · 2023-07-30T08:09:16Z

I'll try to test the following locally and propose it if it does not break stuff:

--- a/pkgs/build-support/setup-hooks/strip.sh
+++ b/pkgs/build-support/setup-hooks/strip.sh
@@ -68,6 +68,11 @@ stripDirs() {
         striperr="$(mktemp 'striperr.XXXXXX')"
         # Do not strip lib/debug. This is a directory used by setup-hooks/separate-debug-info.sh.
         find $paths -type f -a '!' -path "$prefix/lib/debug/*" -print0 |
+            # Make sure we process files under symlinks only once. Otherwise
+            # 'strip` can corrupt files when writes to them in parallel:
+            #   https://github.com/NixOS/nixpkgs/issues/246147#issuecomment-1657072039
+            xargs realpath | uniq |
+
             xargs -r -0 -n1 -P "$NIX_BUILD_CORES" -- $cmd $stripFlags 2>"$striperr" || exit_code=$?
         # xargs exits with status code 123 if some but not all of the
         # processes fail. We don't care if some of the files couldn't

trofi · 2023-07-30T08:46:25Z

Proposed the change as #246164

lheckemann requested a review from Ericson2314 as a code owner December 21, 2022 13:59

alyssais reviewed Dec 21, 2022

View reviewed changes

ofborg bot added 10.rebuild-darwin-stdenv 10.rebuild-linux-stdenv 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ labels Dec 21, 2022

alyssais approved these changes Dec 21, 2022

View reviewed changes

grahamc reviewed Dec 21, 2022

View reviewed changes

pkgs/build-support/setup-hooks/strip.sh Outdated Show resolved Hide resolved

alyssais added the 2.status: merge conflict label May 19, 2023

lheckemann requested a review from RaitoBezarius May 26, 2023 22:46

lheckemann force-pushed the parallel-strip branch from d7fe17e to fcab4d4 Compare June 28, 2023 09:59

ofborg bot removed the 2.status: merge conflict label Jun 28, 2023

Artturin mentioned this pull request Jun 30, 2023

setup-hooks/strip: parallelise stripping #240734

Closed

12 tasks

Artturin reviewed Jul 9, 2023

View reviewed changes

pkgs/build-support/setup-hooks/strip.sh Outdated Show resolved Hide resolved

setup-hooks/strip: parallelise stripping

70945eb

This makes bootstrapping to GNU hello ~1-2% faster on an 8-core machine and ~3-4% faster on a 64-core machine.

lheckemann force-pushed the parallel-strip branch from fcab4d4 to 70945eb Compare July 10, 2023 10:24

SuperSandro2000 approved these changes Jul 18, 2023

View reviewed changes

Artturin merged commit bba1411 into NixOS:staging Jul 18, 2023

lheckemann deleted the parallel-strip branch July 19, 2023 12:22

Artturin reviewed Jul 20, 2023

View reviewed changes

This was referenced Jul 20, 2023

nixos/stage-1: build extraUtils without compiler #213782

Merged

nixos/stage-1: fix stripping #244449

Merged

puckipedia mentioned this pull request May 14, 2024

mesa store path is haunted tpwrules/nixos-apple-silicon#199

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

setup-hooks/strip: parallelise stripping #207101

setup-hooks/strip: parallelise stripping #207101

lheckemann commented Dec 21, 2022

alyssais Dec 21, 2022 •

edited

Loading

grahamc Dec 21, 2022 •

edited

Loading

alyssais Dec 21, 2022

Artturin Jun 29, 2023 •

edited

Loading

Artturin Jun 30, 2023

lheckemann Jun 30, 2023

grahamc commented Dec 21, 2022

lheckemann commented Dec 21, 2022 •

edited

Loading

SuperSandro2000 commented Jun 29, 2023

lheckemann commented Jun 30, 2023 •

edited

Loading

trofi commented Jul 19, 2023

Artturin commented Jul 19, 2023

trofi commented Jul 19, 2023

trofi commented Jul 19, 2023

Artturin Jul 20, 2023

Artturin Jul 20, 2023 •

edited

Loading

Artturin Jul 20, 2023

Artturin Jul 20, 2023

Artturin Jul 20, 2023

Artturin Jul 20, 2023

trofi commented Jul 30, 2023

trofi commented Jul 30, 2023

trofi commented Jul 30, 2023

setup-hooks/strip: parallelise stripping #207101

setup-hooks/strip: parallelise stripping #207101

Conversation

lheckemann commented Dec 21, 2022

Description of changes

Things done

alyssais Dec 21, 2022 • edited Loading

Choose a reason for hiding this comment

grahamc Dec 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Artturin Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grahamc commented Dec 21, 2022

lheckemann commented Dec 21, 2022 • edited Loading

SuperSandro2000 commented Jun 29, 2023

lheckemann commented Jun 30, 2023 • edited Loading

trofi commented Jul 19, 2023

Artturin commented Jul 19, 2023

trofi commented Jul 19, 2023

trofi commented Jul 19, 2023

Choose a reason for hiding this comment

Artturin Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trofi commented Jul 30, 2023

trofi commented Jul 30, 2023

trofi commented Jul 30, 2023

alyssais Dec 21, 2022 •

edited

Loading

grahamc Dec 21, 2022 •

edited

Loading

Artturin Jun 29, 2023 •

edited

Loading

lheckemann commented Dec 21, 2022 •

edited

Loading

lheckemann commented Jun 30, 2023 •

edited

Loading

Artturin Jul 20, 2023 •

edited

Loading