[bug#50960,10/10] shell: Maintain a profile cache.

With this change, running "guix shell" (no arguments) is equivalent to:

  guix environment -r ~/.cache/guix/profiles/some-root -l guix.scm

This is the cache miss.  On cache hit, it's equivalent to:

  guix environment -p ~/.cache/guix/profiles/some-root

... which can run in 0.1s.

* guix/scripts/shell.scm (auto-detect-manifest): Looked for a cached GC
root to the profile and use it.
(%profile-cache-directory): New variable.
(profile-cache-key, profile-cached-gc-root): New procedures.
(guix-shell)[cache-entries, entry-expiration]: New procedures.
Add call to 'maybe-remove-expired-cache-entries'.
---
 guix/scripts/shell.scm | 90 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 84 insertions(+), 6 deletions(-)

Ludovic Courtès schreef op za 02-10-2021 om 12:22 [+0200]:
> With this change, running "guix shell" (no arguments) is equivalent to:
> 
>   guix environment -r ~/.cache/guix/profiles/some-root -l guix.scm
> 
> This is the cache miss.  On cache hit, it's equivalent to:
> 
>   guix environment -p ~/.cache/guix/profiles/some-root
> 

What if guix.scm is something like

;; Load custom package definitions
(include "a-package.scm")
(include "b-package.scm")
(include "c-package.scm")
(list a-package b-package c-package bash ...)

and a-package.scm, b-package.scm or c-package.scm is modified?
Then the cached profile should be rebuild, no?

I use something like that for my operating system definition
(though with -L and use-modules).  It would be nice if this worked
for "guix shell" as well.

So I think the cache should also check if these dependencies have been modified.
To keep track of the dependencies, something like the ‘compile-all,compile:
Keep track of dependencies of compiled modules.’ patch from
<https://issues.guix.gnu.org/50384> could be used, though the 'include', 'load',
'include-from-path' and maybe 'use-modules' (if something like "guix shell -Lextra-modules-directory ..."
is done) macros would need to be replaced.

Greetings,
Maxime.

Ludovic Courtès schreef op za 02-10-2021 om 12:22 [+0200]:
> +(define (profile-cache-key file)
> +  "Return the cache key for the profile corresponding to FILE, a 'guix.scm' or
> +'manifest.scm' file, or #f if we lack channel information."
> +  (match (current-channels)
> +    (() #f)
> +    (((= channel-commit commits) ...)
> +     (let ((stat (stat file)))
> +       (bytevector->base32-string
> +        (sha256 (string->utf8
> +                 (string-append (string-join commits) ":"
> +                                (basename file) ":"
> +                                (number->string (stat:dev stat)) ":"
> +                                (number->string (stat:ino stat))))))))))

Why only use the 'basename' of a file name instead of the full name?
(Consider the case where a user has multiple "guix.scm" or "manifest.scm".)
This turns out to be unproblematic, because stat:dev and stat:ino is included
as well, though including (a part of) the file name is superfluous because
stat:dev and stat:ino are included.

Could you document the rationale for including the file name, and why only
the basename is included instead of the full file name?

Greetings,
Maxime.

Maxime Devos <maximedevos@telenet.be> skribis:

> Ludovic Courtès schreef op za 02-10-2021 om 12:22 [+0200]:
>> With this change, running "guix shell" (no arguments) is equivalent to:
>> 
>>   guix environment -r ~/.cache/guix/profiles/some-root -l guix.scm
>> 
>> This is the cache miss.  On cache hit, it's equivalent to:
>> 
>>   guix environment -p ~/.cache/guix/profiles/some-root
>> 
>
> What if guix.scm is something like
>
> ;; Load custom package definitions
> (include "a-package.scm")
> (include "b-package.scm")
> (include "c-package.scm")
> (list a-package b-package c-package bash ...)
>
> and a-package.scm, b-package.scm or c-package.scm is modified?
> Then the cached profile should be rebuild, no?

It should, but it won’t; that’s a weakness of this mtime-based caching
strategy.

I’m not sure how to address it.  We’d need a cache key that’s more
precise than inode/mtime, yet cheap to compute.

Perhaps we need to live with that limitation, document it, and provide a
flag to force a rebuild?

> So I think the cache should also check if these dependencies have been modified.
> To keep track of the dependencies, something like the ‘compile-all,compile:
> Keep track of dependencies of compiled modules.’ patch from
> <https://issues.guix.gnu.org/50384> could be used, though the 'include', 'load',
> 'include-from-path' and maybe 'use-modules' (if something like "guix shell -Lextra-modules-directory ..."
> is done) macros would need to be replaced.

Problem is that any attempt to keep track of dependencies is always an
approximation because macros can do anything—I can have my own macro
that reads files at expansion time.  So I’m inclined to not even try.

Thanks,
Ludo’.

Maxime Devos <maximedevos@telenet.be> skribis:

> Ludovic Courtès schreef op za 02-10-2021 om 12:22 [+0200]:
>> +(define (profile-cache-key file)
>> +  "Return the cache key for the profile corresponding to FILE, a 'guix.scm' or
>> +'manifest.scm' file, or #f if we lack channel information."
>> +  (match (current-channels)
>> +    (() #f)
>> +    (((= channel-commit commits) ...)
>> +     (let ((stat (stat file)))
>> +       (bytevector->base32-string
>> +        (sha256 (string->utf8
>> +                 (string-append (string-join commits) ":"
>> +                                (basename file) ":"
>> +                                (number->string (stat:dev stat)) ":"
>> +                                (number->string (stat:ino stat))))))))))
>
> Why only use the 'basename' of a file name instead of the full name?
> (Consider the case where a user has multiple "guix.scm" or "manifest.scm".)
> This turns out to be unproblematic, because stat:dev and stat:ino is included
> as well, though including (a part of) the file name is superfluous because
> stat:dev and stat:ino are included.
>
> Could you document the rationale for including the file name, and why only
> the basename is included instead of the full file name?

Actually it’s probably not useful to include the file (base)name.  I
think initially I thought about distinguishing between guix.scm and
manifest.scm, since they return different kinds of objects, but dev/ino
is probably enough.  WDYT?

Ludo’.

Ludovic Courtès schreef op za 02-10-2021 om 16:14 [+0200]:
> > Why only use the 'basename' of a file name instead of the full name?
> > (Consider the case where a user has multiple "guix.scm" or "manifest.scm".)
> > This turns out to be unproblematic, because stat:dev and stat:ino is included
> > as well, though including (a part of) the file name is superfluous because
> > stat:dev and stat:ino are included.
> > 
> > Could you document the rationale for including the file name, and why only
> > the basename is included instead of the full file name?
> 
> Actually it’s probably not useful to include the file (base)name.  I
> think initially I thought about distinguishing between guix.scm and
> manifest.scm, since they return different kinds of objects, but dev/ino
> is probably enough.  WDYT?

Looking at https://lwn.net/Articles/866582/, it appears when BTRFS and NFS are
combined, it is possible for two dev/ino pairs to be the same for different files.
It appears to be considered a bug but non-trivial to fix.  Thus, dev/ino apparently
is not always sufficient, so it may be reasonable to include the file name.

I would include the full file name, because the basename is often rather generic
("guix.scm", "manifest.scm"), though this depends on the habits of the user.

Greetings,
Maxime.

Ludovic Courtès schreef op za 02-10-2021 om 16:12 [+0200]:
[reordered]
> > So I think the cache should also check if these dependencies have been modified.
> > To keep track of the dependencies, something like the ‘compile-all,compile:
> > Keep track of dependencies of compiled modules.’ patch from
> > <https://issues.guix.gnu.org/50384> could be used, though the 'include', 'load',
> > 'include-from-path' and maybe 'use-modules' (if something like "guix shell -Lextra-modules-directory ..."
> > is done) macros would need to be replaced.
> 
> Problem is that any attempt to keep track of dependencies is always an
> approximation because macros can do anything—I can have my own macro
> that reads files at expansion time.  So I’m inclined to not even try.

I expect most people use 'include', 'load', 'include-from-path' or 'load-from-path'
instead of writing their own macro reading files at expansion time, and if they do
write their own macro, I expect it would be implemented in terms of the former anyway,
so I expect replacing these macros and 'use-modules' to be sufficient, especially
if the flag proposed below is available as ‘escape hatch’ for when the dependency
tracking is insufficient.

This is not merely an expansion-time problem, files loaded at 'load' or 'eval' time
are important as well.  E.g., consider the case where the manifest is something like

  (list
    (package
      (inherit stuff)
      (source (local-file "stuff" #:recursive? #t)))
    other-packages ...)

then all files inside 'stuff' are important as well.  I don't see how dependencies
could be tracked here without an excessive amount of 'stat' calls, maybe guix should
ignore these dependencies (possibly with a warning, and a reference to the manual
documenting which dependencies are tracked and which are not?). 

Ludovic Courtès schreef op za 02-10-2021 om 16:12 [+0200]:
> I’m not sure how to address it.  We’d need a cache key that’s more
> precise than inode/mtime, yet cheap to compute.

The mtime of the file and the mtime of every dependency (ignoring modules from
Guile, Guix and channels to reduce the number of calls to 'stat'). 

> Perhaps we need to live with that limitation, document it, and provide a
> flag to force a rebuild?

A documented flag to always consider the cache stale seems good, though I think
at least the dependencies made with the common macros and procedures 'include',
'load', 'include-from-path', 'load-from-path', 'use-modules' and non-recursive
'local-file' could be tracked, though this could be left as a TODO for later
I suppose.

Greetings,
Maxime.

Maxime Devos <maximedevos@telenet.be> skribis:

> Ludovic Courtès schreef op za 02-10-2021 om 16:14 [+0200]:
>> > Why only use the 'basename' of a file name instead of the full name?
>> > (Consider the case where a user has multiple "guix.scm" or "manifest.scm".)
>> > This turns out to be unproblematic, because stat:dev and stat:ino is included
>> > as well, though including (a part of) the file name is superfluous because
>> > stat:dev and stat:ino are included.
>> > 
>> > Could you document the rationale for including the file name, and why only
>> > the basename is included instead of the full file name?
>> 
>> Actually it’s probably not useful to include the file (base)name.  I
>> think initially I thought about distinguishing between guix.scm and
>> manifest.scm, since they return different kinds of objects, but dev/ino
>> is probably enough.  WDYT?
>
> Looking at https://lwn.net/Articles/866582/, it appears when BTRFS and NFS are
> combined, it is possible for two dev/ino pairs to be the same for different files.
> It appears to be considered a bug but non-trivial to fix.  Thus, dev/ino apparently
> is not always sufficient, so it may be reasonable to include the file name.
>
> I would include the full file name, because the basename is often rather generic
> ("guix.scm", "manifest.scm"), though this depends on the habits of the user.

OK, that makes sense to me, let’s do that.

Ludo’.

Maxime Devos <maximedevos@telenet.be> skribis:

> Ludovic Courtès schreef op za 02-10-2021 om 16:12 [+0200]:
> [reordered]
>> > So I think the cache should also check if these dependencies have been modified.
>> > To keep track of the dependencies, something like the ‘compile-all,compile:
>> > Keep track of dependencies of compiled modules.’ patch from
>> > <https://issues.guix.gnu.org/50384> could be used, though the 'include', 'load',
>> > 'include-from-path' and maybe 'use-modules' (if something like "guix shell -Lextra-modules-directory ..."
>> > is done) macros would need to be replaced.
>> 
>> Problem is that any attempt to keep track of dependencies is always an
>> approximation because macros can do anything—I can have my own macro
>> that reads files at expansion time.  So I’m inclined to not even try.
>
> I expect most people use 'include', 'load', 'include-from-path' or 'load-from-path'
> instead of writing their own macro reading files at expansion time, and if they do
> write their own macro, I expect it would be implemented in terms of the former anyway,
> so I expect replacing these macros and 'use-modules' to be sufficient, especially
> if the flag proposed below is available as ‘escape hatch’ for when the dependency
> tracking is insufficient.
>
> This is not merely an expansion-time problem, files loaded at 'load' or 'eval' time
> are important as well.  E.g., consider the case where the manifest is something like
>
>   (list
>     (package
>       (inherit stuff)
>       (source (local-file "stuff" #:recursive? #t)))
>     other-packages ...)

In a ‘guix.scm’ file, ‘source’ doesn’t matter.  It does matter in a
manifest, but that sounds a bit less common.

> then all files inside 'stuff' are important as well.  I don't see how dependencies
> could be tracked here without an excessive amount of 'stat' calls, maybe guix should
> ignore these dependencies (possibly with a warning, and a reference to the manual
> documenting which dependencies are tracked and which are not?). 

[...]

> A documented flag to always consider the cache stale seems good, though I think
> at least the dependencies made with the common macros and procedures 'include',
> 'load', 'include-from-path', 'load-from-path', 'use-modules' and non-recursive
> 'local-file' could be tracked, though this could be left as a TODO for later
> I suppose.

Tracking those uses reliably is impossible: there could be same-named
bindings that do other things, there could be custom macros, there could
be “dynamic arguments” (whose value is not known statically), etc.  You
have to expand + evaluate the code to get better results, and even then,
there might be different paths in the code so you can’t be sure you got
it right.

We could get an approximation for common uses by recognizing special
forms as you suggest.  But it’s just that, an approximation.

In such situations, I err on the side of not even trying.  The added
complexity for a flaky result doesn’t pay off to me.  I prefer to be
upfront, document limitations, and let users handle them as they see
fit.

WDYT?

A similar problem occurs with system provenance tracking, which saves
‘configuration.scm’ but leaves it up to the user to preserve additional
files if needed (info "(guix) Service Reference").

Thanks,
Ludo’.

Hi,

On Mon, 4 Oct 2021 at 10:52, Ludovic Courtès <ludo@gnu.org> wrote:

> In such situations, I err on the side of not even trying.  The added
> complexity for a flaky result doesn’t pay off to me.  I prefer to be
> upfront, document limitations, and let users handle them as they see
> fit.

I agree.  Just a flag for forcing seems be worth here.  For this
dependencies use-case or simply for debugging.

Cheers,
simon

Ludovic Courtès schreef op ma 04-10-2021 om 10:19 [+0200]:
> > A documented flag to always consider the cache stale seems good, though I think
> > at least the dependencies made with the common macros and procedures 'include',
> > 'load', 'include-from-path', 'load-from-path', 'use-modules' and non-recursive
> > 'local-file' could be tracked, though this could be left as a TODO for later
> > I suppose.
> 
> Tracking those uses reliably is impossible: there could be same-named
> bindings that do other things, there could be custom macros, there could
> be “dynamic arguments” (whose value is not known statically), etc.  You
> have to expand + evaluate the code to get better results, and even then,
> there might be different paths in the code so you can’t be sure you got
> it right.

I think there's a miscommunication here.  From what I'm reading, what you have
in mind is that, to determine the dependency information, "guix shell" would
open "guix.scm", read it with the procedure 'read' and sniff for 'load',
'include-from-path', 'load-from-path', 'use-modules' and 'local-file' form
-- something like 'module-file-dependencies' and 'extract-dependencies', but
more general.

However, my idea is to replace these macros, such that, when "guix shell"
loads "guix.scm" or "manifest.scm", these macros inform "guix shell"
that "guix.scm" or "manifest.scm" depend on certain files referred to
by 'load', 'include-from-path', etc. forms, using a mechanism like the
'notice-dependency' defined in <https://issues.guix.gnu.org/50384>.

Then, when "guix shell" puts the resulting profile in the cache,
it includes the generated list of files.  And when "guix shell" finds
an entry in the cache, it will check if the files in this list (and guix.scm
or manifest.scm of course) have been modified.  If some are (or the forcing
flag is set), guix.scm needs to be loaded and a new profile needs to be
generated.  If they are all unchanged, guix.scm will _not_ be read: the cached
profile can be reused.

It's not 100% reliable (e.g. the list of packages in the manifest could
depend on the phase of the moon if (current-time) is used) (is that what
you mean by ‘different paths’ and ‘dynamic arguments’?), but it should
cover the vast majority of cases.

I don't know a non-artifical situation where ‘custom macros’ are a problem
-- do you know an example in the wild where this dependency tracking scheme
would fail to work?

> We could get an approximation for common uses by recognizing special
> forms as you suggest.  But it’s just that, an approximation.

It's an approximation, sure, but it seems to be a quite accurate approximation
to me.  And it's not really recognising special forms that I'm suggesting,
but rather modifying the macros behind these forms to inform "guix shell"
of what the dependencies are.

> In such situations, I err on the side of not even trying.  The added
> complexity for a flaky result doesn’t pay off to me.  I prefer to be
> upfront, document limitations, and let users handle them as they see
> fit.

About complexity: there's some extra code, sure, but it doesn't seem complex
to me.  To track dependencies in <https://issues.guix.gnu.org/50384>,
I only needed to add 'notice-dependency' and some other code to (guix build compile),
and some code to build-aux/compile-all.scm to save/load the dependency information
to/from the builddir and to also check the mtime of dependencies.

Does it still seem flaky to you, after my explanation on how the dependency
information would be determined?

Being upfront, documenting limitations, and having a ‘force rebuild flag’
(is that what you mean by ‘letting users handle them as they see fit’?)
(possibly also documenting 'notice-dependency') is not incompatible with
the automatic dependency tracking.

More abstractly, this seems more like a ‘Perfect is the enemy of the good’ situation.
While I would prefer 'perfect' above 'good', and the automatic dependency tracking
certainly isn't 'perfect', it does seem 'good' to me, and perfection appears to
be impossible and there's the ‘force rebuild flag’ as an escape hatch, so I believe
'good-but-not-perfect' to be acceptable here, as long as it is documented in the manual
that there are some limitation on what dependencies are automatically tracked.

Greetings,
Maxime.

Hi Maxime,

Maxime Devos <maximedevos@telenet.be> skribis:

> Ludovic Courtès schreef op ma 04-10-2021 om 10:19 [+0200]:
>> > A documented flag to always consider the cache stale seems good, though I think
>> > at least the dependencies made with the common macros and procedures 'include',
>> > 'load', 'include-from-path', 'load-from-path', 'use-modules' and non-recursive
>> > 'local-file' could be tracked, though this could be left as a TODO for later
>> > I suppose.
>> 
>> Tracking those uses reliably is impossible: there could be same-named
>> bindings that do other things, there could be custom macros, there could
>> be “dynamic arguments” (whose value is not known statically), etc.  You
>> have to expand + evaluate the code to get better results, and even then,
>> there might be different paths in the code so you can’t be sure you got
>> it right.
>
> I think there's a miscommunication here.  From what I'm reading, what you have
> in mind is that, to determine the dependency information, "guix shell" would
> open "guix.scm", read it with the procedure 'read' and sniff for 'load',
> 'include-from-path', 'load-from-path', 'use-modules' and 'local-file' form
> -- something like 'module-file-dependencies' and 'extract-dependencies', but
> more general.

Yes, that’s roughly what I thought you had in mind, sorry!

> However, my idea is to replace these macros, such that, when "guix shell"
> loads "guix.scm" or "manifest.scm", these macros inform "guix shell"
> that "guix.scm" or "manifest.scm" depend on certain files referred to
> by 'load', 'include-from-path', etc. forms, using a mechanism like the
> 'notice-dependency' defined in <https://issues.guix.gnu.org/50384>.
>
> Then, when "guix shell" puts the resulting profile in the cache,
> it includes the generated list of files.  And when "guix shell" finds
> an entry in the cache, it will check if the files in this list (and guix.scm
> or manifest.scm of course) have been modified.  If some are (or the forcing
> flag is set), guix.scm needs to be loaded and a new profile needs to be
> generated.  If they are all unchanged, guix.scm will _not_ be read: the cached
> profile can be reused.
>
> It's not 100% reliable (e.g. the list of packages in the manifest could
> depend on the phase of the moon if (current-time) is used) (is that what
> you mean by ‘different paths’ and ‘dynamic arguments’?), but it should
> cover the vast majority of cases.
>
> I don't know a non-artifical situation where ‘custom macros’ are a problem
> -- do you know an example in the wild where this dependency tracking scheme
> would fail to work?

I think we have to look at the cost and at the benefits.  The cost:
either we spread ‘notice-dependency’ calls all over the place, or we
interpose on ‘open-file’ altogether, assuming that even works.  The
benefit: we’d have an 80% solution.  There would still be edge cases not
handled correctly; a realistic example is a ‘guix.scm’ whose execution
differs based on the presence of a file or environment variable.  People
would have to know about this pitfall and pass the right flag.

[...]

>> In such situations, I err on the side of not even trying.  The added
>> complexity for a flaky result doesn’t pay off to me.  I prefer to be
>> upfront, document limitations, and let users handle them as they see
>> fit.
>
> About complexity: there's some extra code, sure, but it doesn't seem complex
> to me.  To track dependencies in <https://issues.guix.gnu.org/50384>,
> I only needed to add 'notice-dependency' and some other code to (guix build compile),
> and some code to build-aux/compile-all.scm to save/load the dependency information
> to/from the builddir and to also check the mtime of dependencies.
>
> Does it still seem flaky to you, after my explanation on how the dependency
> information would be determined?
>
> Being upfront, documenting limitations, and having a ‘force rebuild flag’
> (is that what you mean by ‘letting users handle them as they see fit’?)
> (possibly also documenting 'notice-dependency') is not incompatible with
> the automatic dependency tracking.
>
> More abstractly, this seems more like a ‘Perfect is the enemy of the good’ situation.
> While I would prefer 'perfect' above 'good', and the automatic dependency tracking
> certainly isn't 'perfect', it does seem 'good' to me, and perfection appears to
> be impossible and there's the ‘force rebuild flag’ as an escape hatch, so I believe
> 'good-but-not-perfect' to be acceptable here, as long as it is documented in the manual
> that there are some limitation on what dependencies are automatically tracked.

Yeah, I can relate to that sentiment, even if I’m all too often on the
“perfectionist” side of things.  ;-)

I guess I could live with an 80% dependency tracking solution.  However,
my criteria for it would be that (1) it should be orthogonal (e.g., not
requiring explicit calls to ‘notice-dependency’ in many places), and (2)
it should be self-contained and reasonably small.  Perhaps having
‘load*’ or similar instrument ‘open-file’ or similar could help achieve
that?

I would rather not block ‘guix shell’ on that though.

WDYT?

Thanks for taking the time to clarify what you had in mind!

Ludo’.