bug#17284: 24.3.90; Host name completion in shell mode take 45 seconds

bug#17284: 24.3.90; Host name completion in shell mode take 45 seconds

[Svend Sorensen]

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

emacs -Q
M-x shell
ssh s<tab>

This hangs for 45 seconds before completions are listed. Emacs is
unresponsive during this time.



In GNU Emacs 24.3.90.1 (x86_64-apple-darwin13.1.0, NS apple-appkit-1265.19)
 of 2014-04-17 on svesorm0.corp.w3data.com
Windowing system distributor `Apple', version 10.3.1265
Configured using:
 `configure --prefix=/usr/local/Cellar/emacs/HEAD --without-dbus
 --enable-locallisppath=/usr/local/share/emacs/site-lisp
 --infodir=/usr/local/Cellar/emacs/HEAD/share/info/emacs
 --without-gnutls --with-ns --disable-ns-self-contained'

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

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
M-x r e p o r t - b u g <return>

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Load-path shadows:
None found.

Features:
(shadow sort gnus-util mail-extr emacsbug message format-spec rfc822 mml
easymenu mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util help-fns mail-prsvr mail-utils time-date tooltip electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel ns-win tool-bar dnd
fontset image regexp-opt fringe tabulated-list newcomment lisp-mode
prog-mode register page menu-bar rfn-eshadow timer select scroll-bar
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
cocoa ns multi-tty emacs)

Memory information:
((conses 16 71390 7340)
 (symbols 48 17149 0)
 (miscs 40 37 118)
 (strings 32 9729 5058)
 (string-bytes 1 258860)
 (vectors 16 8834)
 (vector-slots 8 359444 17254)
 (floats 8 53 168)
 (intervals 56 180 19)
 (buffers 960 11))

[-- Attachment #2: Type: text/html, Size: 3483 bytes --]

bug#17284: Host name completion in shell mode take 45 seconds

[Svend Sorensen]

This delay is happening in the pcmpl-ssh-known-hosts function, during
the second while:

(while (and (looking-back ",")
                      (re-search-forward host-re (line-end-position) t))
            (add-to-list 'ssh-hosts-list (concat (match-string 1)
                                                 (match-string 2))))

If I remove this, I get back a list of hosts immediately.

My known_hosts file has 3600 lines.

bug#17284: Host name completion in shell mode take 45 seconds

[Nicolas Richard]

Svend Sorensen <svend@ciffer.net> writes:
> (while (and (looking-back ",")
>                       (re-search-forward host-re (line-end-position) t))
>             (add-to-list 'ssh-hosts-list (concat (match-string 1)
>                                                  (match-string 2))))
>
> If I remove this, I get back a list of hosts immediately.
>
> My known_hosts file has 3600 lines.

Hello,

Does it help to replace (looking-back ",") by (eq ?, (char-before)) ?

Nicolas.

bug#17284: Host name completion in shell mode take 45 seconds

[Svend Sorensen]

On Thu, Apr 2, 2015 at 10:39 PM, Nicolas Richard
<theonewiththeevillook@yahoo.fr> wrote:
>
> Does it help to replace (looking-back ",") by (eq ?, (char-before)) ?

The function runs quickly with that change.

bug#17284: Host name completion in shell mode take 45 seconds

[Nicolas Richard]

Le 03/04/2015 11:09, Svend Sorensen a écrit :
> On Thu, Apr 2, 2015 at 10:39 PM, Nicolas Richard
> <theonewiththeevillook@yahoo.fr> wrote:
>>
>> Does it help to replace (looking-back ",") by (eq ?, (char-before)) ?
> 
> The function runs quickly with that change.

Thanks.

I'm still not confident enough to push my commits without approval from
actual devs so here's what I suggest:

Commit fd89dbb0e9658874f71a75f0b84ce2f1a3021252
References: yf/bug-17284
Author:     Nicolas Richard <theonewiththeevillook@yahoo.fr>
AuthorDate: Fri Apr 3 12:28:40 2015 +0200
Commit:     Nicolas Richard <theonewiththeevillook@yahoo.fr>
CommitDate: Fri Apr 3 12:33:42 2015 +0200

    pcmpl-ssh-known-hosts: Use `char-before' instead of `looking-back'.
    
    Fixes: debbugs:17284
    
    * lisp/pcmpl-unix.el (pcmpl-ssh-known-hosts): Use `char-before' instead
    of `looking-back'.

1 parent commit, 2 merged branches, 1 containing branch
 Parent     | 0b914ba ; Tweak the previous change
 Merged     | emacs-24 yf/bug-17284
 Containing | yf/bug-17284
 Follows    | emacs-24.5-rc1 (4152)

2 files changed, 6 insertions(+), 1 deletion(-)
 lisp/ChangeLog     | 5 +++++
 lisp/pcmpl-unix.el | 2 +-

modified   lisp/ChangeLog
@@ -1,3 +1,8 @@
+2015-04-03  Nicolas Richard  <theonewiththeevillook@yahoo.fr>
+
+	* pcmpl-unix.el (pcmpl-ssh-known-hosts): Use `char-before' instead
+	of `looking-back' (bug#17284).
+
 2015-04-03  Dmitry Gutov  <dgutov@yandex.ru>
 
 	* progmodes/js.el (js-indent-line): Do nothing when bol is inside
modified   lisp/pcmpl-unix.el
@@ -157,7 +157,7 @@ (defun pcmpl-ssh-known-hosts ()
         (while (re-search-forward (concat "^ *" host-re) nil t)
           (add-to-list 'ssh-hosts-list (concat (match-string 1)
                                                (match-string 2)))
-          (while (and (looking-back ",")
+          (while (and (eq (char-before) ?,)
                       (re-search-forward host-re (line-end-position) t))
             (add-to-list 'ssh-hosts-list (concat (match-string 1)
                                                  (match-string 2)))))

-- 
Nico.

bug#17284: Host name completion in shell mode take 45 seconds

[martin rudalics]

 >      pcmpl-ssh-known-hosts: Use `char-before' instead of `looking-back'.

Couldn't the byte-compiler warn when `looking-back' is used without
specifying LIMIT?  Making LIMIT non-optional seems too harsh.

martin

bug#17284: Host name completion in shell mode take 45 seconds

[Stefan Monnier]

>> pcmpl-ssh-known-hosts: Use `char-before' instead of `looking-back'.
> Couldn't the byte-compiler warn when `looking-back' is used without
> specifying LIMIT?  Making LIMIT non-optional seems too harsh.

diff --git a/lisp/subr.el b/lisp/subr.el
index 163a1c4..16a3440 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3485,6 +3484,7 @@ LIMIT.
 
 As a general recommendation, try to avoid using `looking-back'
 wherever possible, since it is slow."
+  (declare (advertised-calling-convention (regexp limit &optional greedy)))
   (let ((start (point))
 	(pos
 	 (save-excursion


-- Stefan

bug#17284: Host name completion in shell mode take 45 seconds

[martin rudalics]

 > +  (declare (advertised-calling-convention (regexp limit &optional greedy)))

Exactly!  Just that doing this now would probably exasperate
bootstrappers.

martin

bug#17284: Host name completion in shell mode take 45 seconds

[Stefan Monnier]

>> +  (declare (advertised-calling-convention (regexp limit &optional greedy)))
> Exactly!  Just that doing this now would probably exasperate
> bootstrappers.

I don't think so.  It's just going to add a couple more warnings, which
can all be fixed trivially.


        Stefan

bug#17284: Host name completion in shell mode take 45 seconds

[martin rudalics]

 > I don't think so.  It's just going to add a couple more warnings, which
 > can all be fixed trivially.

Well, I asked for it.  Hopefully we're now motivated enough to supply
the (hopefully trivial) fixes.

martin

bug#17284: Host name completion in shell mode take 45 seconds

[Dmitry Gutov]

On 04/03/2015 01:36 PM, Nicolas Richard wrote:

> I'm still not confident enough to push my commits without approval from
> actual devs so here's what I suggest:

Looks good to me.

bug#17284: Host name completion in shell mode take 45 seconds

[Nicolas Richard]

Thanks.

Pushed as 7c691f3 and marking this bug as done.

Nicolas.

bug#17284: Host name completion in shell mode take 45 seconds

[Drew Adams]

> > > Does it help to replace (looking-back ",") by (eq ?, (char-before)) ?
> > 
> > The function runs quickly with that change.
>
> Couldn't the byte-compiler warn when `looking-back' is used without
> specifying LIMIT?  Making LIMIT non-optional seems too harsh.

1. Is it really about LIMIT?  Or is it instead about looking back
   at a literal string of chars?  And typically a short string.
   I'm guessing that that is the use case to pursue here.

2. Instead of (or in addition to) a byte-compiler warning for
   `looking-back', how about adding a function `chars-before'?

Since I use multiple Emacs versions, some quite old, I use this.
But I'm sure that a much better (including prettier) definition
can be had for recent Emacs.  Or (better) define it in C.

(defun chars-before (chars)
  "Return non-nil if the literal string CHARS is right before point."
  (let* ((len  (length chars))
         (idx  (1- len))
         (pt   (point)))
    (catch 'chars-before
      (dolist (char  (append chars ()))
        (unless (condition-case nil
                    (eq char (char-before (- pt idx)))
                  (error nil))
          (throw 'chars-before nil))
        (setq idx  (1- idx)))
      t)))

Likewise (but may be no better than `looking-at' + `regexp-quote'):

(defun chars-after (chars)
  "Return non-nil if the literal string CHARS is right after point."
  (let* ((len  (length chars))
         (idx  (1- len))
         (pt   (point)))
    (catch 'chars-after
      (dolist (char  (nreverse (append chars ())))
        (unless (condition-case nil
                    (eq char (char-after (+ pt idx)))
                  (error nil))          ; e.g. `eobp'
          (throw 'chars-after nil))
        (setq idx  (1- idx)))
      t)))

bug#17284: Host name completion in shell mode take 45 seconds

[martin rudalics]

 > 1. Is it really about LIMIT?  Or is it instead about looking back
 >     at a literal string of chars?  And typically a short string.
 >     I'm guessing that that is the use case to pursue here.

Right.

 > 2. Instead of (or in addition to) a byte-compiler warning for
 >     `looking-back', how about adding a function `chars-before'?

This would be useful.

 > Since I use multiple Emacs versions, some quite old, I use this.
 > But I'm sure that a much better (including prettier) definition
 > can be had for recent Emacs.  Or (better) define it in C.

I guess something like

(defun string-before-p (string)
   "Return t if string before `point' equals STRING."
   (let ((start (- (point) (length string))))
     (and (>= start (point-min))
	 (string-equal
	  (buffer-substring-no-properties start (point))
	  string))))

would do.  And for simpler cases giving `char-before' a second argument
to specify the position of the character before `point' might help too.

But many occurrences of `looking-back' in the code base are inherently
tied to regexps specified via defcustoms or passed as arguments and it
doesn't seem easy to get rid of them.  Hence we should probably add a
TODO item to fix all occurrences of `looking-back' either via the LIMIT
argument, by string or character comparison.  An appetizing formulation
of such an item would be very welcome.

martin

bug#17284: Host name completion in shell mode take 45 seconds

[Drew Adams]

> (defun string-before-p (string)
>    "Return t if string before `point' equals STRING."
>    (let ((start (- (point) (length string))))
>      (and (>= start (point-min))
> 	 (string-equal
> 	  (buffer-substring-no-properties start (point))
> 	  string))))

`buffer-substring-no-properties' is defined in C.  I can't tell
whether this is more efficient because of that or less efficient
because it creates a potentially giant string before starting to
compare.  I was trying to compare starting from the far end,
thinking that that might lead to earlier failure detection.

But I'll take your word for it that this is probably better.

(I guess if we were really worried about the long-string case we
could do what Isearch does, for the comparison.)

I didn't mention this before, because my implementation of
`chars-before' uses `char-before'.  But with such a function,
if it is defined without using `char-before' then the latter
function could be eliminated: (chars-before "a") instead of
(char-before ?a).

> And for simpler cases giving `char-before' a second argument
> to specify the position of the character before `point' might help too.

Yes.  (And that could apply as well to `chars-before', aka
`string-before-p' - optionally specify the start or end position.)

> But many occurrences of `looking-back' in the code base are inherently
> tied to regexps specified via defcustoms or passed as arguments and it
> doesn't seem easy to get rid of them.  Hence we should probably add a
> TODO item to fix all occurrences of `looking-back' either via the LIMIT
> argument, by string or character comparison.  An appetizing formulation
> of such an item would be very welcome.

Yes.  And I think you just formulated it, appetizingly enough. ;-)

bug#17284: Host name completion in shell mode take 45 seconds

[martin rudalics]

 > `buffer-substring-no-properties' is defined in C.  I can't tell
 > whether this is more efficient because of that or less efficient
 > because it creates a potentially giant string before starting to
 > compare.  I was trying to compare starting from the far end,
 > thinking that that might lead to earlier failure detection.

I got your idea.  But you also said that what we are handling here is
"typically a short string" ;-)

martin

bug#17284: Host name completion in shell mode take 45 seconds

[Drew Adams]

>  > `buffer-substring-no-properties' is defined in C.  I can't tell
>  > whether this is more efficient because of that or less efficient
>  > because it creates a potentially giant string before starting to
>  > compare.  I was trying to compare starting from the far end,
>  > thinking that that might lead to earlier failure detection.
> 
> I got your idea.  But you also said that what we are handling here is
> "typically a short string" ;-)

Yes, and I do expect that that's the typical case.  The question
is whether we want to handle more than this typical case.

To be clear, I don't say we that we need to.  But if we are going
to define this, why not?  That's why I suggested that we might
even want to define it C, like `char-before'.

Whatever others decide about that is fine with me.  The suggestion
is to add such a function, however it might be implemented.

bug#17284: Host name completion in shell mode take 45 seconds

[Drew Adams]

Trying again, after trying to unarchive:

> From: Drew Adams Sent: Thursday, December 31, 2015 3:14 PM
> 
> Martin's `string-before-p' Lisp version seems good.
> Did such a function ever get added to Emacs?
> 
> Even if something better gets coded in C someday, it might
> be good to add the Lisp version now.