Subject: Re: [bug#43159] [PATCH 1/2] scripts: Use 'define-command' and have 'guix help' use that.

From: Ludovic Courtès <[email-masked]>
Date: Thu, 03 Sep 2020 15:41:41 +0200

Hi Maxim,

Maxim Cournoyer <[email-masked]> skribis:

> Ludovic Courtès <[email-masked]> writes:

[...]

>> +;; Syntactic keywords.
>> +(define synopsis 'command-synopsis)
>> +(define category 'command-category)
>
> Are these definition really necessary/useful?  I would have thought
> having category and synopsis understood as literals in the
> define-command syntax was enough?

It’s not strictly necessary but it’s been considered “good practice”.
That allows users to detect name clashes, to rename/hide/etc. syntactic
keywords and so on.

>> +(define-syntax define-command
>> +  (syntax-rules (category synopsis)

[...]

>> -(define (guix-archive . args)
>> +(define-command (guix-archive . args)
>> +  (category advanced)
>
> It'd be helpful if the category was an enum to keep the set of
> categories focused and helpful.

Yes, I thought about making it a syntactic keyword; let’s see.

>> +  ;; The strategy here is to parse FILE.  This is much cheaper than a
>> +  ;; technique based on run-time introspection where we'd load FILE and all
>> +  ;; the modules it depends on.
>
> Interesting! Have you measure it?  I would have thought loading a couple
> optimized byte code modules could have been nearly as fast as parsing
> files manually.  If so, I think it'd be preferable to use introspection
> rather than implement a custom parser.

On a fast recent laptop with an SSD, a load of 0, hot cache, etc., we’d
still be below 1s.  But see:

--8<---------------cut here---------------start------------->8---
$ strace -c guix help >/dev/null
% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ------------------
 62.69    0.002698           1      2266      2043 stat
 10.94    0.000471           2       161         2 lstat
  4.55    0.000196           0       246           mmap
  4.51    0.000194           0       330       172 openat

[...]

------ ----------- ----------- --------- --------- ------------------
100.00    0.004304           1      3748      2235 total
$ strace -c guile -c '(use-modules (guix scripts system) (guix scripts authenticate))'
% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ------------------
 54.27    0.007799           1      5735      4518 stat
 12.00    0.001724          11       149        27 futex
  9.06    0.001302           0      1328       651 openat
  7.24    0.001040           1       822           mmap

[...]

------ ----------- ----------- --------- --------- ------------------
100.00    0.014371           1     10334      5202 total
--8<---------------cut here---------------end--------------->8---

(The 1st run is the current ‘guix help’; the 2nd run +/- emulates what
you propose.)

Loading all the modules translates into a lot more I/O, roughly an order
of magnitude.  We’re talking about loading tens of modules just to get
at that synopsis:

--8<---------------cut here---------------start------------->8---
scheme@(guile-user)> ,use(guix modules)
scheme@(guile-user)> (length (source-module-closure '((guix scripts system) (guix scripts authenticate))))
$10 = 439
scheme@(guile-user)> (length (source-module-closure '((guix scripts) (guix ui))))
$11 = 31
--8<---------------cut here---------------end--------------->8---

Memory usage would also be very different:

--8<---------------cut here---------------start------------->8---
$ \time guix help >/dev/null
0.07user 0.01system 0:00.06elapsed 128%CPU (0avgtext+0avgdata 35348maxresident)k
0inputs+0outputs (0major+3906minor)pagefaults 0swaps
$ \time guile -c '(use-modules (guix scripts system) (guix scripts authenticate))'
0.42user 0.05system 0:00.37elapsed 128%CPU (0avgtext+0avgdata 166916maxresident)k
0inputs+0outputs (0major+15148minor)pagefaults 0swaps
--8<---------------cut here---------------end--------------->8---

In summary, while this approach undoubtedly looks awkward to any Lisper,
I think it’s a good way to not contribute to the general impression of
sluggishness and resource-hungriness of ‘guix’ commands.  :-)

>> +  (define (display-commands commands)
>> +    (let* ((names     (map (lambda (command)
>> +                             (string-join (command-name command)))
>> +                           commands))
>> +           (max-width (reduce max 0 (map string-length names))))
>
> You can drop reduce and use (max (map string-length names)) instead.

I could do (apply max (map …)) but I don’t like the idea of abusing
variadic argument lists in that way—I know, it’s very subjective.  ;-)

Thanks for your feedback, I’ll send a v2!

Ludo’.