unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#50599: [PATCH] Don't recommend against "\[...]" substitutions for performance
@ 2021-09-15  6:27 Stefan Kangas
  2021-09-15  6:38 ` Eli Zaretskii
  0 siblings, 1 reply; 10+ messages in thread
From: Stefan Kangas @ 2021-09-15  6:27 UTC (permalink / raw)
  To: 50599

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

Severity: minor

In `(elisp) Documentation Tips', we read:

     It is not practical to use ‘\\[...]’ very many times, because
     display of the documentation string will become slow.  So use this
     to describe the most important commands in your major mode, and
     then use ‘\\{...}’ to display the rest of the mode’s keymap.

When testing this on my machine on a docstring with a large number of
substitutions (107), I get the following (in "emacs -Q"):

(progn (require 'ibuffer)
       (let ((times 100))
         (/ (car (benchmark-run
                     times (documentation 'ibuffer-mode)))
            times)))

    => 0.00499586008

When I increase the number of substitutions in that docstring to around 1000 (by
duplicating the docstring 10 times), I get:

    => 0.05029239337

This is 10 times slower, but still fast enough that it does not matter much.
It also suggests that this is O(N) in time.

My conclusion is that the above recommendation in `(elisp) Documentation Tips'
is irrelevant these days, and I suggest to remove it.

Please see the attached patch.

[-- Attachment #2: 0001-Don-t-recommend-against-using-.-substitutions-many-t.patch --]
[-- Type: text/x-patch, Size: 4473 bytes --]

From d3bca2d60f07876a562378f1b56844770ab87404 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Wed, 15 Sep 2021 08:18:20 +0200
Subject: [PATCH] Don't recommend against using "\[...]" substitutions many
 times

* doc/lispref/tips.texi (Documentation Tips): Don't recommend against
using many "\[...]" substitutions for reasons of performance.  This
recommendation is no longer relevant, as this is more than fast enough
on modern machines.

* lisp/emacs-lisp/checkdoc.el (checkdoc-max-keyref-before-warn):
Add new valid value nil meaning to never warn about too many command
substitutions.  Make nil the new default.
(checkdoc-this-string-valid-engine): Respect above new value.
---
 doc/lispref/tips.texi       |  5 -----
 lisp/emacs-lisp/checkdoc.el | 36 +++++++++++++++++++++---------------
 2 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/doc/lispref/tips.texi b/doc/lispref/tips.texi
index f0eb1079ca..7c3c9b1c2a 100644
--- a/doc/lispref/tips.texi
+++ b/doc/lispref/tips.texi
@@ -789,11 +789,6 @@ Documentation Tips
 @samp{\\<@dots{}>} should be the name of the variable containing the
 local keymap for the major mode.
 
-It is not practical to use @samp{\\[@dots{}]} very many times, because
-display of the documentation string will become slow.  So use this to
-describe the most important commands in your major mode, and then use
-@samp{\\@{@dots{}@}} to display the rest of the mode's keymap.
-
 @item
 For consistency, phrase the verb in the first sentence of a function's
 documentation string as an imperative---for instance, use ``Return the
diff --git a/lisp/emacs-lisp/checkdoc.el b/lisp/emacs-lisp/checkdoc.el
index e10ea736cd..e0797f2b92 100644
--- a/lisp/emacs-lisp/checkdoc.el
+++ b/lisp/emacs-lisp/checkdoc.el
@@ -249,11 +249,17 @@ checkdoc-ispell-lisp-words
   "List of words that are correct when spell-checking Lisp documentation.")
 ;;;###autoload(put 'checkdoc-ispell-list-words 'safe-local-variable #'checkdoc-list-of-strings-p)
 
-(defcustom checkdoc-max-keyref-before-warn 10
-  "The number of \\ [command-to-keystroke] tokens allowed in a doc string.
+(defcustom checkdoc-max-keyref-before-warn nil
+  "If non-nil, number of \\\\=[command-to-keystroke] tokens allowed in a doc string.
 Any more than this and a warning is generated suggesting that the construct
-\\ {keymap} be used instead."
-  :type 'integer)
+\\\\={keymap} be used instead.  If the value is nil, never warn.
+
+It used to not be practical to use `\\\\=[...]' very many times,
+because display of the documentation string would become slow.
+This is typically not an issue on modern machines."
+  :type '(choice (const nil)
+                 integer)
+  :version "28.1")
 
 (defcustom checkdoc-arguments-in-order-flag nil
   "Non-nil means warn if arguments appear out of order.
@@ -1543,17 +1549,17 @@ checkdoc-this-string-valid-engine
 	       " embedded in doc string.  Use \\\\<keymap> & \\\\[function] "
 	       "instead")
 	      (match-beginning 1) (match-end 1) t))))
-     ;; It is not practical to use `\\[...]' very many times, because
-     ;; display of the documentation string will become slow.  So use this
-     ;; to describe the most important commands in your major mode, and
-     ;; then use `\\{...}' to display the rest of the mode's keymap.
-     (save-excursion
-       (if (and (re-search-forward "\\\\\\\\\\[\\w+" e t
-				   (1+ checkdoc-max-keyref-before-warn))
-		(not (re-search-forward "\\\\\\\\{\\w+}" e t)))
-	   (checkdoc-create-error
-	    "Too many occurrences of \\[function].  Use \\{keymap} instead"
-	    s (marker-position e))))
+     ;; It used to not be practical to use `\\[...]' very many times,
+     ;; because display of the documentation string would become slow.
+     ;; This is typically not an issue on modern machines.
+     (when checkdoc-max-keyref-before-warn
+       (save-excursion
+         (if (and (re-search-forward "\\\\\\\\\\[\\w+" e t
+                                     (1+ checkdoc-max-keyref-before-warn))
+                  (not (re-search-forward "\\\\\\\\{\\w+}" e t)))
+             (checkdoc-create-error
+              "Too many occurrences of \\[function].  Use \\{keymap} instead"
+              s (marker-position e)))))
      ;; Ambiguous quoted symbol.  When a symbol is both bound and fbound,
      ;; and is referred to in documentation, it should be prefixed with
      ;; something to disambiguate it.  This check must be before the
-- 
2.30.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#50599: [PATCH] Don't recommend against "\[...]" substitutions for performance
  2021-09-15  6:27 bug#50599: [PATCH] Don't recommend against "\[...]" substitutions for performance Stefan Kangas
@ 2021-09-15  6:38 ` Eli Zaretskii
  2021-09-15  7:11   ` Stefan Kangas
  0 siblings, 1 reply; 10+ messages in thread
From: Eli Zaretskii @ 2021-09-15  6:38 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: 50599

> From: Stefan Kangas <stefan@marxist.se>
> Date: Wed, 15 Sep 2021 08:27:55 +0200
> 
> (progn (require 'ibuffer)
>        (let ((times 100))
>          (/ (car (benchmark-run
>                      times (documentation 'ibuffer-mode)))
>             times)))
> 
>     => 0.00499586008
> 
> When I increase the number of substitutions in that docstring to around 1000 (by
> duplicating the docstring 10 times), I get:
> 
>     => 0.05029239337
> 
> This is 10 times slower, but still fast enough that it does not matter much.
> It also suggests that this is O(N) in time.
> 
> My conclusion is that the above recommendation in `(elisp) Documentation Tips'
> is irrelevant these days, and I suggest to remove it.
> 
> Please see the attached patch.

Thanks for the research, but the removal you propose is too radical.
The text already says "very many times".  You are saying that on your
system (which has what CPU, btw?) "very many" is a very large number,
but even for you 1000 references takes 50 msec, which begins to be
significant.

So I'm okay with somehow modifying the text to provide a better idea
of what "very many" means nowadays, but I think the advice is still
valid and shouldn't be removed.  We cannot guarantee that arbitrarily
large number of such references will not slow down help display.





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#50599: [PATCH] Don't recommend against "\[...]" substitutions for performance
  2021-09-15  6:38 ` Eli Zaretskii
@ 2021-09-15  7:11   ` Stefan Kangas
  2021-09-15  8:24     ` Eli Zaretskii
  0 siblings, 1 reply; 10+ messages in thread
From: Stefan Kangas @ 2021-09-15  7:11 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 50599

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks for the research, but the removal you propose is too radical.
> The text already says "very many times".  You are saying that on your
> system (which has what CPU, btw?) "very many" is a very large number,
> but even for you 1000 references takes 50 msec, which begins to be
> significant.

According to /proc/cpuinfo, machine has a Intel(R) Core(TM) i7-3770
CPU @ 3.40GHz.  I believe this CPU was first released in 2013, and if
I'm not mistaken was not on the high-end even then.

Anything up to 100ms is perceived as instantaneous, so 50ms is still
very fast.  Note in particular that I only came up with the this
result by copying one of our longest docstrings 10 times over.  Even
the longest docstrings we have in Emacs will still be displayed under
10ms, so let's please use that figure as the basis for this
discussion.

> So I'm okay with somehow modifying the text to provide a better idea
> of what "very many" means nowadays, but I think the advice is still
> valid and shouldn't be removed.  We cannot guarantee that arbitrarily
> large number of such references will not slow down help display.

Formally, it is correct that if you throw very large inputs at
`substitute-command-keys', you will start to notice performance
issues.  But when evaluating performance considerations we must also
consider what inputs we will realistically expect to see.

Even in `ibuffer-mode', which already has a very large number of
substitutions (107), this takes only 5ms on my machine.  It is in my
opinion unrealistic to expect that there exist more than a handful
modes out there that has more than an order of magnitude more commands
than this one.  You will start running out of keys, the docstring will
be completely unwieldy, etc., etc.

If there exists any highly extreme outliers out there for which this
might (!) become a problem, it is only because they are doing
something silly like writing major modes with 2000+ commands in them.
If they would come to emacs-devel with this problem I expect that we
would tell them "then don't do that".

But this advice is completely irrelevant for everyone else, and only
wastes valuable space in the reference manual.





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#50599: [PATCH] Don't recommend against "\[...]" substitutions for performance
  2021-09-15  7:11   ` Stefan Kangas
@ 2021-09-15  8:24     ` Eli Zaretskii
  2021-09-15 14:13       ` Stefan Kangas
  0 siblings, 1 reply; 10+ messages in thread
From: Eli Zaretskii @ 2021-09-15  8:24 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: 50599

> From: Stefan Kangas <stefan@marxist.se>
> Date: Wed, 15 Sep 2021 09:11:02 +0200
> Cc: 50599@debbugs.gnu.org
> 
> According to /proc/cpuinfo, machine has a Intel(R) Core(TM) i7-3770
> CPU @ 3.40GHz.  I believe this CPU was first released in 2013, and if
> I'm not mistaken was not on the high-end even then.

Your machine is quite fast, even though it's 8 years old.  (Mine is 9
years old, and is faster.)  There are many machines in use out there
that are much, much slower.

> > So I'm okay with somehow modifying the text to provide a better idea
> > of what "very many" means nowadays, but I think the advice is still
> > valid and shouldn't be removed.  We cannot guarantee that arbitrarily
> > large number of such references will not slow down help display.
> 
> Formally, it is correct that if you throw very large inputs at
> `substitute-command-keys', you will start to notice performance
> issues.  But when evaluating performance considerations we must also
> consider what inputs we will realistically expect to see.

We have no idea what could Lisp programmers out there want to do with
this.  For example, I could envision some programmatically generated
help text with many such references.  Where there are limitations due
to the implementation, we should strive to let people know about them.

> But this advice is completely irrelevant for everyone else, and only
> wastes valuable space in the reference manual.

I disagree with the "completely irrelevant" part, the general advice
to keep the use of \\[..] to the minimum is still valid, so deleting
that text entirely throws away the baby together with the bathwater.
We should adapt the text to modern CPUs without losing the basic idea.





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#50599: [PATCH] Don't recommend against "\[...]" substitutions for performance
  2021-09-15  8:24     ` Eli Zaretskii
@ 2021-09-15 14:13       ` Stefan Kangas
  2021-09-15 15:41         ` Eli Zaretskii
  2021-09-15 15:46         ` bug#50599: [External] : " Drew Adams
  0 siblings, 2 replies; 10+ messages in thread
From: Stefan Kangas @ 2021-09-15 14:13 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 50599

Eli Zaretskii <eliz@gnu.org> writes:

> We have no idea what could Lisp programmers out there want to do with
> this.  For example, I could envision some programmatically generated
> help text with many such references.  Where there are limitations due
> to the implementation, we should strive to let people know about them.

But not every such limitation belongs in `(elisp) Documentation Tips'.

Even if one can imagine that there exists specialized applications
where this limitation will become a problem, that does not mean that
we should mention it here.   This section should IMO be about general
advice for Emacs Lisp programming, and this is not a general problem.

> I disagree with the "completely irrelevant" part, the general advice
> to keep the use of \\[..] to the minimum is still valid,

I see no reason to keep use of "\\[...]" to the minimum.  In any
realistic use, my investigation has demonstrated that there is no
problem with using it for reasons of performance.

Instead of discouraging it, we should encourage its use, as it leads
to better documentation.  For example, compare the current
'ibuffer-mode' docstring to what you get if you replace the list of
commands (with its categories, explanations, etc.) with a blanket
"\\{ibuffer-mode-map}".  So I find the advice not only irrelevant but
misleading.

How about this:

We change, in my patch, 'checkdoc-max-keyref-before-warn' to a value
like 1000 or 500 instead of nil.  This would make me happy by not
impacting any real use-cases [none of which will have 500+ commands in
its docstring, or at least none of the ones that I care about will]
and it would (hopefully) make you happy by sufficiently calling
attention to any possible performance issues in some other cases.

With that, perhaps we could agree that it is okay to delete the
paragraph in `(elisp) Documentation Tips'.  Running 'checkdoc' is
after all recommended in that section as well.  WDYT?





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#50599: [PATCH] Don't recommend against "\[...]" substitutions for performance
  2021-09-15 14:13       ` Stefan Kangas
@ 2021-09-15 15:41         ` Eli Zaretskii
  2021-09-15 18:37           ` Stefan Kangas
  2021-09-15 15:46         ` bug#50599: [External] : " Drew Adams
  1 sibling, 1 reply; 10+ messages in thread
From: Eli Zaretskii @ 2021-09-15 15:41 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: 50599

> From: Stefan Kangas <stefan@marxist.se>
> Date: Wed, 15 Sep 2021 16:13:59 +0200
> Cc: 50599@debbugs.gnu.org
> 
> We change, in my patch, 'checkdoc-max-keyref-before-warn' to a value
> like 1000 or 500 instead of nil.  This would make me happy by not
> impacting any real use-cases [none of which will have 500+ commands in
> its docstring, or at least none of the ones that I care about will]
> and it would (hopefully) make you happy by sufficiently calling
> attention to any possible performance issues in some other cases.

I didn't realize that checkdoc is involved in this.  If the problem is
that it produces annoying diagnostics for \\[..], then I'm okay with
removing it or making it less frequent.

I was only talking about the manual.

> With that, perhaps we could agree that it is okay to delete the
> paragraph in `(elisp) Documentation Tips'.  Running 'checkdoc' is
> after all recommended in that section as well.  WDYT?

My reluctance to delete that advice is unrelated to checkdoc or what
it does.  I don't want to remove that advice completely, as I already
said and explained.  But I'm okay with making the text be a more
general advice as opposed to some rigid rule.





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#50599: [External] : bug#50599: [PATCH] Don't recommend against "\[...]" substitutions for performance
  2021-09-15 14:13       ` Stefan Kangas
  2021-09-15 15:41         ` Eli Zaretskii
@ 2021-09-15 15:46         ` Drew Adams
  1 sibling, 0 replies; 10+ messages in thread
From: Drew Adams @ 2021-09-15 15:46 UTC (permalink / raw)
  To: Stefan Kangas, Eli Zaretskii; +Cc: 50599

> I see no reason to keep use of "\\[...]" to the minimum.  In any
> realistic use, my investigation has demonstrated that there is no
> problem with using it for reasons of performance.
> 
> Instead of discouraging it, we should encourage its use, as it leads
> to better documentation.  For example, compare the current
> 'ibuffer-mode' docstring to what you get if you replace the list of
> commands (with its categories, explanations, etc.) with a blanket
> "\\{ibuffer-mode-map}".  So I find the advice not only irrelevant but
> misleading.

Haven't looked at Stefan's patch, but FWIW I agree
with what he's said: encourage, don't discourage,
the use.  That's the right approach - 100%.

It's fine to simply mention that \[...] (and 
\\{...} and \\<...> perhaps?) takes time to look
up information.  Based on just that info users can
themselves figure out how much they want to use
these constructs, based on expected users of their
doc strings, context, etc.

There's no need for more than a mention that these
constructs require some processing - IMHO.



^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#50599: [PATCH] Don't recommend against "\[...]" substitutions for performance
  2021-09-15 15:41         ` Eli Zaretskii
@ 2021-09-15 18:37           ` Stefan Kangas
  2021-09-15 19:03             ` Eli Zaretskii
  0 siblings, 1 reply; 10+ messages in thread
From: Stefan Kangas @ 2021-09-15 18:37 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 50599

> I didn't realize that checkdoc is involved in this.  If the problem is
> that it produces annoying diagnostics for \\[..], then I'm okay with
> removing it or making it less frequent.

Thanks, I've pushed a patch to that effect.

For anyone interested in looking at the rest of this bug report, the
text that should be deleted is from 1994 (commit 7015aca4520).  That
was the "initial revision" so the text might been around for much
longer than that.  (It makes sense to me that this would have been a
concern in the late 1980's or early 1990's.)





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#50599: [PATCH] Don't recommend against "\[...]" substitutions for performance
  2021-09-15 18:37           ` Stefan Kangas
@ 2021-09-15 19:03             ` Eli Zaretskii
  2021-09-16 14:08               ` Lars Ingebrigtsen
  0 siblings, 1 reply; 10+ messages in thread
From: Eli Zaretskii @ 2021-09-15 19:03 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: 50599

> From: Stefan Kangas <stefan@marxist.se>
> Date: Wed, 15 Sep 2021 20:37:36 +0200
> Cc: 50599@debbugs.gnu.org
> 
> > I didn't realize that checkdoc is involved in this.  If the problem is
> > that it produces annoying diagnostics for \\[..], then I'm okay with
> > removing it or making it less frequent.
> 
> Thanks, I've pushed a patch to that effect.

Thanks, I installed a followup that rewords the manual text.





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#50599: [PATCH] Don't recommend against "\[...]" substitutions for performance
  2021-09-15 19:03             ` Eli Zaretskii
@ 2021-09-16 14:08               ` Lars Ingebrigtsen
  0 siblings, 0 replies; 10+ messages in thread
From: Lars Ingebrigtsen @ 2021-09-16 14:08 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Stefan Kangas, 50599

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, I installed a followup that rewords the manual text.

Looks OK to me, so I'm now closing this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-09-16 14:08 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-15  6:27 bug#50599: [PATCH] Don't recommend against "\[...]" substitutions for performance Stefan Kangas
2021-09-15  6:38 ` Eli Zaretskii
2021-09-15  7:11   ` Stefan Kangas
2021-09-15  8:24     ` Eli Zaretskii
2021-09-15 14:13       ` Stefan Kangas
2021-09-15 15:41         ` Eli Zaretskii
2021-09-15 18:37           ` Stefan Kangas
2021-09-15 19:03             ` Eli Zaretskii
2021-09-16 14:08               ` Lars Ingebrigtsen
2021-09-15 15:46         ` bug#50599: [External] : " Drew Adams

Code repositories for project(s) associated with this inbox:

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).