Avoid subshells #69131

xzfc · 2019-09-20T03:53:12Z

Motivation for this change

Subshells take a significant amount of time during nix-shell startup process.
This PR replaces the most overly used subshell invocations in setup.sh and bintools-wrapper.

Benchmark

time nix-shell -I nixpkgs=/path/to/nixpkgs -p stdenv --run :
# 0.7s → 0.4s

time nix-shell -I nixpkgs=/path/to/nixpkgs -p i3.buildInputs --run :
# 8.0s → 1.3s

Things done

Further work

If someone wants to hunt remaining subshell invocations, there is a possible starting point:

Inside nix-shell:

time bash $stdenv/setup

PS4=$'+\x1b[31m(${BASH_SOURCE}:${LINENO}): ${FUNCNAME[0]:+${FUNCNAME[0]}(): }\x1b[m' strace -f -e process bash -x $stdenv/setup
# ^ search for `clone(` in the output

Notify maintainers

cc @Ericson2314 @matthewbauer

xzfc

Comment on commit: setup.sh: avoid subshells: shopt -po nounset:

TL;DR: replacing

local oldOpts="$(shopt -po nounset)"
# ...
eval "$oldOpts"

with

local oldOpts="-u"
shopt -qo nounset || oldOpts="+u"
# ...
set "$oldOpts"

Test example

#!/usr/bin/env bash

set -eu
set -o pipefail

orig() {
	local oldOpts="$(shopt -po nounset)"
	echo -n "... "
	eval "$oldOpts"
}

new() {
	local oldOpts="-u"
	shopt -qo nounset || oldOpts="+u"
	echo -n "... "
	set "$oldOpts"
}

echo orig
set -u; orig; shopt -po nounset || true
set +u; orig; shopt -po nounset || true
echo
echo new
set -u; new; shopt -po nounset || true
set +u; new; shopt -po nounset || true

xzfc · 2019-09-20T04:02:49Z

pkgs/stdenv/generic/setup.sh

+        eval "${!hookName}"
+    else
+        return "$def"
+    fi


Comment on _callImplicitHook:
This is tricky. Following assumption is taken:
$hookName can't be an alias, builtin or keyword. It's either a function, or a file, or a variable, or the default value is returned.

Test example

#!/usr/bin/env bash set -eu set -o pipefail orig() { local hookName=$1 case "$(type -t "$hookName")" in (function|alias|builtin) echo function;; (file) echo file;; (keyword) :;; (*) if [ -z "${!hookName:-}" ]; then echo default else echo variable fi;; esac } new() { local hookName=$1 if declare -F "$hookName" > /dev/null; then echo function elif type -p "$hookName" > /dev/null; then echo file elif [ -n "${!hookName:-}" ]; then echo variable else echo default fi } dummyfunc() { :; } dummyvar=value echo orig orig dummyfunc orig ./05.sh orig bash orig dummyvar orig nonexistent echo echo new new dummyfunc new ./05.sh new bash new dummyvar new nonexistent

xzfc · 2019-09-20T04:05:50Z

pkgs/stdenv/generic/setup.sh

@@ -96,7 +95,7 @@ _callImplicitHook() {
 # hooks exits the hook, not the caller. Also will only pass args if
 # command can take them
 _eval() {
-    if [ "$(type -t "$1")" = function ]; then
+    if declare -F "$1" > /dev/null 2>&1; then


Test example

#!/usr/bin/env bash set -eu set -o pipefail orig() { local rc=0 [ "$(type -t "$1")" = function ] || rc=$? echo $rc } new() { local rc=0 declare -F "$1" > /dev/null 2>&1 || rc=$? echo $rc } dummyfunc() { :; } alias dummyalias=dummyfunc dummystmt='if [ -z "$noAuditTmpdir" -a -e "$prefix" ]; then auditTmpdir "$prefix"; fi' echo orig orig dummyfunc orig dummyalias orig echo orig pwd orig "$dummystmt" echo echo new new dummyfunc new dummyalias new echo new pwd new "$dummystmt"

xzfc · 2019-09-20T04:08:18Z

pkgs/stdenv/generic/setup.sh

@@ -449,7 +449,8 @@ findInputs() {
            [[ -f "$pkg/nix-support/$file" ]] || continue

            local pkgNext
-            for pkgNext in $(< "$pkg/nix-support/$file"); do
+            read -r -d '' pkgNext < "$pkg/nix-support/$file" || true
+            for pkgNext in $pkgNext; do


Test example

#!/usr/bin/env bash set -eu set -o pipefail orig() { local pkgNext for pkgNext in $(< "$1"); do echo -n " [$pkgNext]" done echo } new() { local pkgNext read -r -d '' pkgNext < "$1" || true for pkgNext in $pkgNext; do echo -n " [$pkgNext]" done echo } printf 'foo ba\\r baz\nabc def\n\n123' > /tmp/testfile1 printf 'foo ba\\r baz\nabc def\n\n123\n' > /tmp/testfile2 echo orig orig /tmp/testfile1 orig /tmp/testfile2 echo new new /tmp/testfile1 new /tmp/testfile2

Mic92 · 2019-09-20T08:12:06Z

Is this ready for review or do you still working on it?

xzfc · 2019-09-20T09:14:01Z

@Mic92 It is ready for review.

Mic92 · 2019-09-20T14:27:33Z

Looks good to me, maybe @zimbatm could also have a look.

Ericson2314 · 2019-09-20T15:25:20Z

pkgs/build-support/bintools-wrapper/setup-hook.sh

@@ -24,7 +24,8 @@ bintoolsWrapper_addLDVars () {
        # Python and Haskell packages often only have directories like $out/lib/ghc-8.4.3/ or
        # $out/lib/python3.6/, so having them in LDFLAGS just makes the linker search unnecessary
        # directories and bloats the size of the environment variable space.
-        if [[ -n "$(echo $1/lib/lib*)" ]]; then
+        local -a glob=( $1/lib/lib* )
+        if [ "${#glob[*]}" -gt 0 ]; then


Why go to single [[?

Just my personal preference. In this case both can be used.

Since we are not trying to be POSIX compatible I think it's best to always use [[ ]] and pretend that [ ] don't exist.

For example here you can use [[ "${#glob[*]}" > 0 ]]. That syntax also reacts better with missing quotes. Eg:

foo="x -o true" [ x = $foo ] # => 0 [[ x = $foo ]] # => 1

Ericson2314 · 2019-09-20T15:27:21Z

pkgs/build-support/bintools-wrapper/setup-hook.sh

-        export "${role_pre}${upper_case}=@targetPrefix@${cmd}";
-        export "${upper_case}${role_post}=@targetPrefix@${cmd}";
+        export "${role_pre}${cmd^^}=@targetPrefix@${cmd}";
+        export "${cmd^^}${role_post}=@targetPrefix@${cmd}";


I originally did something like this, but people complained on darwin that nix-shell uses the ambient bash which is old as hell. But Apple is switching to zsh anyways I think? And also I think supporting ambient bashes is stupid; fix the usability a different way.

I don't think we need to worry about builtin bash anyway. It can be an issue with bootstrapping, but we just need to tell people to build a newer Bash before they build Nix from source.

Oh! running nix-shell shows that it is not impure anymore yay!!!

I think this was only ever a problem in the impure stdenv though. It would be good to document our minimum Bash version though. I think all of these new features were added in Bash 4?

Ericson2314 · 2019-09-20T15:29:44Z

pkgs/stdenv/generic/setup.sh

@@ -17,7 +17,8 @@ fi
 # code). The hooks for <hookName> are the shell function or variable
 # <hookName>, and the values of the shell array ‘<hookName>Hooks’.
 runHook() {
-    local oldOpts="$(shopt -po nounset)"
+    local oldOpts="-u"
+    shopt -qo nounset || oldOpts="+u"


Hahahaha nice trick. I missed it until @matthewbauer pointed it out.

Ericson2314 · 2019-09-20T15:30:03Z

pkgs/stdenv/generic/setup.sh

@@ -17,7 +17,8 @@ fi
 # code). The hooks for <hookName> are the shell function or variable
 # <hookName>, and the values of the shell array ‘<hookName>Hooks’.
 runHook() {
-    local oldOpts="$(shopt -po nounset)"
+    local oldOpts="-u"
+    shopt -qo nounset || oldOpts="+u"
    set -u # May be called from elsewhere, so do `set -u`.


It would be nice if we could just stop set +u-ing in nixpkgs! Wanna tackle that next? :)

Ericson2314 · 2019-09-20T15:35:10Z

@xzfc Great work! This is doubly useful for a) making things fast b) convincing people we need to stop using bash. I heartily approve of both.

Now that it's clear we don't need to support bash 3. Let's also use -A associative arrays rather than the glob dedup trick.

Avoid using subshells (cherry picked from commit 268d510) NixOS#69131

Backport pull request #69131 from xzfc/subshells

Avoid subshells (cherry picked from commit 268d510)

nixos-discourse · 2021-10-19T16:41:08Z

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

https://discourse.nixos.org/t/wrapping-activation-scripts-into-subshell/15564/2

xzfc added 7 commits September 19, 2019 09:54

setup.sh: avoid subshells: shopt -po nounset

463463b

setup.sh: avoid subshells: type -t

6f024f6

setup.sh: avoid subshells: mapOffset

d53920a

setup.sh: avoid subshells: type -t in _callImplicitHook

cf4e482

setup.sh: avoid subshells: iterating a file

42482a1

bintools-wrapper: avoid subshells: upcase

bbc5b22

bintools-wrapper: avoid subshells: glob match

2654701

xzfc requested review from Ericson2314 and matthewbauer as code owners September 20, 2019 03:53

ofborg bot added the 6.topic: stdenv Standard environment label Sep 20, 2019

xzfc commented Sep 20, 2019

View reviewed changes

ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux-stdenv This PR causes stdenv to rebuild 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ labels Sep 20, 2019

xzfc commented Sep 20, 2019

View reviewed changes

Ericson2314 reviewed Sep 20, 2019

View reviewed changes

matthewbauer merged commit 268d510 into NixOS:staging Sep 20, 2019

matthewbauer mentioned this pull request Sep 26, 2019

Slow ghc nix-shell entry time, any way to speed up? reflex-frp/reflex-platform#492

Open

matthewbauer added a commit to matthewbauer/nixpkgs that referenced this pull request Oct 15, 2019

Backport pull request NixOS#69131 from xzfc/subshells

2d81865

Avoid using subshells (cherry picked from commit 268d510) NixOS#69131

matthewbauer mentioned this pull request Oct 15, 2019

Backport pull request #69131 from xzfc/subshells #71196

Merged

10 tasks

matthewbauer added a commit that referenced this pull request Oct 16, 2019

Merge pull request #71196 from matthewbauer/avoid-subshells-19-09

d3a928f

Backport pull request #69131 from xzfc/subshells

dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Oct 22, 2019

Merge pull request NixOS#69131 from xzfc/subshells

dd90e05

Avoid subshells (cherry picked from commit 268d510)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid subshells #69131

Avoid subshells #69131

xzfc commented Sep 20, 2019 •

edited

Loading

xzfc left a comment •

edited

Loading

xzfc Sep 20, 2019

xzfc Sep 20, 2019

xzfc Sep 20, 2019

Mic92 commented Sep 20, 2019

xzfc commented Sep 20, 2019

Mic92 commented Sep 20, 2019

Ericson2314 Sep 20, 2019

xzfc Sep 21, 2019

zimbatm Sep 21, 2019

zimbatm Sep 21, 2019

Ericson2314 Sep 20, 2019

matthewbauer Sep 20, 2019

Ericson2314 Sep 20, 2019

matthewbauer Sep 20, 2019

xzfc Oct 22, 2019

Ericson2314 Sep 20, 2019

Ericson2314 Sep 20, 2019 •

edited

Loading

Ericson2314 commented Sep 20, 2019

nixos-discourse commented Oct 19, 2021

Avoid subshells #69131

Avoid subshells #69131

Conversation

xzfc commented Sep 20, 2019 • edited Loading

Motivation for this change

Benchmark

Things done

Further work

Notify maintainers

xzfc left a comment • 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

Mic92 commented Sep 20, 2019

xzfc commented Sep 20, 2019

Mic92 commented Sep 20, 2019

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

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

Ericson2314 Sep 20, 2019 • edited Loading

Choose a reason for hiding this comment

Ericson2314 commented Sep 20, 2019

nixos-discourse commented Oct 19, 2021

xzfc commented Sep 20, 2019 •

edited

Loading

xzfc left a comment •

edited

Loading

Ericson2314 Sep 20, 2019 •

edited

Loading