GNU bug report logs -
#77708
[PATCH] gexp: ‘with-parameters‘ is respected by caches.
Previous Next
Reported by: David Elsing <david.elsing <at> posteo.net>
Date: Thu, 10 Apr 2025 14:50:02 UTC
Severity: normal
Tags: patch
Done: David Elsing <david.elsing <at> posteo.net>
Bug is archived. No further changes may be made.
Full log
Message #11 received at 77708 <at> debbugs.gnu.org (full text, mbox):
Hi Ludo',
Ludovic Courtès <ludovic.courtes <at> inria.fr> writes:
> Apologies for the delay.
No problem, thanks for looking at the patch!
> 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?
I think parameterized packages would be very useful. Something like the
previous approach [1] would of course be nicer, but I didn't find any
information why it was not continued. From what I can tell,
`with-parameters' is now working except for the caches, so in my opinion
it makes sense to fix that.
> The “counter” really determines the dynamic scope of parameters in fact,
> right? Should it be renamed to “parameter scope” or similar?
I only defined it to avoid frequently recalculating the hash of the
dynamic parameters, so the caches only need to include the single
integer in the key.
>> +(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").
Yes, this just assigns a unique integer to each unique set of
parameters.
>> +;; 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.
Do you think using a cryptographic hash (to avoid collisions) would be
better? I think we need to look at the values of the parameters and not
just at object identity in case an object was modified or for objects
like strings, right?
> The ‘scm->pointer’ trick prevents the expensive <package> structure
> traversal, but I’m unsure about the performance of switching to ‘equal?’
> instead of ‘eq?’.
> I wonder about the performance impact of the extra lookup key.
I did some benchmarking with 10 runs of 'guix build --no-grafts libreoffice -d':
- Previously:
3.684 s ± 0.019 s
- Using `scm->pointer' and `hash-set' and `hash-ref' in `cache!':
3.712 s ± 0.027 s
- Using an additional key in `package->derivation':
3.697 s ± 0.028 s
- With the full path applied:
3.726 s ± 0.032 s
So there indeed seems to be a perfomance impact of the order of ~1% in
this case.
> 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.
Oh that's a good point, is chaining weak hash tables a problem? I
couldn't think of another way to add the integer describing the
parameters to the key though.
Also problematic is the `%parameterized-counter' hash table, as it keeps
the keys (i.e. all parameters passed to `with-parameters') alive
forever, so I think it should also be a weak hash table, right? If the
parameters are dropped and later constructed again, the derivation would
just be recalculated, like for `%bag-cache'.
Cheers,
David
[1] https://guix.gnu.org/en/blog/2023/parameterized-packages-for-gnu-guix/
This bug report was last modified 7 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.