diff mbox series

[bug#39258,v6,1/2] DRAFT packages: Add fields to packages cache.

Message ID 20210715073328.212123-2-zimon.toutoune@gmail.com
State New
Headers show
Series DRAFT "guix search" performances | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/issue success View issue

Commit Message

zimoun July 15, 2021, 7:33 a.m. UTC
* gnu/packages.scm (generate-package-cache)[expand-cache]: Add synopsis and
description.
(load-package-cache, find-packages-by-names, find-packages-locations): Adapt
accordingly.
(fold-available-packages): Add synopsis, description, module and symbol when
cache is authoritative.  Replace 'fold-packages' by
'fold-module-public-variables*' when cache is not authoritative.
---
 gnu/packages.scm | 52 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 36 insertions(+), 16 deletions(-)

Comments

Arun Isaac July 17, 2021, 8:31 a.m. UTC | #1
Hi Simon,

I understand that one of the things you are trying to do is to have a
common interface for the cache and no-cache cases. To achieve this, I
think fold-available-packages and fold-packages should have the same
function signature. They should both pass a <package> object to
PROC. Currently, fold-packages is passing a <package> object whereas
fold-available-packages is passing the fields of the <package> object as
individual parameters. If fold-packages and fold-available-packages have
the same function signature, then the changes in your [PATCH v6 2/2]
would be way simpler.

Also, why do we need two separate functions---fold-available-packages
and fold-packages? Can't fold-available-packages do everything
fold-packages can and thus totally replace it?

> * gnu/packages.scm (generate-package-cache)[expand-cache]: Add synopsis and
> description.
> (load-package-cache, find-packages-by-names, find-packages-locations): Adapt
> accordingly.

A couple of typos here:

find-packages-by-names -> find-packages-by-name
find-packages-locations -> find-package-locations

Regards,
Arun
Ludovic Courtès July 23, 2021, 3:30 p.m. UTC | #2
Hi Arun!

Arun Isaac <arunisaac@systemreboot.net> skribis:

> Also, why do we need two separate functions---fold-available-packages
> and fold-packages? Can't fold-available-packages do everything
> fold-packages can and thus totally replace it?

The initial goal was for ‘fold-available-packages’ to be lightweight.
Currently, it doesn’t allocate anything; instead, it passes info as
keyword parameters, which the callee is free to ignore.  That’s why
these two procedures have different signatures.

One benchmark is “guix package -A > /dev/null”.  This should take
ideally 0.5s at most because that’s what’s used by shell completion (the
first time); currently it takes 0.82s on my laptop, though.

Thanks,
Ludo’.
zimoun Aug. 17, 2021, 2:03 p.m. UTC | #3
Hi Arun and Ludo,

Thanks for the review.

On Sat, 17 Jul 2021 at 14:01, Arun Isaac <arunisaac@systemreboot.net> wrote:

> I understand that one of the things you are trying to do is to have a
> common interface for the cache and no-cache cases. To achieve this, I
> think fold-available-packages and fold-packages should have the same
> function signature. They should both pass a <package> object to
> PROC. Currently, fold-packages is passing a <package> object whereas
> fold-available-packages is passing the fields of the <package> object as
> individual parameters. If fold-packages and fold-available-packages have
> the same function signature, then the changes in your [PATCH v6 2/2]
> would be way simpler.

I agree.  Previously [1], I created ’fold-packages*’ which was a cached
’fold-packages’ and Ludo answered [2]:

        Did you see ‘fold-available-packages’?  It seems you could
        extend it instead of introducing ‘fold-packages*’, no?

therefore, it is somehow another attempt on the other side. :-)

1: <http://issues.guix.gnu.org/39258#91>
2: <http://issues.guix.gnu.org/39258#93>

> Also, why do we need two separate functions---fold-available-packages
> and fold-packages? Can't fold-available-packages do everything
> fold-packages can and thus totally replace it?

To be honest, I have been lazy because unifying
’fold-available-packages’ and ’fold-packages’ means to change the
signature and so a bit of refactoring.  And as Ludo explained,
’fold-available-packages’ has to be as light as possible because it is
used by Emacs-Guix and maybe Nyxt for completion. :-)

>> * gnu/packages.scm (generate-package-cache)[expand-cache]: Add synopsis and
>> description.
>> (load-package-cache, find-packages-by-names, find-packages-locations): Adapt
>> accordingly.
>
> A couple of typos here:
>
> find-packages-by-names -> find-packages-by-name
> find-packages-locations -> find-package-locations

Thanks for the spot.


On Fri, 23 Jul 2021 at 17:30, Ludovic Courtès <ludo@gnu.org> wrote:

> One benchmark is “guix package -A > /dev/null”.  This should take
> ideally 0.5s at most because that’s what’s used by shell completion (the
> first time); currently it takes 0.82s on my laptop, though.

On cold cache, on my laptop:

--8<---------------cut here---------------start------------->8---
$ time guix package -A > /dev/null

real	0m1.717s
user	0m2.526s
sys	0m0.083s
--8<---------------cut here---------------end--------------->8---

and on my (slow) desktop:

real	0m6.196s
user	0m2.008s
sys	0m0.093s


Warn cache:
        laptop          desktop
real	0m1.425s        0m1.217s
user	0m2.505s   	0m1.901s
sys	0m0.033s   	0m0.051s


Well, another story I guess. :-)


Cheers,
simon
diff mbox series

Patch

diff --git a/gnu/packages.scm b/gnu/packages.scm
index ccfc83dd11..34c6d73b86 100644
--- a/gnu/packages.scm
+++ b/gnu/packages.scm
@@ -4,6 +4,7 @@ 
 ;;; Copyright © 2014 Eric Bavier <bavier@member.fsf.org>
 ;;; Copyright © 2016, 2017 Alex Kost <alezost@gmail.com>
 ;;; Copyright © 2016 Mathieu Lirzin <mthl@gnu.org>
+;;; Copyright © 2021 Simon Tournier <zimon.toutoune@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -211,28 +212,45 @@  package module."
       (vhash-fold (lambda (name vector result)
                     (match vector
                       (#(name version module symbol outputs
+                              synopsis description
                               supported? deprecated?
                               file line column)
                        (proc name version result
                              #:outputs outputs
+                             #:synopsis synopsis
+                             #:description description
                              #:location (and file
                                              (location file line column))
+                             #:module module
+                             #:symbol symbol
                              #:supported? supported?
                              #:deprecated? deprecated?))))
                   init
                   cache)
-      (fold-packages (lambda (package result)
-                       (proc (package-name package)
-                             (package-version package)
-                             result
-                             #:outputs (package-outputs package)
-                             #:location (package-location package)
-                             #:supported?
-                             (->bool (supported-package? package))
-                             #:deprecated?
-                             (->bool
-                              (package-superseded package))))
-                     init)))
+      (fold-module-public-variables*
+       (lambda (module symbol variable result)
+         (let ((package (false-if-exception
+                         (variable-ref variable))))
+           (if (package? package)
+               (proc (package-name package)
+                     (package-version package)
+                     result
+                     #:outputs (package-outputs package)
+                     #:synopsis (package-synopsis package)
+                     #:description (package-description package)
+                     #:location (package-location package)
+                     #:module (module-name module)
+                     #:symbol symbol
+                     #:supported?
+                     (->bool (supported-package? package))
+                     #:deprecated?
+                     (->bool
+                      (package-superseded package)))
+               result)))
+       init
+       (all-modules (%package-module-path)
+                    #:warn
+                    warn-about-load-error))))
 
 (define* (fold-packages proc init
                         #:optional
@@ -268,6 +286,7 @@  package names.  Return #f on failure."
            (fold (lambda (item vhash)
                    (match item
                      (#(name version module symbol outputs
+                             synopsis description
                              supported? deprecated?
                              file line column)
                       (vhash-cons name item vhash))))
@@ -316,7 +335,7 @@  decreasing version order."
   (if (and (cache-is-authoritative?) cache)
       (match (cache-lookup cache name)
         (#f #f)
-        ((#(_ versions modules symbols _ _ _ _ _ _) ...)
+        ((#(_ versions modules symbols _ _ _ _ _ _ _ _) ...)
          (fold (lambda (version* module symbol result)
                  (if (or (not version)
                          (version-prefix? version version*))
@@ -337,9 +356,8 @@  matching NAME and VERSION."
   (if (and cache (cache-is-authoritative?))
       (match (cache-lookup cache name)
         (#f '())
-        ((#(name versions modules symbols outputs
-                 supported? deprecated?
-                 files lines columns) ...)
+        ((#(_ versions _ _ _ _ _ _ _
+              files lines columns) ...)
          (fold (lambda (version* file line column result)
                  (if (and file
                           (or (not version)
@@ -393,6 +411,8 @@  reducing the memory footprint."
                             ,(module-name module)
                             ,symbol
                             ,(package-outputs package)
+                            ,(package-synopsis package)
+                            ,(package-description package)
                             ,(->bool (supported-package? package))
                             ,(->bool (package-superseded package))
                             ,@(let ((loc (package-location package)))