[bug#39728] [PATCH] Allow parallel downloads and builds

["Ludovic Courtès" <ludo@gnu.org>]

Hi!

Julien Lepiller <julien@lepiller.eu> 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 <julien@lepiller.eu>
> 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’.