[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#39728] [PATCH] Allow parallel downloads and builds
From: |
Ludovic Courtès |
Subject: |
[bug#39728] [PATCH] Allow parallel downloads and builds |
Date: |
Mon, 24 Feb 2020 22:23:45 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Hi!
Julien Lepiller <address@hidden> skribis:
> This patch allows to count builds and downloads separately. The idea is
> that downloads need bandwidth, but no CPU, while builds do not need
> bandwidth, but need CPU. With this patch, guix will be able to download
> substitutes while building unrelated packages. Currently, guix needs to
> wait for the download to finish before proceeding to the build. This
> should reduce the time of guix commands that need to build and download
> things at the same time.
>
> What do you think?
I think it’s a good idea!
I wonder what the UI will look like: (guix status) would no longer
display a progress bar when there’s more than on job (build or download)
taking place at the same time.
>>From 9c059d81ba4f4016f8c400b403f8c5edbdb160c2 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <address@hidden>
> Date: Fri, 21 Feb 2020 23:41:33 +0100
> Subject: [PATCH] nix: Count build and download jobs separately.
^
I’d write “daemon:” here. :-)
> This allows to run downloads (that take bandwith) and builds (that take
^
“This allows us”
> CPU time) independently from one another.
>
> * nix/nix-daemon/guix-daemon.cc: Add a max-download-jobs option.
> * nix/libstore/globals.hh: Add a maxDownloadJobs setting.
> * nix/libstore/globals.cc: Add a default value to it.
> * nix/libstore/build.cc: Manage build and download jobs separately.
For the final patch, please specify the entities changed (classes,
functions, etc.).
> + /* Number of download slots occupied. This includes substitution and
> + built-ins. */
> + unsigned int nrDownloads;
Note that not all builtins are downloads. Fixed-output derivations are
(usually) also downloads.
(It’d be the first time the daemon gets a notion of “download”. We
should make sure it doesn’t conflict with other assumptions.)
> /* Registers a running child process. `inBuildSlot' means that
> the process counts towards the jobs limit. */
> void childStarted(GoalPtr goal, pid_t pid,
> - const set<int> & fds, bool inBuildSlot, bool respectTimeouts);
> + const set<int> & fds, bool inBuildSlot, bool inDownloadSlot,
> + bool respectTimeouts);
How about replacing these two Booleans by a single enum?
> + unsigned int curDownloads = worker.getNrDownloads();
> + if (curDownloads >=
> (settings.maxDownloadJobs==0?1:settings.maxDownloadJobs) &&
> + fixedOutput) {
This is hard to parse and lacking spacing. :-) Perhaps make an
intermediate function or variable?
> +void Worker::waitForDownloadSlot(GoalPtr goal)
> +{
> + debug("wait for download slot");
> + if (getNrDownloads() <
> (settings.maxDownloadJobs==0?1:settings.maxDownloadJobs))
Same here.
> @@ -118,6 +119,7 @@ void Settings::update()
> {
> _get(tryFallback, "build-fallback");
> _get(maxBuildJobs, "build-max-jobs");
> + _get(maxDownloadJobs, "download-max-jobs");
We should also allow ‘set-build-options’ to set this option, as well as
add it to ‘%standard-build-options’. That can prolly come in a separate
patch.
> + { "max-downloads", 'D', n_("N"), 0,
> + n_("allow at most N download jobs") },
We’d need to update doc/guix.texi.
It would be great if you could test this patch for your daily usage. I
find it surprisingly easy to break things in the daemon. :-)
Thank you!
Ludo’.