bug#25122: 24.5; function describe-variable hangs on large variables

bug#25122: 24.5; function describe-variable hangs on large variables

[Boruch Baum]

Subject: 24.5; function describe-variable hangs on large variables

1) When evaluating function describe-variable for variable
package-archive-conteqnts, emacs hangs for minutes before I gave up.

2) Aborting vua C-g works.

3) Viewing the buffer list revealed that a *Help* buffer had begun to be
created. Its content was "package-archive-contents is a variable defined
in `package.el'. Its value is " (new-lines removed).

4) If emacs is trying to stuff into that variable (and into that *Help*
buffer) all the archive information from the archive files of my
~/.emacs.d/elpa/archives/ tree, that would be about 730kb of elisp.

In GNU Emacs 24.5.1 (x86_64-pc-linux-gnu)
 of 2016-03-19 on trouble, modified by Debian
System Description:	Devuan GNU/Linux 1.0 (jessie)

Configured using:
 `configure --build x86_64-linux-gnu --prefix=/usr
 --sharedstatedir=/var/lib --libexecdir=/usr/lib
 --localstatedir=/var/lib --infodir=/usr/share/info
 --mandir=/usr/share/man --with-pop=yes
 --enable-locallisppath=/etc/emacs24:/etc/emacs:/usr/local/share/emacs/24.5/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/24.5/site-lisp:/usr/share/emacs/site-lisp
 --build x86_64-linux-gnu --prefix=/usr --sharedstatedir=/var/lib
 --libexecdir=/usr/lib --localstatedir=/var/lib
 --infodir=/usr/share/info --mandir=/usr/share/man --with-pop=yes
 --enable-locallisppath=/etc/emacs24:/etc/emacs:/usr/local/share/emacs/24.5/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/24.5/site-lisp:/usr/share/emacs/site-lisp
 --with-x=no --without-gconf --without-gsettings 'CFLAGS=-g -O2
 -fstack-protector-strong -Wformat -Werror=format-security -Wall'
 CPPFLAGS=-D_FORTIFY_SOURCE=2 LDFLAGS=-Wl,-z,relro'

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
  global-anzu-mode: t
  anzu-mode: t
  ws-butler-mode: t
  dtrt-indent-mode: t
  clean-aindent-mode: t
  yas-minor-mode: t
  global-undo-tree-mode: t
  undo-tree-mode: t
  volatile-highlights-mode: t
  global-ede-mode: t
  ede-minor-mode: t
  global-semantic-idle-scheduler-mode: t
  global-semanticdb-minor-mode: t
  async-bytecomp-package-mode: t
  global-semantic-stickyfunc-mode: t
  semantic-mode: t
  helm-mode: t
  shell-dirtrack-mode: t
  projectile-mode: t
  global-company-mode: t
  company-mode: t
  override-global-mode: t
  winner-mode: t
  show-paren-mode: t
  savehist-mode: t
  desktop-save-mode: t
  delete-selection-mode: t
  tooltip-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  size-indication-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t


Features:
(shadow sort mail-extr eieio-opt emacsbug helm-command tramp-cache
conf-mode org-element org-rmail org-mhe org-irc org-info org-gnus
org-docview doc-view image-mode org-bibtex bibtex org-bbdb org-w3m
misearch multi-isearch zygospore sh-script smie executable setup-editing
help-macro sgml-mode iedit-lib rect anzu mule-util ws-butler benchmark
dtrt-indent clean-aindent-mode yasnippet undo-tree diff
volatile-highlights ede/cpp-root ede/emacs setup-cedet ede/speedbar
ede/files ede ede/base ede/auto ede/source eieio-speedbar speedbar
sb-image dframe eieio-custom wid-edit semantic/idle semantic/format
ezimage semantic/tag-ls semantic/find semantic/ctxt semantic/db-mode
semantic/db eieio-base cc-mode cc-fonts cc-guess cc-menus cc-cmds
cc-styles cc-align cc-engine cc-vars cc-defs setup-helm-gtags helm-gtags
subr-x pulse which-func setup-helm helm-projectile helm-config
async-bytecomp helm-imenu semantic/util-modes semantic/util semantic
semantic/tag semantic/lex semantic/fw mode-local cedet org org-macro
org-footnote org-pcomplete org-list org-faces org-entities noutline
outline org-version ob-sh ob-awk ob-latex ob-emacs-lisp ob ob-tangle
ob-ref ob-lob ob-table ob-exp org-src ob-keys ob-comint ob-core ob-eval
org-compat org-macs org-loaddefs cal-menu calendar cal-loaddefs imenu
helm-easymenu helm-mode helm-elisp helm-files tramp tramp-compat
tramp-loaddefs trampver shell pcomplete ffap helm-buffers helm-tags
helm-bookmark helm-locate helm-eval edebug eldoc helm-grep helm-regexp
helm-elscreen helm-adaptive helm-info info helm-types helm-external
helm-net browse-url xml helm-utils helm-help helm helm-source
helm-multi-match helm-lib smtpmail sendmail async setup-general windmove
projectile skeleton grep ibuf-ext thingatpt json epl rx company-oddmuse
company-keywords company-etags company-gtags company-dabbrev-code
company-files company-capf company-cmake company-xcode company-clang
company-semantic company-eclim company-css company-nxml company-bbdb
tempo ispell etags find-func company-dabbrev company-template company
tar-mode use-package cl diminish bind-key compile comint tool-bar
autoload lisp-mnt finder-inf mm-archive message rfc822 mml mml-sec
mailabbrev gmm-utils mailheader mm-decode mm-bodies mm-encode mail-utils
gnutls network-stream starttls url-http tls mail-parse rfc2231 rfc2047
rfc2045 ietf-drums url-gw url-cache url-auth url url-proxy url-privacy
url-expand url-methods url-history url-cookie url-domsuf url-util
mailcap url-handlers url-parse auth-source eieio byte-opt bytecomp
byte-compile cl-extra cconv eieio-core gnus-util mm-util mail-prsvr
password-cache url-vars epg xterm server warnings dired-details+
dired-details help-mode advice help-fns dired+ image-dired image-file
image dired-x dired-aux dired winner ring pcase git-blame format-spec
package epg-config bookmark cl-macs gv derived pp jka-compr ibuf-macs
ibuffer paren woman man easymenu regexp-opt ansi-color edmacro kmacro
time-date savehist desktop frameset cl-loaddefs cl-lib elec-pair delsel
tango-dark-theme debian-el debian-el-loaddefs w3m-load emacs-goodies-el
emacs-goodies-custom emacs-goodies-loaddefs easy-mmode devhelp tooltip
electric uniquify ediff-hook vc-hooks lisp-float-type tabulated-list
newcomment lisp-mode prog-mode register page menu-bar rfn-eshadow timer
select mouse jit-lock font-lock syntax facemenu font-core frame cham
georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese hebrew greek romanian slovak czech european ethiopic
indian cyrillic chinese case-table epa-hook jka-cmpr-hook help simple
abbrev minibuffer nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote make-network-process
dbusbind gfilenotify multi-tty emacs)

Memory information:
((conses 16 582332 490741)
 (symbols 48 55652 20)
 (miscs 40 421 1843)
 (strings 32 146045 231132)
 (string-bytes 1 4264772)
 (vectors 16 55051)
 (vector-slots 8 1598839 288778)
 (floats 8 286 3294)
 (intervals 56 4525 912)
 (buffers 960 27)
 (heap 1024 62515 75886))

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#25122: 24.5; function describe-variable hangs on large variables

[Thierry Volpiatto]

Boruch Baum <boruch_baum@gmx.com> writes:

> Subject: 24.5; function describe-variable hangs on large variables
>
> 1) When evaluating function describe-variable for variable
> package-archive-conteqnts, emacs hangs for minutes before I gave up.

I have already reported this bug.
I use this to workaround it:

    (progn
        ;; Speedup `describe-variable' for variables with huge value description.
        (defun tv/describe-variable (old-fn &rest args)
          ;; `cl-flet' can't be used here because `pp' should
          ;; appear lexically in its body, which is not the case.
          ;; Using `flet' is an option, but even better is binding
          ;; (symbol-function 'pp) with `cl-letf'.
          (cl-letf (((symbol-function 'pp)
                     (lambda (object &optional stream)
                       (let ((fn (lambda (ob &optional stream)
                                   (princ (pp-to-string ob)
                                          (or stream standard-output))
                                   (terpri)))
                             (print-circle t))
                         (if (consp object)
                             (progn
                               (insert "\n(")
                               (mapc fn object)
                               (cl-letf (((point) (1- (point))))
                                 (insert ")")))
                             (funcall fn object stream))))))
            (apply old-fn args)))
        (advice-add 'describe-variable :around #'tv/describe-variable))

bug#25122: 24.5; function describe-variable hangs on large variables

[npostavs]

merge 13439 25122
quit

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Boruch Baum <boruch_baum@gmx.com> writes:
>
>> Subject: 24.5; function describe-variable hangs on large variables
>>
>> 1) When evaluating function describe-variable for variable
>> package-archive-conteqnts, emacs hangs for minutes before I gave up.
>
> I have already reported this bug.
> I use this to workaround it:

I wonder if the suggestion in #13439 might also help?

bug#25122: 24.5; function describe-variable hangs on large variables

[Thierry Volpiatto]

npostavs@users.sourceforge.net writes:

> merge 13439 25122
> quit
>
> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>> Boruch Baum <boruch_baum@gmx.com> writes:
>>
>>> Subject: 24.5; function describe-variable hangs on large variables
>>>
>>> 1) When evaluating function describe-variable for variable
>>> package-archive-conteqnts, emacs hangs for minutes before I gave up.
>>
>> I have already reported this bug.
>> I use this to workaround it:
>
> I wonder if the suggestion in #13439 might also help?

I don't think it would be as fast as printing one by one each elements.
What is slow is printing the whole object.
See how bookmark file is saved, it was taking more than one minute for
my bookmarks before printing one by one, now it is instant or nearly.
  
-- 
Thierry

bug#25122: 24.5; function describe-variable hangs on large variables

[npostavs]

[-- Attachment #1: Type: text/plain, Size: 1578 bytes --]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
> I don't think it would be as fast as printing one by one each elements.
> What is slow is printing the whole object.
> See how bookmark file is saved, it was taking more than one minute for
> my bookmarks before printing one by one, now it is instant or nearly.

Actually, it's the indent-sexp on the whole object that takes time.
Possibly we could sacrifice some indentation correctness if the printed
representation is big.

I've been attempting an alternate approach which prettyprints the object
while scanning it instead of the current way of printing and then
reindenting.  Without optimizing, it's about 3 times as fast as the
current pp (it's the pp-prin1 in the benchmarks below), though more than
3 times slower than your mapc pp trick.  On the other hand, it also
doesn't yet handle function-specific indentation or any compound
structure apart from lists, so I'm not sure if it will end up being much
faster.

(benchmark 1 '(with-temp-buffer (pp-prin1 long-list (current-buffer)) nil)) "Elapsed time: 3.391232s (0.565806s in 11 GCs)"
(benchmark 1 '(progn (pp-to-string long-list) nil))                         "Elapsed time: 9.988515s (0.148034s in 3 GCs)"
(benchmark 1 '(progn (with-output-to-string (mapc 'pp long-list)) nil))     "Elapsed time: 0.983493s (0.144424s in 3 GCs)"
(benchmark 1 '(progn (cl-prin1-to-string long-list) nil))                   "Elapsed time: 0.511617s (0.152483s in 3 GCs)"
(benchmark 1 '(progn (prin1-to-string long-list) nil))                      "Elapsed time: 0.029320s"


[-- Attachment #2: draft patch --]
[-- Type: text/plain, Size: 7056 bytes --]

From 66f6dda0507fa4699ba929379cd1c58ef8b540f5 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sat, 11 Mar 2017 00:09:36 -0500
Subject: [PATCH v1] Initial draft of new pretty printer (Bug#25122)

* lisp/emacs-lisp/pp.el (pp-state): New struct.
(pp--scan): New function, measures length of sublists (actually
"logical blocks" to allow for more customizable grouping than just by
lists).  Calls pp--print when scanned tokens are too wide to fit on a
single line.
(pp--print): New function, prints tokens horizontally or vertically
depending on whether the sublist can fit within the line.
(pp-prin1): New function, entry point for pp--scan and pp-print.
Wraps stream so that cl-print will dispatch to prettyprinting methods.
(cl-print-object) <_ (head :pprint)>: New method, wraps
cl-prin1-to-string for prettyprinting.
(cl-print-object) <cons (head :pprint)>: New mthod, prettyprinter for
lists.
---
 lisp/emacs-lisp/pp.el | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 143 insertions(+)

diff --git a/lisp/emacs-lisp/pp.el b/lisp/emacs-lisp/pp.el
index 7ef46a48bd..3809325c4b 100644
--- a/lisp/emacs-lisp/pp.el
+++ b/lisp/emacs-lisp/pp.el
@@ -24,6 +24,9 @@
 
 ;;; Code:
 
+(eval-when-compile (require 'cl-lib))
+(require 'ring)
+
 (defvar font-lock-verbose)
 
 (defgroup pp nil
@@ -121,6 +124,146 @@ pp-display-expression
 	(setq buffer-read-only nil)
 	(set (make-local-variable 'font-lock-verbose) nil)))))
 
+
+(cl-defstruct (pp-state (:constructor
+                         make-pp-state
+                         (stream
+                          &aux
+                          (right-margin fill-column)
+                          (left-margin 0)
+                          (indent '(0))
+                          (scan-depth 0)
+                          (print-depth 0)
+                          (print-width 0)
+                          (scan-width 0)
+                          (block-mode (list nil))
+                          (fifo (make-ring 30)))))
+  stream
+  right-margin ; how far we may go.
+  left-margin  ; how far printer has gone
+  print-width ; total width of tokens printed so far.
+  indent ; left-margin, stack per depth.
+  scan-width ; total width of tokens scanned so far.
+  scan-depth
+  print-depth
+  block-widths
+  block-mode ; `:vertical', `:horizontal', nil (undecided); stack per depth.
+  fifo
+  )
+
+(defun pp--print (state)
+  (cl-symbol-macrolet ((stream (pp-state-stream state))
+                       (depth (pp-state-print-depth state))
+                       (scan-depth (pp-state-scan-depth state))
+                       (fifo (pp-state-fifo state))
+                       (left-margin (pp-state-left-margin state))
+                       (width (pp-state-print-width state))
+                       (indent (pp-state-indent state))
+                       (right-margin (pp-state-right-margin state))
+                       (block-mode (pp-state-block-mode state)))
+    (catch 'rescan
+      (while (not (ring-empty-p fifo))
+        (pcase (ring-remove fifo)
+          ((and `(,len . :open-block) token)
+           (if (<= len 0)
+               ;; Not ready to print this yet!
+               (progn (ring-insert-at-beginning fifo token)
+                      (throw 'rescan nil))
+             (cl-incf depth)
+             (push left-margin indent)
+             (push (if (> (+ left-margin len) right-margin)
+                       :vertical :horizontal)
+                   block-mode)))
+          (:close-block (cl-decf depth) (pop indent) (pop block-mode))
+          (:blank
+           (pcase (car block-mode)
+            (:vertical
+             (terpri stream)
+             (princ (make-string (car indent) ?\s) stream)
+             (setf left-margin (car indent)))
+            ((or :horizontal 'nil)
+             (write-char ?\s stream)
+             (cl-incf left-margin))
+            (_ (error "oops")))
+           (cl-incf width))
+          (:eof nil)
+          ((and (pred characterp) char)
+           (write-char char stream)
+           (cl-incf left-margin (char-width char))
+           (cl-incf width (char-width char)))
+          (string
+           (princ string stream)
+           (cl-incf left-margin (string-width string))
+           (cl-incf width (string-width string))))))))
+
+(defun pp--scan (token state)
+  (cl-symbol-macrolet ((stream (pp-state-stream state))
+                       (depth (pp-state-scan-depth state))
+                       (print-depth (pp-state-print-depth state))
+                       (fifo (pp-state-fifo state))
+                       (width (pp-state-scan-width state))
+                       (right-margin (pp-state-right-margin state))
+                       (block-widths (pp-state-block-widths state)))
+    (cl-flet ((scanlen (len) (cl-incf width len)))
+      (cl-assert (> (ring-size fifo) (ring-length fifo)))
+      (ring-insert fifo token)
+      (pcase token
+        (:open-block
+         (cl-incf depth)
+         (let ((block-token (cons (- width) (ring-remove fifo 0))))
+           (push block-token block-widths)
+           (ring-insert fifo block-token)))
+        (:close-block
+         (cl-incf (caar block-widths) width)
+         (when (> (caar block-widths) right-margin)
+           (pp--print state))
+         (cl-decf depth)
+         (pop block-widths))
+        (:blank (scanlen 1))
+        (:eof (pp--print state))
+        ((pred characterp) (scanlen (char-width token)))
+        (_ (scanlen (string-width token)))))
+    (when block-widths
+      (when (> (+ (caar block-widths) width) right-margin)
+        (dolist (block-width block-widths)
+          (setf (car block-width) (+ right-margin 1))))
+      (when (> (caar block-widths) right-margin)
+        (pp--print state)))))
+
+(defvar cl-print-readably) ; cl-print.el
+
+(defun pp-prin1 (object &optional stream)
+  (let ((cl-print-readably nil)
+        (stream (make-pp-state (or stream standard-output))))
+    (pp--scan :open-block stream)
+    (prog1 (cl-prin1 object (cons :pprint stream))
+      (pp--scan :close-block stream)
+      (pp--scan :eof stream))))
+
+;; fallback to standard `cl-print-object'.
+(cl-defmethod cl-print-object (object (stream (head :pprint)))
+  (pp--scan (cl-prin1-to-string object) (cdr stream))
+  object)
+
+(cl-defmethod cl-print-object ((list cons) (stream (head :pprint)))
+  (let ((state (cdr stream)))
+    (pcase list
+      (`(,head . ,tail)
+       (pp--scan "(" state)
+       (pp--scan :open-block state)
+       (cl-print-object head stream)
+       (while (consp tail)
+         (pp--scan :blank state)
+         (cl-print-object (pop tail) stream))
+       (when tail
+         (pp--scan :blank state)
+         (pp--scan ?\. state)
+         (pp--scan :blank state)
+         (cl-print-object tail stream))
+       (pp--scan :close-block state)
+       (pp--scan ")" state))))
+  list)
+
 ;;;###autoload
 (defun pp-eval-expression (expression)
   "Evaluate EXPRESSION and pretty-print its value.
-- 
2.11.1

bug#25122: 24.5; function describe-variable hangs on large variables

[Stefan Monnier]

>           (cl-letf (((symbol-function 'pp)
>                      (lambda (object &optional stream)
>                        (let ((fn (lambda (ob &optional stream)
>                                    (princ (pp-to-string ob)
>                                           (or stream standard-output))
>                                    (terpri)))
>                              (print-circle t))
>                          (if (consp object)
>                              (progn
>                                (insert "\n(")
>                                (mapc fn object)
>                                (cl-letf (((point) (1- (point))))
>                                  (insert ")")))
>                              (funcall fn object stream))))))

Hmm... I wonder why this would be faster.  In the past, the
implementation of `print-circle` had a poor complexity, but we fixed
that around Emacs-24, IIRC so it now uses a hash-table and should have
O(n) complexity, which means that pp shouldn't be slower than (mapc
#'pp).


        Stefan

bug#25122: 24.5; function describe-variable hangs on large variables

[Stefan Monnier]

> Actually, it's the indent-sexp on the whole object that takes time.
> Possibly we could sacrifice some indentation correctness if the printed
> representation is big.

Actually, I think that trying to optimize this is bound to fail: we want
*Help* to show up "instantly", so we're off by a factor of more
than 100, which seems way out of reach (unless there's really an
algorithmic problem in our code, admittedly).

I think the better way out is to do less work.  E.g. bound the total
amount printed (and replace the rest with "..." that can be expanded on
demand).


        Stefan

bug#25122: 24.5; function describe-variable hangs on large variables

[npostavs]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>           (cl-letf (((symbol-function 'pp)
>>                      (lambda (object &optional stream)
>>                        (let ((fn (lambda (ob &optional stream)
>>                                    (princ (pp-to-string ob)
>>                                           (or stream standard-output))
>>                                    (terpri)))
>>                              (print-circle t))
>>                          (if (consp object)
>>                              (progn
>>                                (insert "\n(")
>>                                (mapc fn object)
>>                                (cl-letf (((point) (1- (point))))
>>                                  (insert ")")))
>>                              (funcall fn object stream))))))
>
> Hmm... I wonder why this would be faster.  In the past, the
> implementation of `print-circle` had a poor complexity, but we fixed
> that around Emacs-24, IIRC so it now uses a hash-table and should have
> O(n) complexity, which means that pp shouldn't be slower than (mapc
> #'pp).

I think it's because when we indent-sexp only on individual entries, we
don't parse as far back.

bug#25122: 24.5; function describe-variable hangs on large variables

[Thierry Volpiatto]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>           (cl-letf (((symbol-function 'pp)
>>                      (lambda (object &optional stream)
>>                        (let ((fn (lambda (ob &optional stream)
>>                                    (princ (pp-to-string ob)
>>                                           (or stream standard-output))
>>                                    (terpri)))
>>                              (print-circle t))
>>                          (if (consp object)
>>                              (progn
>>                                (insert "\n(")
>>                                (mapc fn object)
>>                                (cl-letf (((point) (1- (point))))
>>                                  (insert ")")))
>>                              (funcall fn object stream))))))
>
> Hmm... I wonder why this would be faster.

However it is fast, it is anyway better than the actual situation were e.g C-h v
load-history takes minutes in the best case to load the 1.8M help buffer.
With this code it takes around 1s.

> In the past, the implementation of `print-circle` had a poor
> complexity, but we fixed that around Emacs-24, IIRC so it now uses a
> hash-table and should have O(n) complexity, which means that pp
> shouldn't be slower than (mapc #'pp).

The code above run at the same speed with and without print-circle...

-- 
Thierry

bug#25122: 24.5; function describe-variable hangs on large variables

[Thierry Volpiatto]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I think the better way out is to do less work.  E.g. bound the total
> amount printed (and replace the rest with "..." that can be expanded on
> demand).

The problem will be here again when you hit RET on "..." 

-- 
Thierry

bug#25122: 24.5; function describe-variable hangs on large variables

[Thierry Volpiatto]

npostavs@users.sourceforge.net writes:

> I've been attempting an alternate approach which prettyprints the object
> while scanning it instead of the current way of printing and then
> reindenting.  Without optimizing, it's about 3 times as fast as the
> current pp (it's the pp-prin1 in the benchmarks below), though more than
> 3 times slower than your mapc pp trick.  On the other hand, it also
> doesn't yet handle function-specific indentation or any compound
> structure apart from lists, so I'm not sure if it will end up being much
> faster.
>
> (benchmark 1 '(with-temp-buffer (pp-prin1 long-list (current-buffer)) nil)) "Elapsed time: 3.391232s (0.565806s in 11 GCs)"
> (benchmark 1 '(progn (pp-to-string long-list) nil))                         "Elapsed time: 9.988515s (0.148034s in 3 GCs)"
> (benchmark 1 '(progn (with-output-to-string (mapc 'pp long-list)) nil))     "Elapsed time: 0.983493s (0.144424s in 3 GCs)"
> (benchmark 1 '(progn (cl-prin1-to-string long-list) nil))                   "Elapsed time: 0.511617s (0.152483s in 3 GCs)"
> (benchmark 1 '(progn (prin1-to-string long-list) nil))                      "Elapsed time: 0.029320s"

Interesting, thanks to work on this.

-- 
Thierry

bug#25122: 24.5; function describe-variable hangs on large variables

[npostavs]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> I think the better way out is to do less work.  E.g. bound the total
>> amount printed (and replace the rest with "..." that can be expanded on
>> demand).

Yes, there's no method that will achieve "instant" speeds for
load-history sized lists (well except for plain prin1, but that output
is hardly readable).

> The problem will be here again when you hit RET on "..." 

Perhaps we could run the printing in a thread and suspend it after
printing X lines.  Then hitting RET on "..." would just print another X
lines.

bug#25122: 24.5; function describe-variable hangs on large variables

[Drew Adams]

> >> I think the better way out is to do less work.  E.g. bound the total
> >> amount printed (and replace the rest with "..." that can be expanded on
> >> demand).
> 
> Yes, there's no method that will achieve "instant" speeds for
> load-history sized lists (well except for plain prin1, but that output
> is hardly readable).
> 
> > The problem will be here again when you hit RET on "..."
> 
> Perhaps we could run the printing in a thread and suspend it after
> printing X lines.  Then hitting RET on "..." would just print another X
> lines.

(Caveat: I'm not really following this thread.)

If you plan to do things like what I think you're suggesting,
which will make a user hit several keys (e.g. RET on ...
repeatedly) then please make that behavior _optional_.

Give users the _possibility_ to just print the whole thing,
without any other action (no prefix arg, no clicking ... etc.).

I generally don't mind waiting, for example, and I don't
want to have to hit ... and wait for a bit more - I want
the whole thing the first time.

And if/when you do show the value only partially, please
make that _obvious_ to users.  They should _immediately_
know that they are not seeing the full value.

bug#25122: 24.5; function describe-variable hangs on large variables

[Thierry Volpiatto]

npostavs@users.sourceforge.net writes:

> Perhaps we could run the printing in a thread

This is a good idea.

> and suspend it after printing X lines.

Instead just print something like "Computing foo value ..." and let finish
the thread, letting user reading and moving cursor in docstring while it
finishes, once done save-excursion and send message "Computing foo value
done".

> Then hitting RET on "..."  would just print another X lines.

I think like Drew that this would be annoying.


That said, what's the reason of choosing the slower approach to compute
value (in a thread or not) instead of using the approach described in
the advice I sent here which takes 1s to compute load-history instead of
3mn ? (I use this advice since one year now without any problems).

-- 
Thierry

bug#25122: 24.5; function describe-variable hangs on large variables

[Stefan Monnier]

> That said, what's the reason of choosing the slower approach to compute
> value (in a thread or not) instead of using the approach described in
> the advice I sent here which takes 1s to compute load-history instead of
> 3mn ? (I use this advice since one year now without any problems).

Yes, we should definitely do that.  I'm still not sure why it's so much
faster: it indicates we have a performance bug in pp.el and all it take
is to fix it.


        Stefan

bug#25122: 24.5; function describe-variable hangs on large variables

[npostavs]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> npostavs@users.sourceforge.net writes:
>
>> and suspend it after printing X lines.
>
> Instead just print something like "Computing foo value ..." and let finish
> the thread, letting user reading and moving cursor in docstring while it
> finishes, once done save-excursion and send message "Computing foo value
> done".

Threads aren't truly parallel.  The user can't do anything while the
thread is running.

>
>> Then hitting RET on "..."  would just print another X lines.
>
> I think like Drew that this would be annoying.

I wonder if we could just hook this into scrolling somehow?  So the
lines would only be printed when you scroll to look at them.

>
> That said, what's the reason of choosing the slower approach to compute
> value (in a thread or not) instead of using the approach described in
> the advice I sent here which takes 1s to compute load-history instead of
> 3mn ? (I use this advice since one year now without any problems).

As mentioned in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=21717#8,
it breaks circularity.  Try describing this variable:

    (defvar circular-list
      (let ((l (number-sequence 1 100)))
        (setcdr (last l) l)
        l)
      "A circular list that has problems with (mapc 'pp val).")

We could probably achieve something similar without breaking circular
printing by not calling indent-sexp on the full list, but 1s is longer
than "instant" anyway (which is about 50ms or less) which is why I'm
exploring other options.

bug#25122: 24.5; function describe-variable hangs on large variables

[Drew Adams]

> >> Then hitting RET on "..."  would just print another X lines.
> >
> > I think like Drew that this would be annoying.
> 
> I wonder if we could just hook this into scrolling somehow?  So the
> lines would only be printed when you scroll to look at them.

I still would not like that behavior, so would would want to
opt out, personally.  If it take a minute to display *Help*
I can at least do something different (outside Emacs) during
that time.  When I'm scrolling I'm likely examining the value
as it scrolls, and I don't want to wait (scrolling in chunks
separated by delays).

E.g., I do `C-h v bookmark-alist' or `load-history' with a
large list.  I know that it will not display immediately,
so I don't grumble about that fact.  I'm free not to sit and
stare at the screen waiting for it to return.  What you
describe just chops up the wait into scrolled chunks.

> > That said, what's the reason of choosing the slower approach to compute
> > value (in a thread or not) instead of using the approach described in
> > the advice I sent here which takes 1s to compute load-history instead of
> > 3mn ? (I use this advice since one year now without any problems).
> 
> As mentioned in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=21717#8,
> it breaks circularity.  Try describing this variable:
> 
>   (defvar circular-list
>     (let ((l (number-sequence 1 100))) (setcdr (last l) l) l) "")
> 
> We could probably achieve something similar without breaking circular
> printing by not calling indent-sexp on the full list, but 1s is longer
> than "instant" anyway (which is about 50ms or less) which is why I'm
> exploring other options.

1 sec is not a problem, IMO.  0.7 sec is a typical Emacs
`sit-for' value, i.e., something that, yes, allows time to
notice the change/wait, but it is not so long that it
becomes annoying.

(It would be annoying if it happened for all or most *Help*
displays, but printing large values is the exception.)

bug#25122: 24.5; function describe-variable hangs on large variables

[npostavs]

[-- Attachment #1: Type: text/plain, Size: 2534 bytes --]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> npostavs@users.sourceforge.net writes:
>
>> I've been attempting an alternate approach which prettyprints the object
>> while scanning it instead of the current way of printing and then
>> reindenting.  Without optimizing, it's about 3 times as fast as the
>> current pp (it's the pp-prin1 in the benchmarks below), though more than
>> 3 times slower than your mapc pp trick.  On the other hand, it also
>> doesn't yet handle function-specific indentation or any compound
>> structure apart from lists, so I'm not sure if it will end up being much
>> faster.
>>
>> (benchmark 1 '(with-temp-buffer (pp-prin1 long-list (current-buffer)) nil)) "Elapsed time: 3.391232s (0.565806s in 11 GCs)"
>> (benchmark 1 '(progn (pp-to-string long-list) nil))                         "Elapsed time: 9.988515s (0.148034s in 3 GCs)"
>> (benchmark 1 '(progn (with-output-to-string (mapc 'pp long-list)) nil))     "Elapsed time: 0.983493s (0.144424s in 3 GCs)"
>> (benchmark 1 '(progn (cl-prin1-to-string long-list) nil))                   "Elapsed time: 0.511617s (0.152483s in 3 GCs)"
>> (benchmark 1 '(progn (prin1-to-string long-list) nil))                      "Elapsed time: 0.029320s"
>
> Interesting, thanks to work on this.

With a couple of minor optimizations it's down to about 1.8s.  The first
is to reuse the tempbuffer instead of letting cl-prin1-to-string make a
new one each time.  Second is to add

    (eval-when-compile
      (cl-proclaim '(optimize (speed 3) (safety 0))))

This also stops these warnings (I guess they're caused by the "safety" code?):

../../emacs-master/lisp/emacs-lisp/pp.el:159:19:Warning: value returned from
    (aref state 6) is unused
../../emacs-master/lisp/emacs-lisp/pp.el:204:24:Warning: value returned from
    (aref state 10) is unused

New times:

(benchmark 1 '(with-temp-buffer (pp-prin1 long-list (current-buffer)) nil)) "Elapsed time: 1.800146s (0.231706s in 6 GCs)"
(benchmark 1 '(progn (pp-to-string long-list) nil))                         "Elapsed time: 9.950225s (0.154100s in 4 GCs)"
(benchmark 1 '(progn (with-output-to-string (mapc 'pp long-list)) nil))     "Elapsed time: 0.980923s (0.149787s in 4 GCs)"

I foolishly neglected to write down what exactly long-list was before,
starting from emacs -Q this seems to approximate it though:

(progn (require 'pp)
       (require 'dabbrev)
       (require 'edebug)
       (require 'cc-mode)
       (require 'vc)
       (setq long-list load-history)
       (length long-list)) ;=> 142


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 7289 bytes --]

From e4b1a2ef3b4b11466e81d639a09ff671318e0968 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sat, 11 Mar 2017 00:09:36 -0500
Subject: [PATCH v2] New pretty printer (Bug#25122)

* lisp/emacs-lisp/pp.el (pp-state): New struct.
(pp--scan): New function, measures length of sublists (actually
"logical blocks" to allow for more customizable grouping than just by
lists).  Calls pp--print when scanned tokens are too wide to fit on a
single line.
(pp--print): New function, prints tokens horizontally or vertically
depending on whether the sublist can fit within the line.
(pp-prin1): New function, entry point for pp--scan and pp-print.
(pp-print-object): New generic function.
(pp-print-object) <cons , _>: New method, prettyprinter for lists.
---
 lisp/emacs-lisp/pp.el | 156 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 156 insertions(+)

diff --git a/lisp/emacs-lisp/pp.el b/lisp/emacs-lisp/pp.el
index 7ef46a48bd..8c2ed24ffd 100644
--- a/lisp/emacs-lisp/pp.el
+++ b/lisp/emacs-lisp/pp.el
@@ -24,6 +24,9 @@
 
 ;;; Code:
 
+(eval-when-compile (require 'cl-lib))
+(require 'ring)
+
 (defvar font-lock-verbose)
 
 (defgroup pp nil
@@ -121,6 +124,159 @@ pp-display-expression
 	(setq buffer-read-only nil)
 	(set (make-local-variable 'font-lock-verbose) nil)))))
 
+(eval-when-compile
+  ;; FIXME: should we try to restore original settings? (how?)
+  (cl-proclaim '(optimize (speed 3) (safety 0))))
+
+(cl-defstruct (pp-state (:constructor
+                         make-pp-state
+                         (stream
+                          tempbuffer
+                          right-margin
+                          &aux
+                          (left-margin 0)
+                          (indent '(0))
+                          (scan-depth 0)
+                          (print-depth 0)
+                          (print-width 0)
+                          (scan-width 0)
+                          (block-mode (list nil))
+                          (fifo (make-ring 30)))))
+  stream
+  tempbuffer
+  right-margin ; how far we may go.
+  left-margin  ; how far printer has gone
+  print-width ; total width of tokens printed so far.
+  indent ; left-margin, stack per depth.
+  scan-width ; total width of tokens scanned so far.
+  scan-depth
+  print-depth
+  block-widths
+  block-mode ; `:vertical', `:horizontal', nil (undecided); stack per depth.
+  fifo
+  )
+
+(defun pp--print (state)
+  (cl-symbol-macrolet ((stream (pp-state-stream state))
+                       (depth (pp-state-print-depth state))
+                       (scan-depth (pp-state-scan-depth state))
+                       (fifo (pp-state-fifo state))
+                       (left-margin (pp-state-left-margin state))
+                       (width (pp-state-print-width state))
+                       (indent (pp-state-indent state))
+                       (right-margin (pp-state-right-margin state))
+                       (block-mode (pp-state-block-mode state)))
+    (catch 'rescan
+      (while (not (ring-empty-p fifo))
+        (pcase (ring-remove fifo)
+          ((and `(,len . :open-block) token)
+           (if (<= len 0)
+               ;; Not ready to print this yet!
+               (progn (ring-insert-at-beginning fifo token)
+                      (throw 'rescan nil))
+             (cl-incf depth)
+             (push left-margin indent)
+             (push (if (> (+ left-margin len) right-margin)
+                       :vertical :horizontal)
+                   block-mode)))
+          (:close-block (cl-decf depth) (pop indent) (pop block-mode))
+          (:blank
+           (pcase (car block-mode)
+            (:vertical
+             (terpri stream)
+             (princ (make-string (car indent) ?\s) stream)
+             (setf left-margin (car indent)))
+            ((or :horizontal 'nil)
+             (write-char ?\s stream)
+             (cl-incf left-margin))
+            (_ (error "oops")))
+           (cl-incf width))
+          (:eof nil)
+          ((and (pred characterp) char)
+           (write-char char stream)
+           (cl-incf left-margin (char-width char))
+           (cl-incf width (char-width char)))
+          (string
+           (princ string stream)
+           (cl-incf left-margin (string-width string))
+           (cl-incf width (string-width string))))))))
+
+(defun pp--scan (token state)
+  (cl-symbol-macrolet ((stream (pp-state-stream state))
+                       (depth (pp-state-scan-depth state))
+                       (print-depth (pp-state-print-depth state))
+                       (fifo (pp-state-fifo state))
+                       (width (pp-state-scan-width state))
+                       (right-margin (pp-state-right-margin state))
+                       (block-widths (pp-state-block-widths state)))
+    (cl-flet ((scanlen (len) (cl-incf width len)))
+      (cl-assert (> (ring-size fifo) (ring-length fifo)))
+      (ring-insert fifo token)
+      (pcase token
+        (:open-block
+         (cl-incf depth)
+         (let ((block-token (cons (- width) (ring-remove fifo 0))))
+           (push block-token block-widths)
+           (ring-insert fifo block-token)))
+        (:close-block
+         (cl-incf (caar block-widths) width)
+         (when (> (caar block-widths) right-margin)
+           (pp--print state))
+         (cl-decf depth)
+         (pop block-widths))
+        (:blank (scanlen 1))
+        (:eof (pp--print state))
+        ((pred characterp) (scanlen (char-width token)))
+        (_ (scanlen (string-width token)))))
+    (when block-widths
+      (when (> (+ (caar block-widths) width) right-margin)
+        (dolist (block-width block-widths)
+          (setf (car block-width) (+ right-margin 1))))
+      (when (> (caar block-widths) right-margin)
+        (pp--print state)))))
+
+(defvar cl-print-readably) ; cl-print.el
+
+(defun pp-prin1 (object &optional stream right-margin)
+  (unless right-margin
+    (setq right-margin fill-column))
+  (with-temp-buffer
+    (let ((cl-print-readably nil)
+          (state (make-pp-state (or stream standard-output) (current-buffer)
+                                right-margin)))
+      (pp--scan :open-block state)
+      (prog1 (pp-print-object object state)
+        (pp--scan :close-block state)
+        (pp--scan :eof state)))))
+
+
+(cl-defgeneric pp-print-object (object state)
+  ;; Fallback to standard `cl-print-object'.
+  (pp--scan (with-current-buffer (pp-state-tempbuffer state)
+              (cl-prin1 object (current-buffer))
+              (prog1 (buffer-string)
+                (erase-buffer)))
+            state)
+  object)
+
+(cl-defmethod pp-print-object ((list cons) state)
+  (pcase list
+    (`(,head . ,tail)
+     (pp--scan "(" state)
+     (pp--scan :open-block state)
+     (pp-print-object head state)
+     (while (consp tail)
+       (pp--scan :blank state)
+       (pp-print-object (pop tail) state))
+     (when tail
+       (pp--scan :blank state)
+       (pp--scan ?\. state)
+       (pp--scan :blank state)
+       (pp-print-object tail state))
+     (pp--scan :close-block state)
+     (pp--scan ")" state)))
+  list)
+
 ;;;###autoload
 (defun pp-eval-expression (expression)
   "Evaluate EXPRESSION and pretty-print its value.
-- 
2.11.1

bug#25122: 24.5; function describe-variable hangs on large variables

[Stefan Monnier]

> Threads aren't truly parallel.  The user can't do anything while the
> thread is running.

I think the idea is to build the printout while the user is reading the
docstring (for example).  The loop that prints would have regular
`yield` calls so that as soon as not to block other operations.

> I wonder if we could just hook this into scrolling somehow?

We definitely could (at least, there's no fundamental reason why we
can't insert the text from a jit-lock thingy, although modifying the
buffer text from jit-lock might trigger some latent bugs).

> As mentioned in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=21717#8,
> it breaks circularity.  Try describing this variable:

>     (defvar circular-list
>       (let ((l (number-sequence 1 100)))
>         (setcdr (last l) l)
>         l)
>       "A circular list that has problems with (mapc 'pp val).")

That's why I think the (mapc #'pp) trick is an interesting observation
(it points to a performance bug that should be easy to fix) but is not
a solution in itself.


        Stefan

bug#25122: 24.5; function describe-variable hangs on large variables

[npostavs]

> We could probably achieve something similar without breaking circular
> printing by not calling indent-sexp on the full list

The following patch is almost as fast as the mapc 'pp trick, but doesn't
get stuck on circular lists.  The indentation is a bit off: sublists
after the first one incorrectly start in column 0.  Also, this doesn't
really solve the performance problem, it just makes it much less likely
to occur, e.g., (pp (list load-history)) is still slow.

---   i/lisp/emacs-lisp/pp.el
+++   w/lisp/emacs-lisp/pp.el
@@ -76,9 +76,15 @@ pp-buffer
        (progn (skip-chars-forward " \t\n") (point)))
       (insert ?\n))
      (t (goto-char (point-max)))))
   (goto-char (point-min))
-  (indent-sexp))
+  (condition-case () (down-list)
+    (scan-error nil))
+  (while (and (not (eobp))
+              (condition-case () (progn (indent-sexp)
+                                        (forward-sexp)
+                                        t)
+                (scan-error nil)))))
 
 ;;;###autoload
 (defun pp (object &optional stream)
   "Output the pretty-printed representation of OBJECT, any Lisp object.

bug#25122: 24.5; function describe-variable hangs on large variables

[npostavs]

[-- Attachment #1: Type: text/plain, Size: 258 bytes --]

tags 25122 patch
quit

npostavs@users.sourceforge.net writes:

> Also, this doesn't really solve the performance problem, it just makes
> it much less likely to occur, e.g., (pp (list load-history)) is still
> slow.

Okay, I think I found the real fix now:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 3568 bytes --]

From 5188d6e366426d83934505296b585744f50e24a5 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 12 Mar 2017 23:59:19 -0400
Subject: [PATCH] Don't reparse the sexp in indent-sexp (Bug#25122)

* lisp/emacs-lisp/lisp-mode.el (calculate-lisp-indent): Let
PARSE-START be a parse state that can be reused.
(indent-sexp): Pass the running parse state to calculate-lisp-indent
instead of the sexp beginning position.
---
 lisp/emacs-lisp/lisp-mode.el | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index eb07c18b03..8d4abc24e8 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -781,6 +781,10 @@ calculate-lisp-indent
 If the value is nil, that means don't change the indentation
 because the line starts inside a string.
 
+PARSE-START may be a buffer position to start parsing from, or a
+parse state as returned by calling `parse-partial-sexp' up to the
+beginning of the current line.
+
 The value can also be a list of the form (COLUMN CONTAINING-SEXP-START).
 This means that following lines at the same level of indentation
 should not necessarily be indented the same as this line.
@@ -794,12 +798,14 @@ calculate-lisp-indent
           (desired-indent nil)
           (retry t)
           calculate-lisp-indent-last-sexp containing-sexp)
-      (if parse-start
-          (goto-char parse-start)
-          (beginning-of-defun))
-      ;; Find outermost containing sexp
-      (while (< (point) indent-point)
-        (setq state (parse-partial-sexp (point) indent-point 0)))
+      (cond ((or (markerp parse-start) (integerp parse-start))
+             (goto-char parse-start))
+            ((null parse-start) (beginning-of-defun))
+            (t (setq state parse-start)))
+      (unless state
+        ;; Find outermost containing sexp
+        (while (< (point) indent-point)
+          (setq state (parse-partial-sexp (point) indent-point 0))))
       ;; Find innermost containing sexp
       (while (and retry
 		  state
@@ -1070,11 +1076,6 @@ indent-sexp
 ENDPOS is encountered."
   (interactive)
   (let* ((indent-stack (list nil))
-         ;; If ENDPOS is non-nil, use beginning of defun as STARTING-POINT.
-         ;; If ENDPOS is nil, it is safe not to scan before point
-         ;; since every line we indent is more deeply nested than point is.
-         (starting-point (save-excursion (if endpos (beginning-of-defun))
-                                         (point)))
          ;; Use `syntax-ppss' to get initial state so we don't get
          ;; confused by starting inside a string.  We don't use
          ;; `syntax-ppss' in the loop, because this is measurably
@@ -1132,8 +1133,12 @@ indent-sexp
           (unless (or (eolp) (eq (char-syntax (char-after)) ?<))
             (let ((this-indent (car indent-stack)))
               (when (listp this-indent)
-                (let ((val (calculate-lisp-indent
-                            (or (car this-indent) starting-point))))
+                ;; The state here is actually to the end of the
+                ;; previous line, but that's fine for our purposes.
+                ;; And continuing the parse to the next line would
+                ;; destroy element 2 (last sexp position) which
+                ;; `calculate-lisp-indent' needs.
+                (let ((val (calculate-lisp-indent state)))
                   (setq
                    this-indent
                    (cond ((integerp val)
-- 
2.11.1

bug#25122: 24.5; function describe-variable hangs on large variables

[npostavs]

[-- Attachment #1: Type: text/plain, Size: 307 bytes --]

npostavs@users.sourceforge.net writes:

> npostavs@users.sourceforge.net writes:
>
>> Also, this doesn't really solve the performance problem, it just makes
>> it much less likely to occur, e.g., (pp (list load-history)) is still
>> slow.
>
> Okay, I think I found the real fix now:

Missed a corner case.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 7743 bytes --]

From b8dd372f65ce8979324c00b12a9ae767c1ffabda Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 12 Mar 2017 23:59:19 -0400
Subject: [PATCH v2] Don't reparse the sexp in indent-sexp (Bug#25122)

* lisp/emacs-lisp/lisp-mode.el (calculate-lisp-indent): Let
PARSE-START be a parse state that can be reused.
(indent-sexp): Pass the running parse state to calculate-lisp-indent
instead of the sexp beginning position.  Saving the
CONTAINING-SEXP-START returned by `calculate-lisp-indent' is no longer
needed.
* test/lisp/emacs-lisp/lisp-mode-tests.el (indent-sexp): Add blank
line to test case.
---
 lisp/emacs-lisp/lisp-mode.el            | 71 ++++++++++++++++++---------------
 test/lisp/emacs-lisp/lisp-mode-tests.el |  5 ++-
 2 files changed, 42 insertions(+), 34 deletions(-)

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index eb07c18b03..3fefb69066 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -781,6 +781,10 @@ calculate-lisp-indent
 If the value is nil, that means don't change the indentation
 because the line starts inside a string.
 
+PARSE-START may be a buffer position to start parsing from, or a
+parse state as returned by calling `parse-partial-sexp' up to the
+beginning of the current line.
+
 The value can also be a list of the form (COLUMN CONTAINING-SEXP-START).
 This means that following lines at the same level of indentation
 should not necessarily be indented the same as this line.
@@ -794,12 +798,14 @@ calculate-lisp-indent
           (desired-indent nil)
           (retry t)
           calculate-lisp-indent-last-sexp containing-sexp)
-      (if parse-start
-          (goto-char parse-start)
-          (beginning-of-defun))
-      ;; Find outermost containing sexp
-      (while (< (point) indent-point)
-        (setq state (parse-partial-sexp (point) indent-point 0)))
+      (cond ((or (markerp parse-start) (integerp parse-start))
+             (goto-char parse-start))
+            ((null parse-start) (beginning-of-defun))
+            (t (setq state parse-start)))
+      (unless state
+        ;; Find outermost containing sexp
+        (while (< (point) indent-point)
+          (setq state (parse-partial-sexp (point) indent-point 0))))
       ;; Find innermost containing sexp
       (while (and retry
 		  state
@@ -1070,11 +1076,6 @@ indent-sexp
 ENDPOS is encountered."
   (interactive)
   (let* ((indent-stack (list nil))
-         ;; If ENDPOS is non-nil, use beginning of defun as STARTING-POINT.
-         ;; If ENDPOS is nil, it is safe not to scan before point
-         ;; since every line we indent is more deeply nested than point is.
-         (starting-point (save-excursion (if endpos (beginning-of-defun))
-                                         (point)))
          ;; Use `syntax-ppss' to get initial state so we don't get
          ;; confused by starting inside a string.  We don't use
          ;; `syntax-ppss' in the loop, because this is measurably
@@ -1093,16 +1094,21 @@ indent-sexp
     (save-excursion
       (while (< (point) endpos)
         ;; Parse this line so we can learn the state to indent the
-        ;; next line.
-        (while (progn
-                 (setq state (parse-partial-sexp
-                              last-syntax-point (progn (end-of-line) (point))
-                              nil nil state))
-                 ;; Skip over newlines within strings.
-                 (nth 3 state))
-          (setq state (parse-partial-sexp (point) (point-max)
-                                          nil nil state 'syntax-table))
-          (setq last-syntax-point (point)))
+        ;; next line.  Preserve element 2 of the state (last sexp) for
+        ;; `calculate-lisp-indent'.
+        (let ((last-sexp (nth 2 state)))
+          (while (progn
+                   (setq state (parse-partial-sexp
+                                last-syntax-point (progn (end-of-line) (point))
+                                nil nil state))
+                   (setq last-sexp (or (nth 2 state) last-sexp))
+                   ;; Skip over newlines within strings.
+                   (nth 3 state))
+            (setq state (parse-partial-sexp (point) (point-max)
+                                            nil nil state 'syntax-table))
+            (setq last-sexp (or (nth 2 state) last-sexp))
+            (setq last-syntax-point (point)))
+          (setf (nth 2 state) last-sexp))
         (setq next-depth (car state))
         ;; If the line contains a comment indent it now with
         ;; `indent-for-comment'.
@@ -1115,6 +1121,8 @@ indent-sexp
                                     (make-list (- init-depth next-depth) nil))
                 last-depth (- last-depth next-depth)
                 next-depth init-depth))
+        ;; Now indent the next line according to what we learned from
+        ;; parsing the previous one.
         (forward-line 1)
         (when (< (point) endpos)
           (let ((depth-delta (- next-depth last-depth)))
@@ -1124,28 +1132,25 @@ indent-sexp
                    (setq indent-stack (nconc (make-list depth-delta nil)
                                              indent-stack))))
             (setq last-depth next-depth))
-          ;; Now indent the next line according
-          ;; to what we learned from parsing the previous one.
-          (skip-chars-forward " \t")
           ;; But not if the line is blank, or just a comment (we
           ;; already called `indent-for-comment' above).
+          (skip-chars-forward " \t")
           (unless (or (eolp) (eq (char-syntax (char-after)) ?<))
-            (let ((this-indent (car indent-stack)))
-              (when (listp this-indent)
-                (let ((val (calculate-lisp-indent
-                            (or (car this-indent) starting-point))))
-                  (setq
-                   this-indent
+            (indent-line-to
+             (or (car indent-stack)
+                 ;; The state here is actually to the end of the
+                 ;; previous line, but that's fine for our purposes.
+                 ;; And parsing over the newline would only destroy
+                 ;; element 2 (last sexp position).
+                 (let ((val (calculate-lisp-indent state)))
                    (cond ((integerp val)
                           (setf (car indent-stack) val))
                          ((consp val) ; (COLUMN CONTAINING-SEXP-START)
-                          (setf (car indent-stack) (cdr val))
                           (car val))
                          ;; `calculate-lisp-indent' only returns nil
                          ;; when we're in a string, but this won't
                          ;; happen because we skip strings above.
-                         (t (error "This shouldn't happen!"))))))
-              (indent-line-to this-indent))))))))
+                         (t (error "This shouldn't happen!"))))))))))))
 
 (defun indent-pp-sexp (&optional arg)
   "Indent each line of the list starting just after point, or prettyprint it.
diff --git a/test/lisp/emacs-lisp/lisp-mode-tests.el b/test/lisp/emacs-lisp/lisp-mode-tests.el
index 2801f23df6..848dd7ca3e 100644
--- a/test/lisp/emacs-lisp/lisp-mode-tests.el
+++ b/test/lisp/emacs-lisp/lisp-mode-tests.el
@@ -31,6 +31,9 @@
          1
        2)
    2)
+ (fun arg1
+
+      arg2)
  (1
   \"string
 noindent\" (\"string2
@@ -58,7 +61,7 @@
         (save-excursion
           (let ((n 0))
             (while (not (eobp))
-              (unless (looking-at "noindent")
+              (unless (looking-at "noindent\\|^[[:blank:]]*$")
                 (insert (make-string n ?\s)))
               (cl-incf n)
               (forward-line))))
-- 
2.11.1

bug#25122: 24.5; function describe-variable hangs on large variables

[npostavs]

[-- Attachment #1: Type: text/plain, Size: 304 bytes --]

>> npostavs@users.sourceforge.net writes:
>>
>> Okay, I think I found the real fix now:

Same issue with indent-region.  The gains are not so dramatic for
typical lisp code that has normal size sexps, but C-x h C-M-\ on
subr.el's text runs twice as fast for me with the new lisp-indent-region
function.


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1155 bytes --]

From 1a5464d36582b5efcea43aac37bf0b9ddd4c1ff1 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 15 Mar 2017 21:59:13 -0400
Subject: [PATCH v2 2/4] Remove ignored argument from lisp-indent-line

* lisp/emacs-lisp/lisp-mode.el (lisp-indent-line): Remove WHOLE-EXP
argument, the behvior has long since been handled in
`indent-for-tab-command'.
---
 lisp/emacs-lisp/lisp-mode.el | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index 3fefb69066..7eb8b986be 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -744,11 +744,9 @@ lisp-indent-function
   :type 'function
   :group 'lisp)
 
-(defun lisp-indent-line (&optional _whole-exp)
-  "Indent current line as Lisp code.
-With argument, indent any additional lines of the same expression
-rigidly along with this one."
-  (interactive "P")
+(defun lisp-indent-line ()
+  "Indent current line as Lisp code."
+  (interactive)
   (let ((indent (calculate-lisp-indent)) shift-amt
 	(pos (- (point-max) (point)))
 	(beg (progn (beginning-of-line) (point))))
-- 
2.11.1


[-- Attachment #3: patch --]
[-- Type: text/plain, Size: 3088 bytes --]

From 9cf8f32910c0c899f108c3907d099a18fff2cec4 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 15 Mar 2017 22:27:27 -0400
Subject: [PATCH v2 3/4] Add new `lisp-indent-region' that doesn't reparse the
 code.

* lisp/emacs-lisp/lisp-mode.el (lisp-indent-region): New function,
like `indent-region-line-by-line' but additionally keep a running
parse state to avoid reparsing the code repeatedly.
(lisp-indent-line): Take optional PARSE-STATE argument, pass it to
`calculate-lisp-indent'.
(lisp-mode-variables): Set `indent-region-function' to
`lisp-indent-region'.
---
 lisp/emacs-lisp/lisp-mode.el | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index 7eb8b986be..7577267903 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -586,6 +586,7 @@ lisp-mode-variables
   ;;  I believe that newcomment's auto-fill code properly deals with it  -stef
   ;;(set (make-local-variable 'adaptive-fill-mode) nil)
   (setq-local indent-line-function 'lisp-indent-line)
+  (setq-local indent-region-function 'lisp-indent-region)
   (setq-local outline-regexp ";;;\\(;* [^ \t\n]\\|###autoload\\)\\|(")
   (setq-local outline-level 'lisp-outline-level)
   (setq-local add-log-current-defun-function #'lisp-current-defun-name)
@@ -744,10 +745,38 @@ lisp-indent-function
   :type 'function
   :group 'lisp)
 
-(defun lisp-indent-line ()
+(defun lisp-indent-region (start end)
+  "Indent region as Lisp code, efficiently."
+  (save-excursion
+    (setq end (copy-marker end))
+    (goto-char start)
+    ;; The default `indent-region-line-by-line' doesn't hold a running
+    ;; parse state, which forces each indent call to reparse from the
+    ;; beginning.  That has O(n^2) complexity.
+    (let* ((parse-state (syntax-ppss start))
+           (last-syntax-point start)
+           (pr (unless (minibufferp)
+                 (make-progress-reporter "Indenting region..." (point) end))))
+      (while (< (point) end)
+        (unless (and (bolp) (eolp))
+          (lisp-indent-line parse-state))
+        (forward-line 1)
+        (let ((last-sexp (nth 2 parse-state)))
+          (setq parse-state (parse-partial-sexp last-syntax-point (point)
+                                                nil nil parse-state))
+          ;; It's important to preserve last sexp location for
+          ;; `calculate-lisp-indent'.
+          (unless (nth 2 parse-state)
+            (setf (nth 2 parse-state) last-sexp))
+          (setq last-syntax-point (point)))
+        (and pr (progress-reporter-update pr (point))))
+      (and pr (progress-reporter-done pr))
+      (move-marker end nil))))
+
+(defun lisp-indent-line (&optional parse-state)
   "Indent current line as Lisp code."
   (interactive)
-  (let ((indent (calculate-lisp-indent)) shift-amt
+  (let ((indent (calculate-lisp-indent parse-state)) shift-amt
 	(pos (- (point-max) (point)))
 	(beg (progn (beginning-of-line) (point))))
     (skip-chars-forward " \t")
-- 
2.11.1


[-- Attachment #4: patch --]
[-- Type: text/plain, Size: 1969 bytes --]

From 4dbe8fd84064b1b3fed4188f0a2a84de5550cf11 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 15 Mar 2017 22:35:47 -0400
Subject: [PATCH v2 4/4] * lisp/emacs-lisp/lisp-mode.el (indent-sexp): Clean up
 marker.

---
 lisp/emacs-lisp/lisp-mode.el | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index 7577267903..4fa9eb97de 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -1112,12 +1112,13 @@ indent-sexp
          (next-depth init-depth)
          (last-depth init-depth)
          (last-syntax-point (point)))
-    (unless endpos
-      ;; Get error now if we don't have a complete sexp after point.
-      (save-excursion (forward-sexp 1)
-                      ;; We need a marker because we modify the buffer
-                      ;; text preceding endpos.
-                      (setq endpos (point-marker))))
+    ;; We need a marker because we modify the buffer
+    ;; text preceding endpos.
+    (setq endpos (copy-marker
+                  (if endpos endpos
+                    ;; Get error now if we don't have a complete sexp
+                    ;; after point.
+                    (save-excursion (forward-sexp 1) (point)))))
     (save-excursion
       (while (< (point) endpos)
         ;; Parse this line so we can learn the state to indent the
@@ -1177,7 +1178,8 @@ indent-sexp
                          ;; `calculate-lisp-indent' only returns nil
                          ;; when we're in a string, but this won't
                          ;; happen because we skip strings above.
-                         (t (error "This shouldn't happen!"))))))))))))
+                         (t (error "This shouldn't happen!"))))))))))
+    (move-marker endpos nil)))
 
 (defun indent-pp-sexp (&optional arg)
   "Indent each line of the list starting just after point, or prettyprint it.
-- 
2.11.1

bug#25122: 24.5; function describe-variable hangs on large variables

[npostavs]

[-- Attachment #1: Type: text/plain, Size: 974 bytes --]

npostavs@users.sourceforge.net writes:

>>> npostavs@users.sourceforge.net writes:
>>>
>>> Okay, I think I found the real fix now:
>
> Same issue with indent-region.  The gains are not so dramatic for
> typical lisp code that has normal size sexps, but C-x h C-M-\ on
> subr.el's text runs twice as fast for me with the new lisp-indent-region
> function.

Had some test failures due to some corner cases, should all be fixed
now.  I think this is the final patch set.  This also fixes
lisp-indent-region and lisp-indent-line to not indent string literal
contents.

I intend to close this bug as fixed after merging these, as this does
fix the performance bug.  I will probably pursue the alternate pretty
printer separately; it doesn't help performance (after the indent-sexp
performance is fixed), but gives prettier results in some cases.  The
threading idea may also be worth looking at, but can also be considered
separately, as this bug report is already long enough.


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 8386 bytes --]

From 9ae3809189d08c15d64363c963eeeae62efae242 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 12 Mar 2017 23:59:19 -0400
Subject: [PATCH v3 1/4] Don't reparse the sexp in indent-sexp (Bug#25122)

* lisp/emacs-lisp/lisp-mode.el (calculate-lisp-indent): Let
PARSE-START be a parse state that can be reused.
(indent-sexp): Pass the running parse state to calculate-lisp-indent
instead of the sexp beginning position.  Saving the
CONTAINING-SEXP-START returned by `calculate-lisp-indent' is no longer
needed.  Don't bother stopping if we don't descend below init-depth,
since we now alway scan the whole buffer (via syntax-ppss) anyway.
* test/lisp/emacs-lisp/lisp-mode-tests.el (indent-sexp): Add blank
line to test case.
---
 lisp/emacs-lisp/lisp-mode.el            | 76 +++++++++++++++++----------------
 test/lisp/emacs-lisp/lisp-mode-tests.el |  5 ++-
 2 files changed, 43 insertions(+), 38 deletions(-)

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index 2e6e13f1dd..607a4c3d11 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -785,6 +785,10 @@ calculate-lisp-indent
 If the value is nil, that means don't change the indentation
 because the line starts inside a string.
 
+PARSE-START may be a buffer position to start parsing from, or a
+parse state as returned by calling `parse-partial-sexp' up to the
+beginning of the current line.
+
 The value can also be a list of the form (COLUMN CONTAINING-SEXP-START).
 This means that following lines at the same level of indentation
 should not necessarily be indented the same as this line.
@@ -798,12 +802,14 @@ calculate-lisp-indent
           (desired-indent nil)
           (retry t)
           calculate-lisp-indent-last-sexp containing-sexp)
-      (if parse-start
-          (goto-char parse-start)
-          (beginning-of-defun))
-      ;; Find outermost containing sexp
-      (while (< (point) indent-point)
-        (setq state (parse-partial-sexp (point) indent-point 0)))
+      (cond ((or (markerp parse-start) (integerp parse-start))
+             (goto-char parse-start))
+            ((null parse-start) (beginning-of-defun))
+            (t (setq state parse-start)))
+      (unless state
+        ;; Find outermost containing sexp
+        (while (< (point) indent-point)
+          (setq state (parse-partial-sexp (point) indent-point 0))))
       ;; Find innermost containing sexp
       (while (and retry
 		  state
@@ -1074,11 +1080,6 @@ indent-sexp
 ENDPOS is encountered."
   (interactive)
   (let* ((indent-stack (list nil))
-         ;; If ENDPOS is non-nil, use beginning of defun as STARTING-POINT.
-         ;; If ENDPOS is nil, it is safe not to scan before point
-         ;; since every line we indent is more deeply nested than point is.
-         (starting-point (save-excursion (if endpos (beginning-of-defun))
-                                         (point)))
          ;; Use `syntax-ppss' to get initial state so we don't get
          ;; confused by starting inside a string.  We don't use
          ;; `syntax-ppss' in the loop, because this is measurably
@@ -1087,8 +1088,7 @@ indent-sexp
          (init-depth (car state))
          (next-depth init-depth)
          (last-depth init-depth)
-         (last-syntax-point (point))
-         (real-endpos endpos))
+         (last-syntax-point (point)))
     (unless endpos
       ;; Get error now if we don't have a complete sexp after point.
       (save-excursion (forward-sexp 1)
@@ -1098,16 +1098,21 @@ indent-sexp
     (save-excursion
       (while (< (point) endpos)
         ;; Parse this line so we can learn the state to indent the
-        ;; next line.
-        (while (progn
-                 (setq state (parse-partial-sexp
-                              last-syntax-point (progn (end-of-line) (point))
-                              nil nil state))
-                 ;; Skip over newlines within strings.
-                 (nth 3 state))
-          (setq state (parse-partial-sexp (point) (point-max)
-                                          nil nil state 'syntax-table))
-          (setq last-syntax-point (point)))
+        ;; next line.  Preserve element 2 of the state (last sexp) for
+        ;; `calculate-lisp-indent'.
+        (let ((last-sexp (nth 2 state)))
+          (while (progn
+                   (setq state (parse-partial-sexp
+                                last-syntax-point (progn (end-of-line) (point))
+                                nil nil state))
+                   (setq last-sexp (or (nth 2 state) last-sexp))
+                   ;; Skip over newlines within strings.
+                   (nth 3 state))
+            (setq state (parse-partial-sexp (point) (point-max)
+                                            nil nil state 'syntax-table))
+            (setq last-sexp (or (nth 2 state) last-sexp))
+            (setq last-syntax-point (point)))
+          (setf (nth 2 state) last-sexp))
         (setq next-depth (car state))
         ;; If the line contains a comment indent it now with
         ;; `indent-for-comment'.
@@ -1120,9 +1125,9 @@ indent-sexp
                                     (make-list (- init-depth next-depth) nil))
                 last-depth (- last-depth next-depth)
                 next-depth init-depth))
+        ;; Now indent the next line according to what we learned from
+        ;; parsing the previous one.
         (forward-line 1)
-        (when (and (not real-endpos) (<= next-depth init-depth))
-          (goto-char endpos))
         (when (< (point) endpos)
           (let ((depth-delta (- next-depth last-depth)))
             (cond ((< depth-delta 0)
@@ -1131,28 +1136,25 @@ indent-sexp
                    (setq indent-stack (nconc (make-list depth-delta nil)
                                              indent-stack))))
             (setq last-depth next-depth))
-          ;; Now indent the next line according
-          ;; to what we learned from parsing the previous one.
-          (skip-chars-forward " \t")
           ;; But not if the line is blank, or just a comment (we
           ;; already called `indent-for-comment' above).
+          (skip-chars-forward " \t")
           (unless (or (eolp) (eq (char-syntax (char-after)) ?<))
-            (let ((this-indent (car indent-stack)))
-              (when (listp this-indent)
-                (let ((val (calculate-lisp-indent
-                            (or (car this-indent) starting-point))))
-                  (setq
-                   this-indent
+            (indent-line-to
+             (or (car indent-stack)
+                 ;; The state here is actually to the end of the
+                 ;; previous line, but that's fine for our purposes.
+                 ;; And parsing over the newline would only destroy
+                 ;; element 2 (last sexp position).
+                 (let ((val (calculate-lisp-indent state)))
                    (cond ((integerp val)
                           (setf (car indent-stack) val))
                          ((consp val) ; (COLUMN CONTAINING-SEXP-START)
-                          (setf (car indent-stack) (cdr val))
                           (car val))
                          ;; `calculate-lisp-indent' only returns nil
                          ;; when we're in a string, but this won't
                          ;; happen because we skip strings above.
-                         (t (error "This shouldn't happen!"))))))
-              (indent-line-to this-indent))))))))
+                         (t (error "This shouldn't happen!"))))))))))))
 
 (defun indent-pp-sexp (&optional arg)
   "Indent each line of the list starting just after point, or prettyprint it.
diff --git a/test/lisp/emacs-lisp/lisp-mode-tests.el b/test/lisp/emacs-lisp/lisp-mode-tests.el
index 8e3f2e185c..27f0bb5ec1 100644
--- a/test/lisp/emacs-lisp/lisp-mode-tests.el
+++ b/test/lisp/emacs-lisp/lisp-mode-tests.el
@@ -31,6 +31,9 @@
          1
        2)
    2)
+ (fun arg1
+
+      arg2)
  (1
   \"string
 noindent\" (\"string2
@@ -58,7 +61,7 @@
         (save-excursion
           (let ((n 0))
             (while (not (eobp))
-              (unless (looking-at "noindent")
+              (unless (looking-at "noindent\\|^[[:blank:]]*$")
                 (insert (make-string n ?\s)))
               (cl-incf n)
               (forward-line))))
-- 
2.11.1


[-- Attachment #3: patch --]
[-- Type: text/plain, Size: 1969 bytes --]

From b14e7d088ef23074841cb1384503dd60c1e939e9 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 15 Mar 2017 22:35:47 -0400
Subject: [PATCH v3 2/4] * lisp/emacs-lisp/lisp-mode.el (indent-sexp): Clean up
 marker.

---
 lisp/emacs-lisp/lisp-mode.el | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index 607a4c3d11..810fc95614 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -1089,12 +1089,13 @@ indent-sexp
          (next-depth init-depth)
          (last-depth init-depth)
          (last-syntax-point (point)))
-    (unless endpos
-      ;; Get error now if we don't have a complete sexp after point.
-      (save-excursion (forward-sexp 1)
-                      ;; We need a marker because we modify the buffer
-                      ;; text preceding endpos.
-                      (setq endpos (point-marker))))
+    ;; We need a marker because we modify the buffer
+    ;; text preceding endpos.
+    (setq endpos (copy-marker
+                  (if endpos endpos
+                    ;; Get error now if we don't have a complete sexp
+                    ;; after point.
+                    (save-excursion (forward-sexp 1) (point)))))
     (save-excursion
       (while (< (point) endpos)
         ;; Parse this line so we can learn the state to indent the
@@ -1154,7 +1155,8 @@ indent-sexp
                          ;; `calculate-lisp-indent' only returns nil
                          ;; when we're in a string, but this won't
                          ;; happen because we skip strings above.
-                         (t (error "This shouldn't happen!"))))))))))))
+                         (t (error "This shouldn't happen!"))))))))))
+    (move-marker endpos nil)))
 
 (defun indent-pp-sexp (&optional arg)
   "Indent each line of the list starting just after point, or prettyprint it.
-- 
2.11.1


[-- Attachment #4: patch --]
[-- Type: text/plain, Size: 2016 bytes --]

From 6cbe2b3acbb22e75b373d39d716ab4dc1a0f276f Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 15 Mar 2017 21:59:13 -0400
Subject: [PATCH v3 3/4] Remove ignored argument from lisp-indent-line

* lisp/emacs-lisp/lisp-mode.el (lisp-indent-line): Remove WHOLE-EXP
argument, the behavior has long since been handled in
`indent-for-tab-command'.  Also remove redundant `beg' and `shift-amt'
variables and use `indent-line-to'.
---
 lisp/emacs-lisp/lisp-mode.el | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index 810fc95614..89d5659f30 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -748,14 +748,12 @@ lisp-indent-function
   :type 'function
   :group 'lisp)
 
-(defun lisp-indent-line (&optional _whole-exp)
-  "Indent current line as Lisp code.
-With argument, indent any additional lines of the same expression
-rigidly along with this one."
-  (interactive "P")
-  (let ((indent (calculate-lisp-indent)) shift-amt
-	(pos (- (point-max) (point)))
-	(beg (progn (beginning-of-line) (point))))
+(defun lisp-indent-line ()
+  "Indent current line as Lisp code."
+  (interactive)
+  (let ((indent (calculate-lisp-indent))
+	(pos (- (point-max) (point))))
+    (beginning-of-line)
     (skip-chars-forward " \t")
     (if (or (null indent) (looking-at "\\s<\\s<\\s<"))
 	;; Don't alter indentation of a ;;; comment line
@@ -767,11 +765,7 @@ lisp-indent-line
 	  ;; as comment lines, not as code.
 	  (progn (indent-for-comment) (forward-char -1))
 	(if (listp indent) (setq indent (car indent)))
-	(setq shift-amt (- indent (current-column)))
-	(if (zerop shift-amt)
-	    nil
-	  (delete-region beg (point))
-	  (indent-to indent)))
+        (indent-line-to indent))
       ;; If initial point was within line's indentation,
       ;; position after the indentation.  Else stay at same point in text.
       (if (> (- (point-max) pos) (point))
-- 
2.11.1


[-- Attachment #5: patch --]
[-- Type: text/plain, Size: 4032 bytes --]

From d2f3101d88eb9601a52c06d1171c413fc23e3d8d Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 15 Mar 2017 22:27:27 -0400
Subject: [PATCH v3 4/4] Add new `lisp-indent-region' that doesn't reparse the
 code.

Both `lisp-indent-region' and `lisp-indent-line' now use `syntax-ppss'
to get initial state, so they will no longer indent string literal
contents.

* lisp/emacs-lisp/lisp-mode.el (lisp-ppss): New function, like
`syntax-ppss', but with a more dependable item 2.
(lisp-indent-region): New function, like `indent-region-line-by-line'
but additionally keep a running parse state to avoid reparsing the
code repeatedly.  Use `lisp-ppss' to get initial state.
(lisp-indent-line): Take optional PARSE-STATE argument, pass it to
`calculate-lisp-indent', use `lisp-ppss' if not given.
(lisp-mode-variables): Set `indent-region-function' to
`lisp-indent-region'.
---
 lisp/emacs-lisp/lisp-mode.el | 48 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 44 insertions(+), 4 deletions(-)

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index 89d5659f30..54d916887c 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -594,6 +594,7 @@ lisp-mode-variables
   ;;  I believe that newcomment's auto-fill code properly deals with it  -stef
   ;;(set (make-local-variable 'adaptive-fill-mode) nil)
   (setq-local indent-line-function 'lisp-indent-line)
+  (setq-local indent-region-function 'lisp-indent-region)
   (setq-local outline-regexp ";;;\\(;* [^ \t\n]\\|###autoload\\)\\|(")
   (setq-local outline-level 'lisp-outline-level)
   (setq-local add-log-current-defun-function #'lisp-current-defun-name)
@@ -748,12 +749,51 @@ lisp-indent-function
   :type 'function
   :group 'lisp)
 
-(defun lisp-indent-line ()
+(defun lisp-ppss (&optional pos)
+  "Return Parse-Partial-Sexp State at POS, defaulting to point.
+Like to `syntax-ppss' but includes the character address of the
+last complete sexp in the innermost containing list at position
+2 (counting from 0).  This is important for lisp indentation."
+  (unless pos (setq pos (point)))
+  (let ((pss (syntax-ppss pos)))
+    (if (nth 9 pss)
+        (parse-partial-sexp (car (last (nth 9 pss))) pos)
+      pss)))
+
+(defun lisp-indent-region (start end)
+  "Indent region as Lisp code, efficiently."
+  (save-excursion
+    (setq end (copy-marker end))
+    (goto-char start)
+    ;; The default `indent-region-line-by-line' doesn't hold a running
+    ;; parse state, which forces each indent call to reparse from the
+    ;; beginning.  That has O(n^2) complexity.
+    (let* ((parse-state (lisp-ppss start))
+           (last-syntax-point start)
+           (pr (unless (minibufferp)
+                 (make-progress-reporter "Indenting region..." (point) end))))
+      (while (< (point) end)
+        (unless (and (bolp) (eolp))
+          (lisp-indent-line parse-state))
+        (forward-line 1)
+        (let ((last-sexp (nth 2 parse-state)))
+          (setq parse-state (parse-partial-sexp last-syntax-point (point)
+                                                nil nil parse-state))
+          ;; It's important to preserve last sexp location for
+          ;; `calculate-lisp-indent'.
+          (unless (nth 2 parse-state)
+            (setf (nth 2 parse-state) last-sexp))
+          (setq last-syntax-point (point)))
+        (and pr (progress-reporter-update pr (point))))
+      (and pr (progress-reporter-done pr))
+      (move-marker end nil))))
+
+(defun lisp-indent-line (&optional parse-state)
   "Indent current line as Lisp code."
   (interactive)
-  (let ((indent (calculate-lisp-indent))
-	(pos (- (point-max) (point))))
-    (beginning-of-line)
+  (let ((pos (- (point-max) (point)))
+        (indent (progn (beginning-of-line)
+                       (calculate-lisp-indent (or parse-state (lisp-ppss))))))
     (skip-chars-forward " \t")
     (if (or (null indent) (looking-at "\\s<\\s<\\s<"))
 	;; Don't alter indentation of a ;;; comment line
-- 
2.11.1

bug#25122: 24.5; function describe-variable hangs on large variables

[npostavs]

tags 25122 fixed
close 25122 26.1
quit

npostavs@users.sourceforge.net writes:
>
> I intend to close this bug as fixed after merging these, as this does
> fix the performance bug.

Pushed to master.

[1: 6fa9cc0593]: 2017-04-22 14:18:46 -0400
  ; Merge: improve indent-sexp and lisp-indent-region performance
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=6fa9cc0593150a318f0e08e69ec10672d548a7c1

[2: 4713dd425b]: 2017-04-22 14:09:58 -0400
  Add new `lisp-indent-region' that doesn't reparse the code.
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=4713dd425beac5cb459704e67dcb8f6faf714375

[3: 2f6769f9cd]: 2017-04-22 14:09:58 -0400
  Remove ignored argument from lisp-indent-line
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=2f6769f9cdb799e880fdcc09057353a0a2349bfc

[4: 8bb5d7adaf]: 2017-04-22 14:09:57 -0400
  * lisp/emacs-lisp/lisp-mode.el (indent-sexp): Clean up marker.
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=8bb5d7adaf45264900385530c7f76175ba490a77

[5: 43c84577a3]: 2017-04-22 14:09:57 -0400
  Don't reparse the sexp in indent-sexp (Bug#25122)
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=43c84577a3055d5ddf1f5d1b999e6ecca6139f60

bug#25122: 24.5; function describe-variable hangs on large variables

[Michael Heerdegen]

Hi Noam,

> > I intend to close this bug as fixed after merging these, as this does
> > fix the performance bug.

I have a question: how much performance gain can I expect from your
changes?

My use case is the following: "el-search" has now an occur-like
operation mode.  It copies all matches (i.e. s-expressions) into an *El
Occur* buffer.  Matches can be tiny (like `1') or arbitrarily large.

Since most expressions don't start at column 0 in their source buffer
(unless they are top-level), I have to remove the amount of additional
indentation from all but the first line of a match.  Well - not from
all, that's where it gets complicated - there are cases where this is
wrong, like in strings and some comments.

To have a sane solution, I just used `indent-region' to reindent the
expression in the occur buffer.  This works great, but `indent-region'
was so slow that it took up to 50 percent of the whole running time of
the occur search.

With an naive alternative approach (just remove additional indentation
from lines where it makes sense (whereby finding out whether it "makes
sense" involves running syntax scanning)), I get an improvement of a
factor around five or ten relative to `indent-region' for reindenting.

Would you expect your improved `indent-region' is fast enough now so
that it makes sense that I switch back to it in my code?  Is the running
time now something close to O(number of lines to indent)?


Thanks,

Michael.

bug#25122: 24.5; function describe-variable hangs on large variables

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Would you expect your improved `indent-region' is fast enough now so
> that it makes sense that I switch back to it in my code?  Is the
> running time now something close to O(number of lines to indent)?

After some tests, I think it is fast enough for my scenario.  And I see
from the comments in the code that you addressed the complexity problem.

Michael.