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

go: build each package single-threaded #53390

Merged
merged 3 commits into from Feb 7, 2019
Merged

Conversation

andrew-d
Copy link
Contributor

@andrew-d andrew-d commented Jan 4, 2019

Motivation for this change

I noticed that I was seeing the Go compiler build things in parallel even when I'd set -j1 --cores 1. It appears that the compiler, by default, uses the number of CPUs that are available to perform a build, while nixpkgs parallelizes at the directory level.

In order to change the fewest assumptions, this explicitly tells the Go compiler to run single-threaded. The flag's documentation is:

-p n
	the number of programs, such as build commands or
	test binaries, that can be run in parallel.
	The default is the number of CPUs available.

So this should function as expected. Feedback appreciated!

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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@@ -130,7 +130,7 @@ let
echo "$d" | grep -q "\(/_\|examples\|Godeps\)" && return 0
[ -n "$excludedPackages" ] && echo "$d" | grep -q "$excludedPackages" && return 0
local OUT
if ! OUT="$(go $cmd $buildFlags "''${buildFlagsArray[@]}" -v $d 2>&1)"; then
if ! OUT="$(go $cmd $buildFlags "''${buildFlagsArray[@]}" -v -p 1 $d 2>&1)"; then
Copy link
Member

Choose a reason for hiding this comment

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

mhm. Would it be better to just remove the parallel xargs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mic92 - I pushed a commit that removes the parallel xargs in favor of passing $NIX_BUILD_CORES through to the compiler. Thoughts? If the new approach looks good, I can squash the commits together.

@andrew-d
Copy link
Contributor Author

andrew-d commented Jan 4, 2019 via email

@andrew-d
Copy link
Contributor Author

andrew-d commented Jan 4, 2019

cc @wkennington, who originally wrote this code (assuming my git blame-fu is correct)

It looks like this was written prior to Go having support for parallel builds, so I think we should be fine to remove all the buildGoDir / xargs stuff and just let the Go compiler parallelize things appropriately?

@Mic92
Copy link
Member

Mic92 commented Jan 6, 2019

@GrahamcOfBorg build vgo2nix

@andrew-d
Copy link
Contributor Author

andrew-d commented Jan 6, 2019

@Mic92 - It looks like the build failed due to running out of space on the builder (EDIT: on x86_64-linux, looks like the aarch64-linux build succeeded, and the Darwin build timed out):

cannot link '/nix/store/.links/1zb3j2wvqcwgq3k6bhx63isr9qd4fm9ayf5c18qyxxcds643l468' to '/nix/store/292y1nd3595hcc444v4hn12z0k2mpqhl-go-1.11.4/share/go/src/cmd/compile/internal/types/type.go': No space left on device
cannot link '/nix/store/.links/15npiazz09z6wmsr57g7cmrf2vihjfa9dsry1ga413gvl1nriwh8' to '/nix/store/292y1nd3595hcc444v4hn12z0k2mpqhl-go-1.11.4/share/go/src/net/lookup_unix.go': No space left on device
cannot link '/nix/store/.links/1bq9gv5i4276pxsyn3nwqqzys47dmcp6vgp3is8pdny0anwdlzn8' to '/nix/store/292y1nd3595hcc444v4hn12z0k2mpqhl-go-1.11.4/share/go/pkg/linux_amd64_race/hash.a': No space left on device
cannot link '/nix/store/.links/1yxziwxr9w38y39l2xna0h73hj2wm5janva5n2752hf3l2p3lgav' to '/nix/store/292y1nd3595hcc444v4hn12z0k2mpqhl-go-1.11.4/share/go/pkg/linux_amd64/regexp.a': No space left on device

Any suggestions?

@Mic92
Copy link
Member

Mic92 commented Jan 9, 2019

I just re-test. I don't think it is the fault of your change.
@GrahamcOfBorg build vgo2nix

@Mic92
Copy link
Member

Mic92 commented Jan 9, 2019

maybe. I just test it myself locally.

@@ -167,7 +167,7 @@ let
if [ -z "$enableParallelBuilding" ]; then
export NIX_BUILD_CORES=1
fi
getGoDirs "" | xargs -n1 -P $NIX_BUILD_CORES bash -c 'buildGoDir install "$@"' --
Copy link
Member

@Mic92 Mic92 Jan 9, 2019

Choose a reason for hiding this comment

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

I think we still need -n1. However it would might be easier to just loop over getGoDirs in a bash for-loop since we no longer need parallel execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mic92 - I decided to keep the xargs instead of looping to avoid having to re-run the go compiler; if we're building a package with lots of Go modules, it avoids the startup time. I also want to keep the xargs command to ensure that we don't go past the maximum length of a command line for large packages.

Copy link
Member

Choose a reason for hiding this comment

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

What startup time do you avoid? xargs needs to start and its bash too. A for loop stays within the same process and does not have the argument length limitation:

for pkg in getGoDirs ""; do
  buildGoDir install "$pkg"
done

@andrew-d
Copy link
Contributor Author

andrew-d commented Feb 4, 2019

@Mic92 - Sorry for the delay here; life got busy 🙂 Just pushed a change that uses a for loop now.

Co-Authored-By: andrew-d <andrew@du.nham.ca>
@andrew-d
Copy link
Contributor Author

andrew-d commented Feb 7, 2019

@Mic92 - Done! Also, GitHub's "suggestions" feature is pretty cool 👍

@Mic92 Mic92 merged commit 274afc4 into NixOS:staging Feb 7, 2019
@andrew-d andrew-d deleted the andrew/go-cores branch February 17, 2019 23:31
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

3 participants