Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid subshells #69131

Merged
merged 7 commits into from Sep 20, 2019
Merged

Avoid subshells #69131

merged 7 commits into from Sep 20, 2019

Conversation

xzfc
Copy link
Contributor

@xzfc xzfc commented Sep 20, 2019

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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
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

@ofborg ofborg bot added the 6.topic: stdenv Standard environment label Sep 20, 2019
Copy link
Contributor Author

@xzfc xzfc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

eval "${!hookName}"
else
return "$def"
fi
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

@@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Mic92 commented Sep 20, 2019

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

@xzfc
Copy link
Contributor Author

xzfc commented Sep 20, 2019

@Mic92 It is ready for review.

@Mic92
Copy link
Member

Mic92 commented Sep 20, 2019

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why go to single [[?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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}";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Ericson2314
Copy link
Member

@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.

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

(cherry picked from commit 268d510)

NixOS#69131
matthewbauer added a commit that referenced this pull request Oct 16, 2019
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Oct 22, 2019
Avoid subshells

(cherry picked from commit 268d510)
@nixos-discourse
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants