GNU bug report logs - #50599
[PATCH] Don't recommend against "\[...]" substitutions for performance

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 50599 in the body.
You can then email your comments to 50599 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Don't recommend against "\[...]" substitutions for performance
Date: Wed, 15 Sep 2021 08:27:55 +0200

[Message part 1 (text/plain, inline)]

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.

[0001-Don-t-recommend-against-using-.-substitutions-many-t.patch (text/x-patch, attachment)]

Message #8 received at 50599 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 50599 <at> debbugs.gnu.org
Subject: Re: bug#50599: [PATCH] Don't recommend against "\[...]" substitutions
 for performance
Date: Wed, 15 Sep 2021 09:38:53 +0300

> From: Stefan Kangas <stefan <at> 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.

Message #11 received at 50599 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 50599 <at> debbugs.gnu.org
Subject: Re: bug#50599: [PATCH] Don't recommend against "\[...]" substitutions
 for performance
Date: Wed, 15 Sep 2021 09:11:02 +0200

Eli Zaretskii <eliz <at> 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.

Message #14 received at 50599 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 50599 <at> debbugs.gnu.org
Subject: Re: bug#50599: [PATCH] Don't recommend against "\[...]" substitutions
 for performance
Date: Wed, 15 Sep 2021 11:24:14 +0300

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Wed, 15 Sep 2021 09:11:02 +0200
> Cc: 50599 <at> 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.

Message #17 received at 50599 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 50599 <at> debbugs.gnu.org
Subject: Re: bug#50599: [PATCH] Don't recommend against "\[...]" substitutions
 for performance
Date: Wed, 15 Sep 2021 16:13:59 +0200

Eli Zaretskii <eliz <at> 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?

Message #20 received at 50599 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 50599 <at> debbugs.gnu.org
Subject: Re: bug#50599: [PATCH] Don't recommend against "\[...]" substitutions
 for performance
Date: Wed, 15 Sep 2021 18:41:27 +0300

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Wed, 15 Sep 2021 16:13:59 +0200
> Cc: 50599 <at> 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.

Message #23 received at 50599 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Stefan Kangas <stefan <at> marxist.se>, Eli Zaretskii <eliz <at> gnu.org>
Cc: "50599 <at> debbugs.gnu.org" <50599 <at> debbugs.gnu.org>
Subject: RE: [External] : bug#50599: [PATCH] Don't recommend against "\[...]"
 substitutions for performance
Date: Wed, 15 Sep 2021 15:46:52 +0000

> 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.

Message #26 received at 50599 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 50599 <at> debbugs.gnu.org
Subject: Re: bug#50599: [PATCH] Don't recommend against "\[...]" substitutions
 for performance
Date: Wed, 15 Sep 2021 20:37:36 +0200

> 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.)

Message #29 received at 50599 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 50599 <at> debbugs.gnu.org
Subject: Re: bug#50599: [PATCH] Don't recommend against "\[...]" substitutions
 for performance
Date: Wed, 15 Sep 2021 22:03:42 +0300

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Wed, 15 Sep 2021 20:37:36 +0200
> Cc: 50599 <at> 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.

Message #32 received at 50599 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Stefan Kangas <stefan <at> marxist.se>, 50599 <at> debbugs.gnu.org
Subject: Re: bug#50599: [PATCH] Don't recommend against "\[...]"
 substitutions for performance
Date: Thu, 16 Sep 2021 16:08:35 +0200

Eli Zaretskii <eliz <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.