bug#48738: [PATCH] Optimized `describe-variable' predicate

bug#48738: [PATCH] Optimized `describe-variable' predicate

[Daniel Mendler]

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

Do not switch to the original buffer.  Use `buffer-local-value`
instead.  The buffer switching is more costly, which slows down
continuously updating completion UIs like Icomplete or Vertico if
there are many variables.

Ideally there would be a `buffer-local-boundp` predicate we could use
here.

In GNU Emacs 28.0.50 (build 12, x86_64-pc-linux-gnu, GTK+ Version
3.24.5, cairo version 1.16.0)
 of 2021-05-23 built on projects
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux 10 (buster)

[-- Attachment #2: 0001-describe-variable-Do-not-switch-to-the-original-buff.patch --]
[-- Type: text/x-diff, Size: 1877 bytes --]

From 9100bce6f057787df9f255a9bb98f37bbef44940 Mon Sep 17 00:00:00 2001
From: Daniel Mendler <mail@daniel-mendler.de>
Date: Sun, 30 May 2021 06:16:17 +0200
Subject: [PATCH] (describe-variable): Do not switch to the original buffer in
 the predicate

Do not switch to the original buffer.  Use `buffer-local-value`
instead.  The buffer switching is more costly, which slows down
continuously updating completion UIs like Icomplete or Vertico if
there are many variables.

* lisp/help-fns.el (describe-variable): Use optimized predicate.
---
 lisp/help-fns.el | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/lisp/help-fns.el b/lisp/help-fns.el
index 0b0ae4364c..46ace6bd24 100644
--- a/lisp/help-fns.el
+++ b/lisp/help-fns.el
@@ -1022,12 +1022,14 @@ describe-variable
                 (format-prompt "Describe variable" (and (symbolp v) v))
                 #'help--symbol-completion-table
                 (lambda (vv)
-                  ;; In case the variable only exists in the buffer
-                  ;; the command we switch back to that buffer before
-                  ;; we examine the variable.
-                  (with-current-buffer orig-buffer
-                    (or (get vv 'variable-documentation)
-                        (and (boundp vv) (not (keywordp vv))))))
+                  (or (get vv 'variable-documentation)
+                      (and (not (keywordp vv))
+                           ;; Since the variable may only exists in
+                           ;; the original buffer, we have to look for
+                           ;; it there.
+                           (ignore-errors
+                             (or (buffer-local-value vv orig-buffer)
+                                 t)))))
                 t nil nil
                 (if (symbolp v) (symbol-name v))))
      (list (if (equal val "")
-- 
2.20.1

bug#48738: [PATCH] Optimized `describe-variable' predicate

[Lars Ingebrigtsen]

Daniel Mendler <mail@daniel-mendler.de> writes:

> Do not switch to the original buffer.  Use `buffer-local-value`
> instead.  The buffer switching is more costly, which slows down
> continuously updating completion UIs like Icomplete or Vertico if
> there are many variables.

Thanks; applied to Emacs 28 with some changes -- I thought the
`ignore-errors' logic was kinda unclear, so I rewrote that bit using an
explicit `condition-case' checking for the error thrown in this case.

Adjusted patch included below; hopefully I didn't introduce any errors
here...

> Ideally there would be a `buffer-local-boundp` predicate we could use
> here.

Yes, that would be nice, I think.

And we probably have a lot of code in Emacs that uses
`with-current-buffer' instead of `buffer-local-value' that could be
similarly optimised (but I guess it mostly only makes sense to do so
when it's called in an intensive loop like here).

diff --git a/lisp/help-fns.el b/lisp/help-fns.el
index 666583db72..c8f078cb85 100644
--- a/lisp/help-fns.el
+++ b/lisp/help-fns.el
@@ -1024,12 +1024,15 @@ describe-variable
                 (format-prompt "Describe variable" (and (symbolp v) v))
                 #'help--symbol-completion-table
                 (lambda (vv)
-                  ;; In case the variable only exists in the buffer
-                  ;; the command we switch back to that buffer before
-                  ;; we examine the variable.
-                  (with-current-buffer orig-buffer
-                    (or (get vv 'variable-documentation)
-                        (and (boundp vv) (not (keywordp vv))))))
+                  (or (get vv 'variable-documentation)
+                      (and (not (keywordp vv))
+                           ;; Since the variable may only exist in the
+                           ;; original buffer, we have to look for it
+                           ;; there.
+                           (condition-case nil
+                               (buffer-local-value vv orig-buffer)
+                             (:success t)
+                             (void-variable nil)))))
                 t nil nil
                 (if (symbolp v) (symbol-name v))))
      (list (if (equal val "")


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

bug#48738: [PATCH] Optimized `describe-variable' predicate

[Daniel Mendler]

On 5/31/21 6:55 AM, Lars Ingebrigtsen wrote:
> Thanks; applied to Emacs 28 with some changes -- I thought the
> `ignore-errors' logic was kinda unclear, so I rewrote that bit using an
> explicit `condition-case' checking for the error thrown in this case.

Makes sense. Thanks for applying the patch.

> Adjusted patch included below; hopefully I didn't introduce any errors
> here...
> 
>> Ideally there would be a `buffer-local-boundp` predicate we could use
>> here.
> 
> Yes, that would be nice, I think.
> 
> And we probably have a lot of code in Emacs that uses
> `with-current-buffer' instead of `buffer-local-value' that could be
> similarly optimised (but I guess it mostly only makes sense to do so
> when it's called in an intensive loop like here).

Yes, when I find such case I will report them or provide a patch.
However it really matters in very few cases. Here the predicate is
called 10k+ times when generating all completions.

bug#48738: [PATCH] Optimized `describe-variable' predicate

[Lars Ingebrigtsen]

Daniel Mendler <mail@daniel-mendler.de> writes:

> Yes, when I find such case I will report them or provide a patch.
> However it really matters in very few cases. Here the predicate is
> called 10k+ times when generating all completions.

Yup.

I think I'll just go ahead and add `buffer-local-boundp' now -- it'll
perhaps encourage further usage of `buffer-local-value'.  I remember
doing things like

(with-current-buffer foo
  (and (boundp 'zot) zot))

because there's no `buffer-local-boundp'.

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

bug#48738: [PATCH] Optimized `describe-variable' predicate

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I think I'll just go ahead and add `buffer-local-boundp' now -- it'll
> perhaps encourage further usage of `buffer-local-value'.  I remember
> doing things like
>
> (with-current-buffer foo
>   (and (boundp 'zot) zot))
>
> because there's no `buffer-local-boundp'.

local-variable-p and local-variable-if-set-p, but neither really do what
you want in this case.

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

bug#48738: [PATCH] Optimized `describe-variable' predicate

[Daniel Mendler]

On 5/31/21 7:13 AM, Lars Ingebrigtsen wrote:
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
>> I think I'll just go ahead and add `buffer-local-boundp' now -- it'll
>> perhaps encourage further usage of `buffer-local-value'.  I remember
>> doing things like
>>
>> (with-current-buffer foo
>>   (and (boundp 'zot) zot))
>>
>> because there's no `buffer-local-boundp'.
> 
> local-variable-p and local-variable-if-set-p, but neither really do what
> you want in this case.

Yes, I am aware of those. As you say, they don't do what we want here.
The question is which name convention to use here. I think having a
`buffer-local-boundp` makes sense.

bug#48738: [PATCH] Optimized `describe-variable' predicate

[Lars Ingebrigtsen]

Daniel Mendler <mail@daniel-mendler.de> writes:

> Yes, I am aware of those. As you say, they don't do what we want here.
> The question is which name convention to use here. I think having a
> `buffer-local-boundp` makes sense.

Added now.

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

bug#48738: [PATCH] Optimized `describe-variable' predicate

[Daniel Mendler]

On 5/31/21 7:23 AM, Lars Ingebrigtsen wrote:
> Daniel Mendler <mail@daniel-mendler.de> writes:
> 
>> Yes, I am aware of those. As you say, they don't do what we want here.
>> The question is which name convention to use here. I think having a
>> `buffer-local-boundp` makes sense.
> 
> Added now.

Thanks. While we are on it optimizing, does it make sense to make this a
c function? You can probably tell more about the overhead of condition-case?

However I think `buffer-local-boundp` is really only performance
critical in this single predicate in `describe-variable`. I cannot think
of another use case iterating over thousands of symbols and checking for
bound variables. So probably we are good now.

bug#48738: [PATCH] Optimized `describe-variable' predicate

[Lars Ingebrigtsen]

Daniel Mendler <mail@daniel-mendler.de> writes:

> Thanks. While we are on it optimizing, does it make sense to make this a
> c function? You can probably tell more about the overhead of condition-case?

Making it a C function might make sense, but condition-case is really
fast -- it's basically negligible, if I remember previous benchmarkings
of that correctly.

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