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

[Ludovic Courtès]

Hi!

Some initial comments…

Maxime Devos <maximedevos@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’.