Hi!

I took the liberty to reopen this patch because there were good ideas
IMO.  I’m sorry if my many questions and lack of responsiveness came out
as a suggestion that this approach wasn’t good.

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

>> +;; repeated 'stat' calls.  Allow computing the hash of the file in advance,
>> +;; to avoid having to send the file to the daemon when it is already interned
>> +;; in the store.
>>  (define-record-type <local-file>
>> -  (%%local-file file absolute name recursive? select?)
>> +  (%%local-file file absolute name sha256 recursive? select?)
>>    local-file?
>>    (file       local-file-file)                    ;string
>>    (absolute   %local-file-absolute-file-name)     ;promise string
>>    (name       local-file-name)                    ;string
>> +  (sha256     local-file-sha256)                  ;sha256 bytevector | #f
>
> Could we store the result of ‘fixed-output-path’ rather than the SHA256,
> while we’re at it?

I tried that with the patch below, roughly taking the same approach as
your patch series, but somewhat simplified, mostly so I could
experiment.

I changed just a few files to use the new ‘local-patches’ instead of
‘search-patches’ (I thought it might make sense to introduce a new
macro, to make it clear that ‘%package-module-path’ is not used at all).

The end result is that it works as intended :-), but it’s actually a
tiny bit slower: on a cache hit, we do 2 RPCs (add-temp-root +
valid-path?) instead of 1 (add-to-store).  The extra round-trip is more
expensive than the I/O we’re saving, at least on my laptop (with SSD; it
might be different with slower disk I/O and/or when talking to a remote
daemon, as on clusters.)

Now, this could be addressed by adding an ‘add-temp-root-if-valid’ RPC,
which would do both in one.

We can estimate the performance of that strategy by commenting out the
‘add-temp-root*’ call (thus getting a single RPC) in
‘local-file-compiler’: this time it’s slightly faster, but we’re in the
1% range on the wall-clock time of ‘guix build pigx -d --no-grafts’:

--8<---------------cut here---------------start------------->8---
$ time GUIX_DISABLE_LOCAL_FILE_CACHE=t ./pre-inst-env guix build pigx -d --no-grafts
/gnu/store/dqaknknlsw8a97xwjrhhd1g4jg71jqg7-pigx-0.0.3.drv

real	0m3.488s
user	0m3.718s
sys	0m0.132s
$ time GUIX_DISABLE_LOCAL_FILE_CACHE=t ./pre-inst-env guix build pigx -d --no-grafts
/gnu/store/dqaknknlsw8a97xwjrhhd1g4jg71jqg7-pigx-0.0.3.drv

real	0m3.501s
user	0m3.722s
sys	0m0.138s
$ time ./pre-inst-env guix build pigx -d --no-grafts
/gnu/store/dqaknknlsw8a97xwjrhhd1g4jg71jqg7-pigx-0.0.3.drv

real	0m3.437s
user	0m3.622s
sys	0m0.174s
$ time ./pre-inst-env guix build pigx -d --no-grafts
/gnu/store/dqaknknlsw8a97xwjrhhd1g4jg71jqg7-pigx-0.0.3.drv

real	0m3.492s
user	0m3.708s
sys	0m0.151s
--8<---------------cut here---------------end--------------->8---

Perhaps the gains would be a bit higher if we change all the package
files to use ‘local-patches’, but we probably can’t expect a lot more
anyway since that process is CPU-bound.

So I don’t know.  It feels like a worthy optimization, and one that’s
manageable from a maintenance viewpoint, but it buys us very little.

Thoughts?

Looking at the big picture, what I’d like to have is a package
derivation cache designed in such a way that “guix install foo” wouldn’t
even need to load any package module on a cache hit.  That’d make a
noticeable difference performance-wise, that’s another level of
complexity…  (I have a rough design in mind that we could discuss.)

Ludo’.