Subject: Re: [bug#77708] [PATCH] gexp: ‘with-parameters‘ is respected by caches.

From: Ludovic Courtès <[email-masked]>
Date: Mon, 02 Jun 2025 10:45:06 +0200

Hi David,

Apologies for the delay.

David Elsing <[email-masked]> writes:

> As noted by Ludo' [1], several objects dependent on packages
> (such as derivations or grafts) are cached by the package and do not
> take parameters (apart from %current-system, %current-target-system and
> %graft?) into account. To fix that, my idea was to introduce an
> additional parameter `%parameterized-counter', which uniquely identifies
> a set of parameters and values in the <parameterized> object and which
> is used as additional key by the caches.
>
> To prevent a collision, the parameters and values are stored in a hash table,
> which keeps them alive forever. Would it be preferable to use something like a
> cryptographic hash instead?
>
> For `cache!' in (guix packages), I used
> `(,(scm->pointer package) . ,(%parameterized-counter)) as key together with
> hash-set! and hash-ref instead of hashq-set! and hashq-ref. Is that OK?
>
> [1] https://issues.guix.gnu.org/75879

Overall I have two main concerns: the added complexity and the run-time
overhead.  I would be tempted to just not pretend supporting
<parameterized> for arbitrary parameters than to pay this price.

WDYT?

> @@ -302,7 +305,7 @@ (define* (lower-object obj
>                                   (not (derivation? lowered)))
>                              (loop lowered)
>                              (return lowered)))
> -                      obj
> +                      obj (%parameterized-counter)
>                        system target graft?)))))))

[...]

> +;; Counter which uniquely identifies specific parameters and values used for
> +;; <parameterized>.
> +(define %parameterized-counter
> +  (make-parameter #f))

The “counter” really determines the dynamic scope of parameters in fact,
right?  Should it be renamed to “parameter scope” or similar?

> +(define %parameterized-counter-next-value 0)
> +
> +(define %parameterized-counters (make-hash-table))

This is very much like the regular dynamic environment (info "(guile)
Fluids and Dynamic States").

> +;; Add %parameterized-counter to PARAMETERS and its value,
> +;; which depends on PARAMETERS and VALUES, to PARAMETER-VALUES.
> +(define (add-parameterized-counter parameters parameter-values)
> +  (let* ((key `(,parameters . ,parameter-values))
> +         (counter
> +          (match (hash-ref %parameterized-counters key)

Note that this is quite expensive: hashing traverses part of KEY, and
then all of it must be traversed for comparison; in practice there would
probably be few parameters though.

> @@ -1689,13 +1690,12 @@ (define (cache! cache package system thunk)
>  SYSTEM."
>    ;; FIXME: This memoization should be associated with the open store, because
>    ;; otherwise it breaks when switching to a different store.
> -  (let ((result (thunk)))
> -    ;; Use `hashq-set!' instead of `hash-set!' because `hash' returns the
> -    ;; same value for all structs (as of Guile 2.0.6), and because pointer
> -    ;; equality is sufficient in practice.
> -    (hashq-set! cache package
> -                `((,system . ,result)
> -                  ,@(or (hashq-ref cache package) '())))
> +  (let ((result (thunk))
> +        (key `(,(scm->pointer package) . ,(%parameterized-counter))))
> +    (hash-set! cache key
> +               `((,system . ,result)
> +                 ,@(or (hash-ref cache key)
> +                       '())))

The ‘scm->pointer’ trick prevents the expensive <package> structure
traversal, but I’m unsure about the performance of switching to ‘equal?’
instead of ‘eq?’.

Does the ‘scm->pointer’ trick work with weak-key hash tables like
‘%bag-cache’?  I think it does because ‘scm->pointer’ records the link
between the pointer object and the package object in a weak hash table.
But it’s still quite bumpy.

> @@ -2068,7 +2068,7 @@ (define* (package->derivation package
>                                                #:system system
>                                                #:guile guile)))))
>                   (return drv)))
> -           package system #f graft?))
> +           package (%parameterized-counter) system #f graft?))

I wonder about the performance impact of the extra lookup key.

Thanks for working on this!

Ludo’.