| Message ID | 874kj40wbp.fsf_-_@gnu.org |
|---|---|
| State | Accepted |
| Headers | show |
Ludovic Courtès <ludo@gnu.org> writes: > Ludovic Courtès <ludo@gnu.org> skribis: > >> There’s a catch here: OUTPUT should be taken into account. >> >> Also it’s better to use eq?-ness but… I realized >> ‘inferior-package-inputs’ & co. do not preserve eq?-ness. > > I think I went overboard here: given that <inferior-package> is a simple > flat record type, using ‘equal?’/‘hash-ref’ is reasonable and that way > we avoid the troubles of building an ID-to-package table. All in all > it’s slightly more efficient. This looks good to me. It is very similar to my first version (which I didn’t send to the list), which also built a key consisting of the arguments to inferior-package->manifest-entry — I wasn’t sure which of them was important so I used them all instead of just consing package and output. I also like the use of define-syntax-rule to make it all look neater.
Ricardo Wurmus <rekado@elephly.net> skribis: > Ludovic Courtès <ludo@gnu.org> writes: > >> Ludovic Courtès <ludo@gnu.org> skribis: >> >>> There’s a catch here: OUTPUT should be taken into account. >>> >>> Also it’s better to use eq?-ness but… I realized >>> ‘inferior-package-inputs’ & co. do not preserve eq?-ness. >> >> I think I went overboard here: given that <inferior-package> is a simple >> flat record type, using ‘equal?’/‘hash-ref’ is reasonable and that way >> we avoid the troubles of building an ID-to-package table. All in all >> it’s slightly more efficient. > > This looks good to me. > > It is very similar to my first version (which I didn’t send to the > list), which also built a key consisting of the arguments to > inferior-package->manifest-entry — I wasn’t sure which of them was > important so I used them all instead of just consing package and > output. > > I also like the use of define-syntax-rule to make it all look neater. I pushed it as 0f20b3fa2050ba6e442e340a204516b9375cd231. I wonder if the other patches improve the situation. If you run the same test case with: GUIX_PROFILING=memoization what hit rates does it show for these spots? Thanks, Ludo’.
Ludovic Courtès <ludo@gnu.org> writes: > I pushed it as 0f20b3fa2050ba6e442e340a204516b9375cd231. Thanks! > I wonder if the other patches improve the situation. If you run the > same test case with: > > GUIX_PROFILING=memoization > > what hit rates does it show for these spots? Memoization: 15 tables, 2 non-empty guix/inferior.scm:438:2: 403 entries, 403 lookups, 0% hits guix/inferior.scm:392:2: 403 entries, 403 lookups, 0% hits So, I guess we can drop those two patches.
Ricardo Wurmus <rekado@elephly.net> skribis: > Ludovic Courtès <ludo@gnu.org> writes: > >> I pushed it as 0f20b3fa2050ba6e442e340a204516b9375cd231. > > Thanks! > >> I wonder if the other patches improve the situation. If you run the >> same test case with: >> >> GUIX_PROFILING=memoization >> >> what hit rates does it show for these spots? > > Memoization: 15 tables, 2 non-empty > guix/inferior.scm:438:2: 403 entries, 403 lookups, 0% hits > guix/inferior.scm:392:2: 403 entries, 403 lookups, 0% hits > > So, I guess we can drop those two patches. Looks like it. :-) Closing! Thanks, Ludo’.
diff --git a/guix/inferior.scm b/guix/inferior.scm index 2fe91beaab..d813b3b918 100644 --- a/guix/inferior.scm +++ b/guix/inferior.scm @@ -642,29 +642,41 @@ failing when GUIX is too old and lacks the 'guix repl' command." (define* (inferior-package->manifest-entry package #:optional (output "out") - #:key (parent (delay #f)) - (properties '())) + #:key (properties '())) "Return a manifest entry for the OUTPUT of package PACKAGE." ;; For each dependency, keep a promise pointing to its "parent" entry. - (letrec* ((deps (map (match-lambda - ((label package) - (inferior-package->manifest-entry package - #:parent (delay entry))) - ((label package output) - (inferior-package->manifest-entry package output - #:parent (delay entry)))) - (inferior-package-propagated-inputs package))) - (entry (manifest-entry - (name (inferior-package-name package)) - (version (inferior-package-version package)) - (output output) - (item package) - (dependencies (delete-duplicates deps)) - (search-paths - (inferior-package-transitive-native-search-paths package)) - (parent parent) - (properties properties)))) - entry)) + (define cache + (make-hash-table)) + + (define-syntax-rule (memoized package output exp) + (let ((compute (lambda () exp)) + (key (cons package output))) + (or (hash-ref cache key) + (let ((result (compute))) + (hash-set! cache key result) + result)))) + + (let loop ((package package) + (output output) + (parent (delay #f))) + (memoized package output + (letrec* ((deps (map (match-lambda + ((label package) + (loop package "out" (delay entry))) + ((label package output) + (loop package output (delay entry)))) + (inferior-package-propagated-inputs package))) + (entry (manifest-entry + (name (inferior-package-name package)) + (version (inferior-package-version package)) + (output output) + (item package) + (dependencies (delete-duplicates deps)) + (search-paths + (inferior-package-transitive-native-search-paths package)) + (parent parent) + (properties properties)))) + entry)))) ;;; @@ -750,3 +762,7 @@ This is a convenience procedure that people may use in manifests passed to #:cache-directory cache-directory #:ttl ttl))) (open-inferior cached)) + +;;; Local Variables: +;;; eval: (put 'memoized 'scheme-indent-function 1) +;;; End: