Subject: Re: [bug#69291] [PATCH 4/5] scripts: substitute: Untangle selecting fast vs small compressions.

From: Ludovic Courtès <[email-masked]>
Date: Fri, 23 Feb 2024 17:26:43 +0100

Christopher Baines <[email-masked]> skribis:

> Pulling the logic up to the script makes this code more portable and not
> reliant on setting a global variable.
>
> * guix/scripts/substitute.scm (%prefer-fast-decompression?): Rename to…
> (%default-prefer-fast-decompression?): this.
> (display-narinfo-data): Update accordingly.
> (download-nar): Add prefer-fast-decompression? as a keyword argument, remove
> code to set! it and return the cpu-usage recorded.
> (process-substitution, process-substitution/fallback): Accept and pass through
> prefer-fast-decompression? to download-nar.
> (guix-substitute): Move the prefer fast decompression switching logic here.
>
> Change-Id: I4e80b457b55bcda8c0ff4ee224dd94a55e1b24fb

[...]

> -(define %prefer-fast-decompression?
> -  ;; Whether to prefer fast decompression over good compression ratios.  This
> -  ;; serves in particular to choose between lzip (high compression ratio but
> -  ;; low decompression throughput) and zstd (lower compression ratio but high
> -  ;; decompression throughput).
> -  #f)
> +(define %default-prefer-fast-decompression? #f)

I would either remove this variable or add a comment describing it (we
should do that for all top-level variables).

> @@ -604,7 +585,9 @@ (define* (download-nar narinfo destination
>              (format status-port "hash-mismatch ~a ~a ~a~%"
>                      (hash-algorithm-name algorithm)
>                      (bytevector->nix-base32-string expected)
> -                    (bytevector->nix-base32-string actual)))))))
> +                    (bytevector->nix-base32-string actual))))
> +
> +      cpu-usage)))

[...]

> +               (let ((cpu-usage
> +                      (process-substitution reply-port store-path destination
> +                                            #:cache-urls (substitute-urls)
> +                                            #:acl (current-acl)
> +                                            #:deduplicate? deduplicate?
> +                                            #:print-build-trace?
> +                                            print-build-trace?
> +                                            #:prefer-fast-decompression?
> +                                            prefer-fast-decompression?)))
> +
> +                 ;; Create a hysteresis: depending on CPU usage, favor
> +                 ;; compression methods with faster decompression (like ztsd)
> +                 ;; or methods with better compression ratios (like lzip).
> +                 ;; This stems from the observation that substitution can be
> +                 ;; CPU-bound when high-speed networks are used:
> +                 ;; <https://lists.gnu.org/archive/html/guix-devel/2020-12/msg00177.html>.
> +                 ;; To simulate "slow" networking or changing conditions, run:
> +                 ;; sudo tc qdisc add dev eno1 root tbf rate 512kbit latency
> +                 ;; 50ms burst 1540 and then cancel with: sudo tc qdisc del
> +                 ;; dev eno1 root
> +                 (loop (cond
> +                        ;; Whether to prefer fast decompression over good
> +                        ;; compression ratios.  This serves in particular to
> +                        ;; choose between lzip (high compression ratio but low
> +                        ;; decompression throughput) and zstd (lower
> +                        ;; compression ratio but high decompression
> +                        ;; throughput).
> +                        ((> cpu-usage .8) #t)
> +                        ((< cpu-usage .2) #f)
> +                        (else prefer-fast-decompression?)))))))))

Instead of having ‘download-nar’ return its CPU usage, which is
surprising, maybe should wrap the ‘process-substitution’ call in
‘guix-substitute’ in ‘with-cpu-usage-monitoring’ and keep all the logic
in ‘guix-substitute’?

Ludo’.