GNU bug report logs - #50384
[PATCH] Optimise search-patch (reducing I/O)

Previous Next

Full log

View this message in rfc822 format

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 50384 <at> debbugs.gnu.org
Subject: [bug#50384] [PATCH] Optimise search-patch (reducing I/O)
Date: Sat, 04 Sep 2021 23:47:39 +0200

Hi!

Some initial comments…

Maxime Devos <maximedevos <at> telenet.be> skribis:

> +++ b/guix/gexp.scm
> @@ -531,13 +531,37 @@ appears."
>  (define-gexp-compiler (local-file-compiler (file <local-file>) system target)
>    ;; "Compile" FILE by adding it to the store.
>    (match file
> -    (($ <local-file> file (= force absolute) name sha256 recursive? select?)
> -     ;; Canonicalize FILE so that if it's a symlink, it is resolved.  Failing
> -     ;; to do that, when RECURSIVE? is #t, we could end up creating a dangling
> -     ;; symlink in the store, and when RECURSIVE? is #f 'add-to-store' would
> -     ;; just throw an error, both of which are inconvenient.
> -     (interned-file absolute name
> -                    #:recursive? recursive? #:select? select?))))
> +    ;; Delay computing the absolute file name until 'intern', as this
> +    ;; might be a relatively expensive computation (e.g. if search-patch
> +    ;; is used), especially on a spinning disk.
> +    (($ <local-file> file absolute-promise name sha256 recursive? select?)
> +     (let ()
> +       (define (intern)
> +         ;; Canonicalize FILE so that if it's a symlink, it is resolved.
> +         ;; Failing to do that, when RECURSIVE? is #t, we could end up creating
> +         ;; a dangling symlink in the store, and when RECURSIVE? is #f
> +         ;; 'add-to-store' would just throw an error, both of which are
> +         ;; inconvenient.
> +         (interned-file (force absolute-promise) name
> +                        #:recursive? recursive? #:select? select?))
> +       (if sha256
> +           (let ((path (fixed-output-path name sha256 #:recursive? recursive?)))
> +             ;; If the hash is known in advance and the store already has the
> +             ;; item, there is no need to intern the file.
> +             (if (file-exists? path)
> +                 (mbegin %store-monad
> +                   ;; Tell the GC that PATH will be used, such that it won't
> +                   ;; be deleted.
> +                   ((store-lift add-temp-root) path)
> +                   ;; The GC could have deleted the item before add-temp-root
> +                   ;; completed, so check again if PATH exists.
> +                   (if (file-exists? path)
> +                       (return path)
> +                       ;; If it has been removed, fall-back interning.
> +                       (intern)))
> +                 ;; If PATH does not yet exist, fall back to interning.
> +                 (intern)))
> +           (intern))))))

‘file-exists?’ won’t work when talking to a remote store (e.g.,
GUIX_DAEMON_SOCKET=ssh://…).

‘add-temp-root’ doesn’t throw if the given store item does not exist.
So it could be written like this:

  (if sha256
      (mbegin %store-monad
        (add-temp-root* item)
        (if (valid-path?* item)
            (return item)
            (intern)))
      (intern))

But then, we’d add one RPC for every ‘add-to-store’ RPC corresponding to
a patch (you can set “GUIX_PROFILING=rpc” to see the numbers), which is
not great.

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.