diff mbox series

[bug#30053,1/3] Improve appearance of tabular output.

Message ID 87im1cnnjb.fsf_-_@gmail.com
State Accepted
Headers show
Series [bug#30053,1/3] Improve appearance of tabular output. | expand

Commit Message

Maxim Cournoyer July 15, 2021, 5:39 a.m. UTC 
Hello!

ludo@gnu.org (Ludovic Courtès) writes:

> Steve Sprang <steve.sprang@gmail.com> skribis:
>
>> On Fri, Jan 12, 2018 at 5:28 AM, Roel Janssen <roel@gnu.org> wrote:
>>> If we use GNU awk instead of cut, I think any whitespace will work:
>>>   $ guix package -A | awk '{ print $1 "@" $2 }'
>>>
>>> And then we can optimize the output reading experience for our users
>>> instead of for the 'cut' program.
>>
>> I like this proposal, unless there is a strong reason to prefer 'cut'?
>
> Again a matter of taste, but ‘cut’ looks to me both easier and simpler
> than awk (since it’s a full language).
>
> But anyway, as Danny write, if your patches retain tabs (in addition to
> spaces), presumably it’s OK even for those of us who prefer ‘cut’,
> right?
>
> Thanks,
> Ludo’.

I rebased this set of patch, and modified them slightly (attached).  One
thing that got my attention is the performance.  For short lists of
packages, it's invisible, but it takes noticeably longer for 'guix
package -A', for example.  I'm not sure where the time gets spent (see:
https://paste.debian.net/1204412/).

This is for guix package -A:

--8<---------------cut here---------------start------------->8---
%     cumulative   self             
time   seconds     seconds  procedure
 17.28     37.22      3.61  guix/memoization.scm:100:0
  9.52      2.25      1.99  set-procedure-property!
  4.23      1.14      0.89  ice-9/vlist.scm:539:0:vhash-assq
  3.70      0.77      0.77  ice-9/popen.scm:183:0:reap-pipes
  3.00      0.63      0.63  ice-9/eval.scm:604:6
  2.82      0.66      0.59  open-output-string
  2.65      1.36      0.55  srfi/srfi-1.scm:1028:0:lset-intersection
  2.29      0.48      0.48  write-char
  2.12      0.44      0.44  display
  1.94      0.44      0.41  ice-9/boot-9.scm:2217:0:%load-announce
  1.59      0.33      0.33  hash-ref
  1.59      0.33      0.33  hashq
  1.41      0.41      0.30  ice-9/vlist.scm:449:0:vhash-cons
  1.23      3.58      0.26  ice-9/format.scm:113:2:format:format-work
  1.23      2.99      0.26  ice-9/format.scm:39:0:format
  1.23      0.33      0.26  srfi/srfi-1.scm:1033:17
  1.06      0.30      0.22  guix/packages.scm:924:6:mproc
  1.06      0.22      0.22  string=?
  1.06      0.22      0.22  hash-set!
  1.06      0.22      0.22  procedure?
  1.06      0.22      0.22  ice-9/boot-9.scm:3569:0:autoload-done-or-in-progress?
  1.06      0.22      0.22  append
  1.06      0.22      0.22  reverse!
  0.88      2.80      0.18  guix/build-system/cargo.scm:246:0:lower
  0.88      0.37      0.18  ice-9/eval.scm:297:11
  0.88      0.18      0.18  list?
  0.71     43.08      0.15  ice-9/eval.scm:292:11
  0.71     24.34      0.15  guix/packages.scm:926:16
  0.71      0.18      0.15  make-string
  0.53    246.72      0.11  ice-9/threads.scm:388:4
  0.53     32.97      0.11  guix/packages.scm:924:6
  0.53      2.07      0.11  ice-9/eval.scm:159:9
  0.53      1.33      0.11  ice-9/format.scm:759:2:format:out-obj-padded
  0.53      0.15      0.11  get-output-string
  0.53      0.11      0.11  ice-9/eval.scm:126:12
  0.53      0.11      0.11  reverse
  [...]
---
Sample count: 567
Total time: 20.913633405 seconds (12.747006885 seconds in GC)
--8<---------------cut here---------------end--------------->8---

Without the change 'guix package -A' runs in about 2 seconds.  With the
change it runs in about 12 seconds here.

Danny's suggestion to use 'guix package -A | columns -t' works too, but
it's not convenient nor discoverable.

Any opinions?  Otherwise I might throw a coin, as I'm 50/50 on this.
Thanks,

Maxim

Comments

Sarah Morgensen July 15, 2021, 10:05 p.m. UTC | #1 
Hi Maxim,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> [...]
> ---
> Sample count: 567
> Total time: 20.913633405 seconds (12.747006885 seconds in GC)
>
> Without the change 'guix package -A' runs in about 2 seconds.  With the
> change it runs in about 12 seconds here.

I cannot replicate this. Without the patch on master (7e0da2f):

--8<---------------cut here---------------start------------->8---
$ time ./pre-inst-env guix package -A > /dev/null

real	0m5.473s
user	0m6.698s
sys	0m0.094s
--8<---------------cut here---------------end--------------->8---

And with the patch:

--8<---------------cut here---------------start------------->8---
$ time ./pre-inst-env guix package -A > /dev/null

real	0m5.778s
user	0m6.862s
sys	0m0.061s
--8<---------------cut here---------------end--------------->8---

Perhaps there's something else going on there? I'm on x86-64, if that's
useful.

> Danny's suggestion to use 'guix package -A | columns -t' works too, but
> it's not convenient nor discoverable.

Definitely agree, though in my opinion neither that nor this *really*
make `guix package --list-installed` pretty. I'm sure I could put
together an alias but it goes a long way toward making Guix look
polished to have it built-in.

>
> Any opinions?  Otherwise I might throw a coin, as I'm 50/50 on this.
>
>
>
>
>
> Thanks,
>
> Maxim

--
Sarah
Maxim Cournoyer July 16, 2021, 1:25 a.m. UTC | #2 
Hello,

Sarah Morgensen <iskarian@mgsn.dev> writes:

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
>
>> [...]
>> ---
>> Sample count: 567
>> Total time: 20.913633405 seconds (12.747006885 seconds in GC)
>>
>> Without the change 'guix package -A' runs in about 2 seconds.  With the
>> change it runs in about 12 seconds here.
>
> I cannot replicate this. Without the patch on master (7e0da2f):
>
> $ time ./pre-inst-env guix package -A > /dev/null
>
> real	0m5.473s
> user	0m6.698s
> sys	0m0.094s
>
>
> And with the patch:
>
> $ time ./pre-inst-env guix package -A > /dev/null
>
> real	0m5.778s
> user	0m6.862s
> sys	0m0.061s

I tested on a different machine after tweakwing the code slightly today,
and the results were not as bad as those I reported earlier.  Buffering
the output helped a lot.

> Perhaps there's something else going on there? I'm on x86-64, if that's
> useful.
>
>> Danny's suggestion to use 'guix package -A | columns -t' works too, but
>> it's not convenient nor discoverable.
>
> Definitely agree, though in my opinion neither that nor this *really*
> make `guix package --list-installed` pretty. I'm sure I could put
> together an alias but it goes a long way toward making Guix look
> polished to have it built-in.

Thanks for sharing your opinion!  I ended up pushing it to the repo a
bit earlier today; after I found it was running acceptably fast and set
an upper limit to the maximum column width so that the output would
remain compact enough.

You should have it available to try on your next 'guix pull', if you
haven't already :-).

Thanks,

Maxim
diff mbox series

Patch

From 43a99413e4a59ed32887b8b0552353637f2b7304 Mon Sep 17 00:00:00 2001
From: Steve Sprang <scs@stevesprang.com>
Date: Tue, 9 Jan 2018 14:20:12 -0800
Subject: [PATCH 3/4] ui: Improve output appearance when listing generations.

* guix/ui.scm (display-profile-content-diff): Use pretty-print-table to format
output.
(display-profile-content): Likewise.
---
 guix/ui.scm | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/guix/ui.scm b/guix/ui.scm
index 26a437e904..1428c254b3 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -16,6 +16,7 @@ 
 ;;; Copyright © 2019, 2021 Simon Tournier <zimon.toutoune@gmail.com>
 ;;; Copyright © 2020 Arun Isaac <arunisaac@systemreboot.net>
 ;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
+;;; Copyright © 2018 Steve Sprang <scs@stevesprang.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -1889,10 +1890,10 @@  DURATION-RELATION with the current time."
   (define (equal-entry? first second)
     (string= (manifest-entry-item first) (manifest-entry-item second)))
 
-  (define (display-entry entry prefix)
+  (define (make-row entry prefix)
     (match entry
       (($ <manifest-entry> name version output location _)
-       (format #t " ~a ~a\t~a\t~a\t~a~%" prefix name version output location))))
+       (list (format #f " ~a ~a" prefix name) version output location))))
 
   (define (list-entries number)
     (manifest-entries (profile-manifest (generation-file-name profile number))))
@@ -1903,8 +1904,8 @@  DURATION-RELATION with the current time."
                   equal-entry? (list-entries new) (list-entries old)))
           (removed (lset-difference
                     equal-entry? (list-entries old) (list-entries new))))
-      (for-each (cut display-entry <> "+") added)
-      (for-each (cut display-entry <> "-") removed)
+      (pretty-print-table (append (map (cut make-row <> "+") added)
+                                  (map (cut make-row <> "-") removed)))
       (newline)))
 
   (display-diff profile gen1 gen2))
@@ -1932,15 +1933,17 @@  already taken."
 (define (display-profile-content profile number)
   "Display the packages in PROFILE, generation NUMBER, in a human-readable
 way."
-  (for-each (match-lambda
-              (($ <manifest-entry> name version output location _)
-               (format #t "  ~a\t~a\t~a\t~a~%"
-                       name version output location)))
-
-            ;; Show most recently installed packages last.
-            (reverse
-             (manifest-entries
-              (profile-manifest (generation-file-name profile number))))))
+
+  (define entry->row
+    (match-lambda
+      (($ <manifest-entry> name version output location _)
+       (list (string-append "  " name) version output location))))
+
+  (let* ((manifest (profile-manifest (generation-file-name profile number)))
+         (entries  (manifest-entries manifest))
+         (rows     (map entry->row entries)))
+    ;; Show most recently installed packages last.
+    (pretty-print-table (reverse rows))))
 
 (define (display-generation-change previous current)
   (format #t (G_ "switched from generation ~a to ~a~%") previous current))
-- 
2.32.0