GNU bug report logs - #77708
[PATCH] gexp: ‘with-parameters‘ is respected by caches.

Previous Next

Full log

View this message in rfc822 format

From: Ludovic Courtès <ludovic.courtes <at> inria.fr>
To: David Elsing <david.elsing <at> posteo.net>
Cc: dev <at> jpoiret.xyz, zimon.toutoune <at> gmail.com, othacehe <at> gnu.org, ludo <at> gnu.org, me <at> tobias.gr, 77708 <at> debbugs.gnu.org, guix <at> cbaines.net
Subject: [bug#77708] [PATCH] gexp: ‘with-parameters‘ is respected by caches.
Date: Mon, 02 Jun 2025 10:45:06 +0200

Hi David,

Apologies for the delay.

David Elsing <david.elsing <at> posteo.net> 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’.

Previous Next

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