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

[Christopher Baines]

Ludovic Courtès <ludo@gnu.org> writes:

> Christopher Baines <mail@cbaines.net> 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).

I've added a comment now, and I'll sent an updated patch.

>> @@ -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’?

Yeah, that makes sense. I'll send an updated patch shortly.

signature.asc
Description: PGP signature