GNU bug report logs -
#77708
[PATCH] gexp: ‘with-parameters‘ is respected by caches.
Previous Next
Full log
Message #8 received at 77708 <at> debbugs.gnu.org (full text, mbox):
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’.
This bug report was last modified 3 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.