bug#63622: lisp/progmodes/python.el: performance regression introduced by multiline font-lock

bug#63622: lisp/progmodes/python.el: performance regression introduced by multiline font-lock

[Tom Gillespie]

The changes in 4915ca5dd4245a909c046e6691e8d4a1919890c8 have
introduced a significant performance regression when editing python
code that contains dictionary structures across multiple lines.

The current behavior makes Emacs unusable when editing python
dictionaries with more than 20 or so lines. I would suggest reverting
the commit until the performance issue can be addressed.

If I had to guess, this is probably being caused by a double
zero-or-more pattern (possibly implicit) in the new regexps that were
added/changed.

The literal dictionary below is sufficient to demonstrate the issue
and if you bisect and compare the behavior to the immediately prior
commit 31e32212670f5774a6dbc0debac8854fa01d8f92 the difference is
clear. Open the file and hit enter a couple of times and the lag is
obvious (if you can't detect the issue try doubling the number of
lines at the deepest nesting level from 25 to 50).

By profiling and varying the number of repeated lines (e.g. by
doubling them) it appears that the issue is some lurking quadratic
behavior in syntax-ppss as a result of the changes in 4914ca. In my
testing 25, 50, and 100 lines take 100ms, 800ms, and 5 seconds
respectively to insert a new line while the cursor is inside the outer
most paren.

Collapsing all the structures into one line hides the issue. The
longer each individual line the more rapid the slowdown.

The example below is not syntactically correct python, however it
highlights the issue in a way that is clearer than it would be
otherwise.

Examples that trigger the issue (repeat the 2nd line 50 or 100 times
to see the effect). Any combination of paren types will cause the
issue.  The closing paren does not have to be present and does not
prevent the issue.

#+begin_src python
[''
'' []
#+end_src
#+begin_src python
[''
[] ''
#+end_src
#+begin_src python
[''
'' {}
#+end_src
#+begin_src python
{''
'' ()
#+end_src
#+begin_src python
[""
'' []
#+end_src

Examples that are do not cause the issue.
#+begin_src python
[a
'' []
#+end_src
#+begin_src python
[''
'' a
#+end_src
#+begin_src python
[''
'' ''
#+end_src
#+begin_src python
[[]
[] []
#+end_src
#+begin_src python
[[]
[] ''
#+end_src

Example of syntactically correct python that causes the issue.
#+begin_src python
{'':
 {
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
  '': {'': ''},
 },
 '': ['']}
#+end_src

bug#63622: source of problem identified to be python-font-lock-extend-region

[Tom Gillespie]

Despite my previous speculation about the regexp,
the issue is in python-font-lock-extend-region. When
replaced with a no-op the performance returns to normal.

bug#63622: lisp/progmodes/python.el: performance regression introduced by multiline font-lock

[Eli Zaretskii]

> From: Tom Gillespie <tgbugs@gmail.com>
> Date: Sat, 20 May 2023 20:14:19 -0700
> 
> The changes in 4915ca5dd4245a909c046e6691e8d4a1919890c8 have
> introduced a significant performance regression when editing python
> code that contains dictionary structures across multiple lines.
> 
> The current behavior makes Emacs unusable when editing python
> dictionaries with more than 20 or so lines. I would suggest reverting
> the commit until the performance issue can be addressed.

If the problem is so severe, I wonder how come this comes up only now,
9 months after those changes were installed.  It probably means these
cases are quite rare in practice.  Nevertheless, it would be good to
solve them, of course.

FWIW, python-ts-mode doesn't show performance issues in the examples
you posted.

> The literal dictionary below is sufficient to demonstrate the issue
> and if you bisect and compare the behavior to the immediately prior
> commit 31e32212670f5774a6dbc0debac8854fa01d8f92 the difference is
> clear. Open the file and hit enter a couple of times and the lag is
> obvious (if you can't detect the issue try doubling the number of
> lines at the deepest nesting level from 25 to 50).
> 
> By profiling and varying the number of repeated lines (e.g. by
> doubling them) it appears that the issue is some lurking quadratic
> behavior in syntax-ppss as a result of the changes in 4914ca. In my
> testing 25, 50, and 100 lines take 100ms, 800ms, and 5 seconds
> respectively to insert a new line while the cursor is inside the outer
> most paren.
> 
> Collapsing all the structures into one line hides the issue. The
> longer each individual line the more rapid the slowdown.
> 
> The example below is not syntactically correct python, however it
> highlights the issue in a way that is clearer than it would be
> otherwise.

Any chance of your posting some real-life Python code where the issue
rears its head?  I mean, real-life code that makes sense, not just
syntactically correct code invented to make a point?

> Despite my previous speculation about the regexp,
> the issue is in python-font-lock-extend-region. When
> replaced with a no-op the performance returns to normal.

kobarity, could you please look into this ASAP?  Emacs 29.1 is in late
stages of pretest, and I'd like to have this resolved, one way or
another, soon enough.  TIA.

P.S.  Tom, please don't change the Subject when posting followups,
please use the same Subject for all your messages that discuss this
bug.

Also, for the record, please state which Emacs version are you using.
You didn't use "M-x report-emacs-bug" to submit the bug report, so
this and other important information is missing from your OP.

bug#63622: lisp/progmodes/python.el: performance regression introduced by multiline font-lock

[Tom Gillespie]

> If the problem is so severe, I wonder how come this comes up only now,
> 9 months after those changes were installed.  It probably means these
> cases are quite rare in practice.  Nevertheless, it would be good to
> solve them, of course.

I suspect it is because there are 3 factors that have to be just right
to notice.

1 the opening paren and the quote must be immediately adjacent to get
exceptionally bad behavior, there is still some performance degradation
when there is separation, but it would be harder to notice.

2 a user would have to directly edit a dictionary literal with enough lines
to notice the slowdown.

3 assigning the dictionary to a variable mitigates the issue, so only a
dict that is not assigned results in the full slowdown.

> FWIW, python-ts-mode doesn't show performance issues in the examples
> you posted.

I would imagine so. I've continued trying to hunt down the source of the
issue, and it is triggered by setting python-font-lock-extend-region as
the font-lock-extend-after-change-region-function function for python
this is true in old versions of Emacs (e.g. 28.2) as well.

As far as I can tell the existing implementation for python font locking
has some quadratic behavior that is revealed when a region is extended
inside a nested dictionary with multiple lines.

> Any chance of your posting some real-life Python code where the issue
> rears its head?  I mean, real-life code that makes sense, not just
> syntactically correct code invented to make a point?

Yep, basically any nested dictionary literal with more than 15 lines is
affected. With a note that the issue is masked if there is an equal sign
(=) before the opening paren, which is the common case.

An example of the particular file that caused me to spot the issue:
https://github.com/tgbugs/pyontutils/blob/master/pyontutils/auth-config.py

> P.S.  Tom, please don't change the Subject when posting followups,
> please use the same Subject for all your messages that discuss this
> bug.

Ack, apologies. I will keep it the same in the future.

> Also, for the record, please state which Emacs version are you using.
> You didn't use "M-x report-emacs-bug" to submit the bug report, so
> this and other important information is missing from your OP.

Ok, I wasn't sure how to handle it in this case since I was able to
reproduce the issue in multiple versions.

For the record:
The version I spotted it on was the emacs-29 branch at
3bc5efb87e5ac9b7068e71307466b2d0220e92fb but everything
on emacs-29 after 4915ca5dd4245a909c046e6691e8d4a1919890c8
is affected (according to git bisect results). So 29.0.90 and 29.0.91
should be affected as well.

bug#63622: lisp/progmodes/python.el: performance regression introduced by multiline font-lock

[kobarity]

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


Tom Gillespie wrote:
> The changes in 4915ca5dd4245a909c046e6691e8d4a1919890c8 have
> introduced a significant performance regression when editing python
> code that contains dictionary structures across multiple lines.

Hi Tom and Eli,

Thanks for bringing this issue to my attention.

> As far as I can tell the existing implementation for python font locking
> has some quadratic behavior that is revealed when a region is extended
> inside a nested dictionary with multiple lines.

I agree.  It seems to me that it is not python-font-lock-extend-region
itself that is slow, but rather font-lock's processing of the area
extended by it.  So one workaround would be to limit the number of
lines to be extended, as in the attached patch.  If this limit is
exceeded, the area or the entire buffer must be font-locked manually
later.  What do you think of this idea?

Even if we adopt this idea, there remain several points to consider:

- How many lines are appropriate for the limit?
- Is it better to make the limit customizable?
- python-ts-mode should be excluded for this limit?

[-- Attachment #2: 0001-Workaround-performance-degradation-when-editing-mult.patch --]
[-- Type: application/octet-stream, Size: 1754 bytes --]

From 3224ef9d2718c85281f1fa789708efb0b5aa5fff Mon Sep 17 00:00:00 2001
From: kobarity <kobarity@gmail.com>
Date: Sun, 21 May 2023 17:50:09 +0900
Subject: [PATCH] Workaround performance degradation when editing multiline
 Python expression

* lisp/progmodes/python.el (python-font-lock-extend-max-lines): New
variable.
(python-font-lock-extend-region): Limit extending the region to
python-font-lock-extend-max-lines.  (Bug#63622)
---
 lisp/progmodes/python.el | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 6fc05b246a6..82d15536de0 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -869,16 +869,25 @@ python-font-lock-keywords
 Which one will be chosen depends on the value of
 `font-lock-maximum-decoration'.")
 
+(defvar python-font-lock-extend-max-lines 10
+  "Maximum number of lines to extend the font-lock region.
+This is a workaround to avoid performance degradation when
+editing expressions that span many lines.  See Emacs Bug#63622.")
+
 (defun python-font-lock-extend-region (beg end _old-len)
   "Extend font-lock region given by BEG and END to statement boundaries."
   (save-excursion
     (save-match-data
       (goto-char beg)
       (python-nav-beginning-of-statement)
-      (setq beg (point))
+      (when (<= (- (line-number-at-pos beg) (line-number-at-pos))
+                python-font-lock-extend-max-lines)
+        (setq beg (point)))
       (goto-char end)
       (python-nav-end-of-statement)
-      (setq end (point))
+      (when (<= (- (line-number-at-pos) (line-number-at-pos end))
+                python-font-lock-extend-max-lines)
+        (setq end (point)))
       (cons beg end))))
 
 
-- 
2.34.1

bug#63622: lisp/progmodes/python.el: performance regression introduced by multiline font-lock

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> I agree.  It seems to me that it is not python-font-lock-extend-region
> itself that is slow, but rather font-lock's processing of the area
> extended by it.  So one workaround would be to limit the number of
> lines to be extended, as in the attached patch.  If this limit is
> exceeded, the area or the entire buffer must be font-locked manually
> later.  What do you think of this idea?

FWIW, I recommend against using
`font-lock-extend-after-change-region-function`.

E.g. in a case like `python-font-lock-assignment-statement-multiline-1`,
the current code may misfontify code because jit-lock may decide to first
call font-lock on a chunk that goes until:

    [
        a,
        b

and will call again font-lock later to fontify the rest:

    ] # (
        1,
        2
    )

and this can happen with no buffer modification at all (e.g. on the
initial fontification of a buffer).

You can use `font-lock-extend-region-functions` instead (which performs
the region-extension right before fontifying a chunk) to avoid this problem.
[ It won't help with the current performance issue, tho.  ]

`font-lock-extend-after-change-region-function` can also be costly when
a command makes many changes (since
`font-lock-extend-after-change-region-function` is called for every such
change rather than once at the end).
`font-lock-extend-region-functions` tends to be better behaved in this
respect (it's called once per chunk, and there's usually only
a single chunk (re)fontified per command, even after a command that makes
many changes),

One more thing: Tom mentioned a suspicion that the performance issue may
have to do with interaction with `syntax-ppss`.  This is odd, because
`syntax-ppss` and `syntax-propertize` should not be affected by
font-lock.


        Stefan

bug#63622: lisp/progmodes/python.el: performance regression introduced by multiline font-lock

[kobarity]

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

Stefan Monnier wrote:
> FWIW, I recommend against using
> `font-lock-extend-after-change-region-function`.
> 
> You can use `font-lock-extend-region-functions` instead (which performs
> the region-extension right before fontifying a chunk) to avoid this problem.
> [ It won't help with the current performance issue, tho.  ]

Thank you for your advice.  Does the attached patch seem reasonable?

[-- Attachment #2: 0001-Use-font-lock-extend-region-functions-in-python-mode.patch --]
[-- Type: application/octet-stream, Size: 2381 bytes --]

From fb899c0d9596c5912db1dc2f518ff1bab00c9b0d Mon Sep 17 00:00:00 2001
From: kobarity <kobarity@gmail.com>
Date: Mon, 22 May 2023 23:42:28 +0900
Subject: [PATCH] Use font-lock-extend-region-functions in python-mode

* lisp/progmodes/python.el (python-font-lock-extend-region): Change
arguments and return value for python-font-lock-extend-region.
(python-mode): Change from
font-lock-extend-after-change-region-function to
python-font-lock-extend-region.  (Bug#63622)
---
 lisp/progmodes/python.el | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 6fc05b246a6..03d57d0b378 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -869,18 +869,20 @@ python-font-lock-keywords
 Which one will be chosen depends on the value of
 `font-lock-maximum-decoration'.")
 
-(defun python-font-lock-extend-region (beg end _old-len)
-  "Extend font-lock region given by BEG and END to statement boundaries."
-  (save-excursion
-    (save-match-data
-      (goto-char beg)
-      (python-nav-beginning-of-statement)
-      (setq beg (point))
-      (goto-char end)
-      (python-nav-end-of-statement)
-      (setq end (point))
-      (cons beg end))))
-
+(defvar font-lock-beg)
+(defvar font-lock-end)
+(defun python-font-lock-extend-region ()
+  "Extend font-lock region to statement boundaries."
+  (let ((beg font-lock-beg)
+        (end font-lock-end))
+    (goto-char beg)
+    (python-nav-beginning-of-statement)
+    (setq font-lock-beg (point))
+    (goto-char end)
+    (python-nav-end-of-statement)
+    (when (not (eobp)) (forward-char))
+    (setq font-lock-end (point))
+    (or (/= beg font-lock-beg) (/= end font-lock-end))))
 
 (defconst python-syntax-propertize-function
   (syntax-propertize-rules
@@ -6708,8 +6710,8 @@ python-mode
                 nil nil nil nil
                 (font-lock-syntactic-face-function
                  . python-font-lock-syntactic-face-function)
-                (font-lock-extend-after-change-region-function
-                 . python-font-lock-extend-region)))
+                (font-lock-extend-region-functions
+                 . (python-font-lock-extend-region))))
   (setq-local syntax-propertize-function
               python-syntax-propertize-function)
   (setq-local imenu-create-index-function
-- 
2.34.1

bug#63622: lisp/progmodes/python.el: performance regression introduced by multiline font-lock

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> @@ -869,18 +869,20 @@ python-font-lock-keywords
>  Which one will be chosen depends on the value of
>  `font-lock-maximum-decoration'.")
>  
> -(defun python-font-lock-extend-region (beg end _old-len)
> -  "Extend font-lock region given by BEG and END to statement boundaries."
> -  (save-excursion
> -    (save-match-data
> -      (goto-char beg)
> -      (python-nav-beginning-of-statement)
> -      (setq beg (point))
> -      (goto-char end)
> -      (python-nav-end-of-statement)
> -      (setq end (point))
> -      (cons beg end))))
> -
> +(defvar font-lock-beg)
> +(defvar font-lock-end)
> +(defun python-font-lock-extend-region ()
> +  "Extend font-lock region to statement boundaries."
> +  (let ((beg font-lock-beg)
> +        (end font-lock-end))
> +    (goto-char beg)
> +    (python-nav-beginning-of-statement)
> +    (setq font-lock-beg (point))
> +    (goto-char end)
> +    (python-nav-end-of-statement)
> +    (when (not (eobp)) (forward-char))
> +    (setq font-lock-end (point))
> +    (or (/= beg font-lock-beg) (/= end font-lock-end))))
>  
>  (defconst python-syntax-propertize-function
>    (syntax-propertize-rules

Looks fine to me (I assume you've checked that it behaves about as well
as the previous code).

> @@ -6708,8 +6710,8 @@ python-mode
>                  nil nil nil nil
>                  (font-lock-syntactic-face-function
>                   . python-font-lock-syntactic-face-function)
> -                (font-lock-extend-after-change-region-function
> -                 . python-font-lock-extend-region)))
> +                (font-lock-extend-region-functions
> +                 . (python-font-lock-extend-region))))
>    (setq-local syntax-propertize-function
>                python-syntax-propertize-function)
>    (setq-local imenu-create-index-function

This is bound to break in some cases.  Please use `add-hook` instead.


        Stefan

bug#63622: lisp/progmodes/python.el: performance regression introduced by multiline font-lock

[kobarity]

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

Stefan Monnier wrote:
> > @@ -6708,8 +6710,8 @@ python-mode
> >                  nil nil nil nil
> >                  (font-lock-syntactic-face-function
> >                   . python-font-lock-syntactic-face-function)
> > -                (font-lock-extend-after-change-region-function
> > -                 . python-font-lock-extend-region)))
> > +                (font-lock-extend-region-functions
> > +                 . (python-font-lock-extend-region))))
> >    (setq-local syntax-propertize-function
> >                python-syntax-propertize-function)
> >    (setq-local imenu-create-index-function
> 
> This is bound to break in some cases.  Please use `add-hook` instead.

Thanks.  I also revised python-font-lock-extend-region.  The attached
0001-Use-font-lock-extend-region-functions-in-python-mode.patch is the
revised patch.

As for the performance degradation, I am almost certain that it is a
bug in python-info-docstring-p and/or python-rx string-delimiter.
python-info-docstring-p uses the following regex as one of the
conditions to determine if it is a docstring.

          (re (concat "[uU]?[rR]?"
                      (python-rx string-delimiter))))

One of the problems is that string-delimiter matches even if there is
a single character preceding the string.  For example,
string-delimiter matches both x" and 1".  It might be reasonable to
match r" or b", for example.  Because they are correct string starter
in Python.  However, there is no reason to match x" or 1".  Besides,
python-info-docstring-p explicitly states "[uU]?[rR]?" as above.  So
(python-rx string-delimiter) in the above code should only match
string delimiter without prefixes.  Current python-info-docstring-p
incorrectly considers a code like [""] or {""} to be a docstring.

The performance problem in the example shown by Tom can be resolved by
modifying the above code as follows:

          (re (concat "[uU]?[rR]?"
                      (rx (or "\"\"\"" "\"" "'''" "'")))))

It breaks some ERTs, but I think we should fix the ERTs.  However,
there seems to be another problem in python-info-docstring-p.  It
intentionally considers contiguous strings as docstring as in the ERT
python-info-docstring-p-1:

'''
Module Docstring Django style.
'''
u'''Additional module docstring.'''
'''Not a module docstring.'''

However, as far as I have tried with Python 3 and Python 2.7, this is
not correct.  Therefore, I feel it is better to mark the ERTs as
expected failures than to modify it at this stage.  The attached
0001-Fix-python-info-docstring-p.patch is the patch to do this.

[-- Attachment #2: 0001-Use-font-lock-extend-region-functions-in-python-mode.patch --]
[-- Type: application/octet-stream, Size: 2537 bytes --]

From 7ea71e0c07a10959a93e4a1e9a219ad8f6810d22 Mon Sep 17 00:00:00 2001
From: kobarity <kobarity@gmail.com>
Date: Tue, 23 May 2023 21:59:18 +0900
Subject: [PATCH] Use font-lock-extend-region-functions in python-mode

* lisp/progmodes/python.el (python-font-lock-extend-region): Change
arguments and return value for font-lock-extend-region-functions.
(python-mode): Change from
font-lock-extend-after-change-region-function to
font-lock-extend-region-functions.  (Bug#63622)
---
 lisp/progmodes/python.el | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 6fc05b246a6..b363ef874be 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -869,18 +869,22 @@ python-font-lock-keywords
 Which one will be chosen depends on the value of
 `font-lock-maximum-decoration'.")
 
-(defun python-font-lock-extend-region (beg end _old-len)
-  "Extend font-lock region given by BEG and END to statement boundaries."
-  (save-excursion
-    (save-match-data
-      (goto-char beg)
-      (python-nav-beginning-of-statement)
-      (setq beg (point))
-      (goto-char end)
-      (python-nav-end-of-statement)
-      (setq end (point))
-      (cons beg end))))
-
+(defvar font-lock-beg)
+(defvar font-lock-end)
+(defun python-font-lock-extend-region ()
+  "Extend font-lock region to statement boundaries."
+  (let ((beg font-lock-beg)
+        (end font-lock-end))
+    (goto-char beg)
+    (python-nav-beginning-of-statement)
+    (beginning-of-line)
+    (when (< (point) beg)
+      (setq font-lock-beg (point)))
+    (goto-char end)
+    (python-nav-end-of-statement)
+    (when (< end (point))
+      (setq font-lock-end (point)))
+    (or (/= beg font-lock-beg) (/= end font-lock-end))))
 
 (defconst python-syntax-propertize-function
   (syntax-propertize-rules
@@ -6707,9 +6711,9 @@ python-mode
               `(,python-font-lock-keywords
                 nil nil nil nil
                 (font-lock-syntactic-face-function
-                 . python-font-lock-syntactic-face-function)
-                (font-lock-extend-after-change-region-function
-                 . python-font-lock-extend-region)))
+                 . python-font-lock-syntactic-face-function)))
+  (add-hook 'font-lock-extend-region-functions
+            #'python-font-lock-extend-region nil t)
   (setq-local syntax-propertize-function
               python-syntax-propertize-function)
   (setq-local imenu-create-index-function
-- 
2.34.1


[-- Attachment #3: 0001-Fix-python-info-docstring-p.patch --]
[-- Type: application/octet-stream, Size: 2655 bytes --]

From 42dbb7ddc177e98a5b88d51cb51ce993888e5f20 Mon Sep 17 00:00:00 2001
From: kobarity <kobarity@gmail.com>
Date: Wed, 24 May 2023 00:16:50 +0900
Subject: [PATCH] Fix python-info-docstring-p

* lisp/progmodes/python.el (python-info-docstring-p): Stop using
python-rx string-delimiter.
* test/lisp/progmodes/python-tests.el
(python-font-lock-escape-sequence-bytes-newline),
(python-font-lock-escape-sequence-hex-octal),
(python-font-lock-escape-sequence-unicode),
(python-font-lock-raw-escape-sequence): Mark as expected failures
until another bug in python-info-docstring-p is
corrected.  (Bug#63622)
---
 lisp/progmodes/python.el            | 2 +-
 test/lisp/progmodes/python-tests.el | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 6fc05b246a6..2f65f389949 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -6019,7 +6019,7 @@ python-info-docstring-p
           (indentation (current-indentation))
           (backward-sexp-point)
           (re (concat "[uU]?[rR]?"
-                      (python-rx string-delimiter))))
+                      (rx (or "\"\"\"" "\"" "'''" "'")))))
       (when (and
              (not (python-info-assignment-statement-p))
              (looking-at-p re)
diff --git a/test/lisp/progmodes/python-tests.el b/test/lisp/progmodes/python-tests.el
index 50153e66da5..bc3b574e81f 100644
--- a/test/lisp/progmodes/python-tests.el
+++ b/test/lisp/progmodes/python-tests.el
@@ -729,6 +729,7 @@ python-font-lock-escape-sequence-multiline-string
      (845 . font-lock-string-face) (886))))
 
 (ert-deftest python-font-lock-escape-sequence-bytes-newline ()
+  :expected-result :failed
   (python-tests-assert-faces
    "b'\\n'
 b\"\\n\""
@@ -741,6 +742,7 @@ python-font-lock-escape-sequence-bytes-newline
      (11 . font-lock-doc-face))))
 
 (ert-deftest python-font-lock-escape-sequence-hex-octal ()
+  :expected-result :failed
   (python-tests-assert-faces
    "b'\\x12 \\777 \\1\\23'
 '\\x12 \\777 \\1\\23'"
@@ -761,6 +763,7 @@ python-font-lock-escape-sequence-hex-octal
      (36 . font-lock-doc-face))))
 
 (ert-deftest python-font-lock-escape-sequence-unicode ()
+  :expected-result :failed
   (python-tests-assert-faces
    "b'\\u1234 \\U00010348 \\N{Plus-Minus Sign}'
 '\\u1234 \\U00010348 \\N{Plus-Minus Sign}'"
@@ -775,6 +778,7 @@ python-font-lock-escape-sequence-unicode
      (80 . font-lock-doc-face))))
 
 (ert-deftest python-font-lock-raw-escape-sequence ()
+  :expected-result :failed
   (python-tests-assert-faces
    "rb'\\x12 \123 \\n'
 r'\\x12 \123 \\n \\u1234 \\U00010348 \\N{Plus-Minus Sign}'"
-- 
2.34.1

bug#63622: lisp/progmodes/python.el: performance regression introduced by multiline font-lock

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Thanks.  I also revised python-font-lock-extend-region.  The attached
> 0001-Use-font-lock-extend-region-functions-in-python-mode.patch is the
> revised patch.

LGTM, thank you.

I'm not sufficiently familiar with Python to have an opinion on the
rest, tho your analysis sounds convincing.


        Stefan

bug#63622: lisp/progmodes/python.el: performance regression introduced by multiline font-lock

[Tom Gillespie]

I have tested the two patches and the performance is dramatically improved.

> One of the problems is that string-delimiter matches even if there is
> a single character preceding the string.

I agree with the assessment about docstring syntax. The fix in Fix-python-
is ok and fixes the font-locking performance in general. The general issue
with the definition of string-delimiter is something that might need to be
considered in the future if another perf issue crops up, but for now I think it
is safe to leave it alone since docstring detection was the only place where
it seems to be causing issues.

While we're here I think [fF]? should probably be included in the prefix since
format strings are also valid docstrings.

Thank you very much for digging into this!
Tom

bug#63622: lisp/progmodes/python.el: performance regression introduced by multiline font-lock

[kobarity]

Hi Stefan and Tom,

Thank you very much for your confirmation.

Tom Gillespie wrote:
> While we're here I think [fF]? should probably be included in the prefix since
> format strings are also valid docstrings.

Is that so?  As far as I have tried, f-string does not seem to be a
docstring.

Python 3.11.3 (main, May  2 2023, 21:12:31) [GCC 11.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> def f1():
...     "docstring"
...
>>> f1.__doc__
'docstring'
>>> def f2():
...     f"not docstring"
...
>>> f2.__doc__
>>> a = 1
>>> def f3():
...     f"not docstring {a}"
...
>>> f3.__doc__
>>>

bug#63622: lisp/progmodes/python.el: performance regression introduced by multiline font-lock

[Tom Gillespie]

> Is that so?  As far as I have tried, f-string does not seem to be a
> docstring.

Oops. Indeed you are correct. I remember this being an issue in one of my
files as I had to assign to __doc__ manually since of course docstrings do
not require the file to be run for the purposes of extracting documentation.

Best!
Tom

bug#63622: lisp/progmodes/python.el: performance regression introduced by multiline font-lock

[Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On May 23, 2023, at 23:46, kobarity <kobarity@gmail.com> wrote:
> 
> The performance problem in the example shown by Tom can be resolved by
> modifying the above code as follows:
> 
>          (re (concat "[uU]?[rR]?"
>                      (rx (or "\"\"\"" "\"" "'''" "'")))))

I didn’t read the context for this snippet, but isn’t it sufficient to match for only one single-quote and double-quote, instead of also matching for the triple (multiline) counterparts? 

bug#63622: lisp/progmodes/python.el: performance regression introduced by multiline font-lock

[kobarity]

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


Ruijie Yu wrote:
> On May 23, 2023, at 23:46, kobarity <kobarity@gmail.com> wrote:
> > The performance problem in the example shown by Tom can be resolved by
> > modifying the above code as follows:
> > 
> >          (re (concat "[uU]?[rR]?"
> >                      (rx (or "\"\"\"" "\"" "'''" "'")))))
> 
> I didn’t read the context for this snippet, but isn’t it sufficient to match for only one single-quote and double-quote, instead of also matching for the triple (multiline) counterparts? 

You are right.  I copied the above code from the definition of
python-rx string-delimiter.  However, it was inside of the group
construct.  As group capturing is not needed in
python-info-docstring-p, the regex can be simplified to:

          (re "[uU]?[rR]?[\"']"))

The same regex was used in another place in python-info-docstring-p.
So I fixed it too.  Also I added a simple ERT to identify this fix.

I wrote:
> It breaks some ERTs, but I think we should fix the ERTs.  However,
> there seems to be another problem in python-info-docstring-p.  It
> intentionally considers contiguous strings as docstring as in the ERT
> python-info-docstring-p-1:
> 
> '''
> Module Docstring Django style.
> '''
> u'''Additional module docstring.'''
> '''Not a module docstring.'''
> 
> However, as far as I have tried with Python 3 and Python 2.7, this is
> not correct.

This was my misunderstanding.  PEP-257
(https://www.python.org/dev/peps/pep-0257/#what-is-a-docstring)
clearly states:

#+begin_quote
String literals occurring elsewhere in Python code may also act as
documentation. They are not recognized by the Python bytecode compiler
and are not accessible as runtime object attributes (i.e. not assigned
to __doc__), but two types of extra docstrings may be extracted by
software tools:

1. String literals occurring immediately after a simple assignment at
    the top level of a module, class, or __init__ method are called
    “attribute docstrings”.
2. String literals occurring immediately after another docstring are
    called “additional docstrings”.
#+end_quote

However, there still seems to be a bug in python-info-docstring-p.
Therefore, I would like to keep failing ERTs as expected fail at this
time.  Maybe f-string can also be a docstring.

Attached is a series of patches that replace the previous patches.

[-- Attachment #2: 0001-Fix-python-info-docstring-p.patch --]
[-- Type: application/octet-stream, Size: 4001 bytes --]

From ce25adbbae75426ad568a7cd12e02cddc4e9e23e Mon Sep 17 00:00:00 2001
From: kobarity <kobarity@gmail.com>
Date: Wed, 24 May 2023 22:01:12 +0900
Subject: [PATCH 1/2] Fix python-info-docstring-p

* lisp/progmodes/python.el (python-info-docstring-p): Stop using
python-rx string-delimiter.
* test/lisp/progmodes/python-tests.el
(python-font-lock-escape-sequence-bytes-newline),
(python-font-lock-escape-sequence-hex-octal),
(python-font-lock-escape-sequence-unicode),
(python-font-lock-raw-escape-sequence): Mark as expected failures
until another bug in python-info-docstring-p is
corrected.
(python-info-docstring-p-7): New test.  (Bug#63622)
---
 lisp/progmodes/python.el            |  7 ++-----
 test/lisp/progmodes/python-tests.el | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 6fc05b246a6..032a17c52ff 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -6018,8 +6018,7 @@ python-info-docstring-p
     (let ((counter 1)
           (indentation (current-indentation))
           (backward-sexp-point)
-          (re (concat "[uU]?[rR]?"
-                      (python-rx string-delimiter))))
+          (re "[uU]?[rR]?[\"']"))
       (when (and
              (not (python-info-assignment-statement-p))
              (looking-at-p re)
@@ -6040,9 +6039,7 @@ python-info-docstring-p
                                                     backward-sexp-point))
                                      (setq last-backward-sexp-point
                                            backward-sexp-point))
-                                   (looking-at-p
-                                    (concat "[uU]?[rR]?"
-                                            (python-rx string-delimiter))))))
+                                   (looking-at-p re))))
                   ;; Previous sexp was a string, restore point.
                   (goto-char backward-sexp-point)
                   (cl-incf counter))
diff --git a/test/lisp/progmodes/python-tests.el b/test/lisp/progmodes/python-tests.el
index 50153e66da5..cbaf5b698bd 100644
--- a/test/lisp/progmodes/python-tests.el
+++ b/test/lisp/progmodes/python-tests.el
@@ -729,6 +729,7 @@ python-font-lock-escape-sequence-multiline-string
      (845 . font-lock-string-face) (886))))
 
 (ert-deftest python-font-lock-escape-sequence-bytes-newline ()
+  :expected-result :failed
   (python-tests-assert-faces
    "b'\\n'
 b\"\\n\""
@@ -741,6 +742,7 @@ python-font-lock-escape-sequence-bytes-newline
      (11 . font-lock-doc-face))))
 
 (ert-deftest python-font-lock-escape-sequence-hex-octal ()
+  :expected-result :failed
   (python-tests-assert-faces
    "b'\\x12 \\777 \\1\\23'
 '\\x12 \\777 \\1\\23'"
@@ -761,6 +763,7 @@ python-font-lock-escape-sequence-hex-octal
      (36 . font-lock-doc-face))))
 
 (ert-deftest python-font-lock-escape-sequence-unicode ()
+  :expected-result :failed
   (python-tests-assert-faces
    "b'\\u1234 \\U00010348 \\N{Plus-Minus Sign}'
 '\\u1234 \\U00010348 \\N{Plus-Minus Sign}'"
@@ -775,6 +778,7 @@ python-font-lock-escape-sequence-unicode
      (80 . font-lock-doc-face))))
 
 (ert-deftest python-font-lock-raw-escape-sequence ()
+  :expected-result :failed
   (python-tests-assert-faces
    "rb'\\x12 \123 \\n'
 r'\\x12 \123 \\n \\u1234 \\U00010348 \\N{Plus-Minus Sign}'"
@@ -6598,6 +6602,18 @@ python-info-docstring-p-6
    (python-tests-look-at "'''Not a method docstring.'''")
    (should (not (python-info-docstring-p)))))
 
+(ert-deftest python-info-docstring-p-7 ()
+  "Test string in a dictionary."
+  (python-tests-with-temp-buffer
+   "
+{'Not a docstring': 1}
+'Also not a docstring'
+"
+   (python-tests-look-at "Not a docstring")
+   (should-not (python-info-docstring-p))
+   (python-tests-look-at "Also not a docstring")
+   (should-not (python-info-docstring-p))))
+
 (ert-deftest python-info-triple-quoted-string-p-1 ()
   "Test triple quoted string."
   (python-tests-with-temp-buffer
-- 
2.34.1


[-- Attachment #3: 0002-Use-font-lock-extend-region-functions-in-python-mode.patch --]
[-- Type: application/octet-stream, Size: 2541 bytes --]

From f166993f4874d28d2f533bfbc3df1c7ca8d3aa2e Mon Sep 17 00:00:00 2001
From: kobarity <kobarity@gmail.com>
Date: Wed, 24 May 2023 22:06:51 +0900
Subject: [PATCH 2/2] Use font-lock-extend-region-functions in python-mode

* lisp/progmodes/python.el (python-font-lock-extend-region): Change
arguments and return value for font-lock-extend-region-functions.
(python-mode): Change from
font-lock-extend-after-change-region-function to
font-lock-extend-region-functions.  (Bug#63622)
---
 lisp/progmodes/python.el | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 032a17c52ff..adaeacc2ec1 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -869,18 +869,22 @@ python-font-lock-keywords
 Which one will be chosen depends on the value of
 `font-lock-maximum-decoration'.")
 
-(defun python-font-lock-extend-region (beg end _old-len)
-  "Extend font-lock region given by BEG and END to statement boundaries."
-  (save-excursion
-    (save-match-data
-      (goto-char beg)
-      (python-nav-beginning-of-statement)
-      (setq beg (point))
-      (goto-char end)
-      (python-nav-end-of-statement)
-      (setq end (point))
-      (cons beg end))))
-
+(defvar font-lock-beg)
+(defvar font-lock-end)
+(defun python-font-lock-extend-region ()
+  "Extend font-lock region to statement boundaries."
+  (let ((beg font-lock-beg)
+        (end font-lock-end))
+    (goto-char beg)
+    (python-nav-beginning-of-statement)
+    (beginning-of-line)
+    (when (< (point) beg)
+      (setq font-lock-beg (point)))
+    (goto-char end)
+    (python-nav-end-of-statement)
+    (when (< end (point))
+      (setq font-lock-end (point)))
+    (or (/= beg font-lock-beg) (/= end font-lock-end))))
 
 (defconst python-syntax-propertize-function
   (syntax-propertize-rules
@@ -6704,9 +6708,9 @@ python-mode
               `(,python-font-lock-keywords
                 nil nil nil nil
                 (font-lock-syntactic-face-function
-                 . python-font-lock-syntactic-face-function)
-                (font-lock-extend-after-change-region-function
-                 . python-font-lock-extend-region)))
+                 . python-font-lock-syntactic-face-function)))
+  (add-hook 'font-lock-extend-region-functions
+            #'python-font-lock-extend-region nil t)
   (setq-local syntax-propertize-function
               python-syntax-propertize-function)
   (setq-local imenu-create-index-function
-- 
2.34.1

bug#63622: lisp/progmodes/python.el: performance regression introduced by multiline font-lock

[Eli Zaretskii]

> Date: Thu, 25 May 2023 00:05:09 +0900
> From: kobarity <kobarity@gmail.com>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
> 	Tom Gillespie <tgbugs@gmail.com>,
> 	Eli Zaretskii <eliz@gnu.org>,
> 	63622@debbugs.gnu.org
> 
> Attached is a series of patches that replace the previous patches.

Thanks, installed on the emacs-29 branch, and closing the bug.

bug#63622: lisp/progmodes/python.el: performance regression introduced by multiline font-lock

[kobarity]

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


I wrote:
> I agree.  It seems to me that it is not python-font-lock-extend-region
> itself that is slow, but rather font-lock's processing of the area
> extended by it.  So one workaround would be to limit the number of
> lines to be extended, as in the attached patch.  If this limit is
> exceeded, the area or the entire buffer must be font-locked manually
> later.  What do you think of this idea?

The cause of the slowdown seems to be in python-info-docstring-p.  So
another option would be to improve it.  Attached is a patch that
determines that if point is in parens except for "(", it is not a
docstring.  I can't think of a case where a docstring is in parens
except for "(".  However, usually more tests should be done.

[-- Attachment #2: 0001-Optimize-python-info-docstring-p.patch --]
[-- Type: application/octet-stream, Size: 1026 bytes --]

From 749afdc3cfe781f785c95501aa58472c78b71bf7 Mon Sep 17 00:00:00 2001
From: kobarity <kobarity@gmail.com>
Date: Mon, 22 May 2023 00:01:16 +0900
Subject: [PATCH] Optimize python-info-docstring-p

* lisp/progmodes/python.el (python-info-docstring-p): Add condition
that is not inside paren except for "(".  (Bug#63622)
---
 lisp/progmodes/python.el | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 6fc05b246a6..af48d852182 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -6021,6 +6021,9 @@ python-info-docstring-p
           (re (concat "[uU]?[rR]?"
                       (python-rx string-delimiter))))
       (when (and
+             (not (and syntax-ppss
+                       (when-let ((pos (nth 1 syntax-ppss)))
+                         (/= (char-after pos) ?\())))
              (not (python-info-assignment-statement-p))
              (looking-at-p re)
              ;; Allow up to two consecutive docstrings only.
-- 
2.34.1