bug#64391: 30.0.50; buffer narrowing slowdown regression in emacs 29

bug#64391: 30.0.50; buffer narrowing slowdown regression in emacs 29

[Andrew Cohen]

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


I have noticed a huge slowdown in parsing email headers in gnus. After
some debugging with help from Mattias Engdegård, the problem has been
traced to the narrowing code introduced in

commit ba9315b1641b483f2bf843c38dcdba0cd1643a55 (HEAD)
Merge: aef803d6c3d a3b654e069e
Author: Gregory Heytings <gregory@heytings.org>
Date:   Thu Nov 24 14:21:30 2022 +0100

    Merge master into feature/improved-locked-narrowing.

Gnus populates a buffer with headers from a set of email messages and
then parses them by: narrowing the buffer to the headers for an
individual message; parsing the headers; widening; and then repeating
for the next message.  As part of the parsing the headers are "unfolded"
so that each header doesn't include line breaks. I noticed that for a
long list of messages (10,000) this takes between one and two orders of
magnitude more time in Emacs 30 than in Emacs 28. Unfolding all the
headers in the full buffer before the parsing process removes most of
the slowdown. The slowdown seems to grow quadratically with the size of
the buffer.

The problem seems fairly general and Mattias has produced a simple test
case to demonstrate the issue (code at the end of this message):

1. Create a buffer with 100,000 lines each containing two characters  "ab".
2. Loop through the buffer narrowing to each line, and immediately widening
    back to the full buffer (so no change is made to the buffer
    contents).
3.  Loop through the buffer removing the first character of each line.

This takes a very long time compared with reversing the order of 2. and 3.

Emacs 28.2
Modify after narrowing:  0.173 s elapsed, 0 GCs, 0.000 s GC, 0.173 s non-GC
Modify before narrowing: 0.160 s elapsed, 0 GCs, 0.000 s GC, 0.160 s non-GC

Emacs 29.0.92
Modify after narrowing:  4.454 s elapsed, 9 GCs, 0.115 s GC, 4.339 s non-GC
Modify before narrowing: 0.291 s elapsed, 9 GCs, 0.114 s GC, 0.177 s non-GC

The problem may be the creation of markers in the narrowing process that
slows down further mutation. (Also note that the new code has introduced
some significant GC that was previously absent).


[-- Attachment #2: Demonstrate narrowing slowdown --]
[-- Type: application/emacs-lisp, Size: 1595 bytes --]

[-- Attachment #3: Type: text/plain, Size: 20 bytes --]




-- 
Andrew Cohen

bug#64391: bug 64391 wrong git info

[Andrew Cohen]

Sorry, I didn't give the right git commit info.

The last GOOD commit was

Commit ba9315b1641b483f2bf843c38dcdba0cd1643a55
Emacs 29.0.50
Modify after narrowing:  0.080 s elapsed, 0 GCs, 0.000 s GC, 0.080 s non-GC
Modify before narrowing: 0.073 s elapsed, 0 GCs, 0.000 s GC, 0.073 s non-GC

The first BAD commit showed a slowdown of a significant factor
Commit 9dee6df39cd14be78ff96cb24169842f4772488a
Emacs 29.0.50
Modify after narrowing:  0.734 s elapsed, 9 GCs, 0.051 s GC, 0.684 s non-GC
Modify before narrowing: 0.153 s elapsed, 9 GCs, 0.052 s GC, 0.102 s non-GC

Then a further order of magnitude slowdown shows up around
 Commit 321d4e61551a0f6dfb1abfc0b54e6177735bde58
Modify after narrowing:  7.954 s elapsed, 3 GCs, 0.022 s GC, 7.932 s non-GC
Modify before narrowing: 0.122 s elapsed, 3 GCs, 0.023 s GC, 0.099 s non-GC


Hope this info is helpful.
-- 
Andrew Cohen

bug#64391: more info

[Andrew Cohen]

Mattias had flagged this code at line 2948 in editfns.c as a likely culprit. 

/* Record the accessible range of the buffer when narrow-to-region
     is called, that is, before applying the narrowing.  That
     information is used only by internal--label-restriction.  */
  Fset (Qoutermost_restriction, list3 (Qoutermost_restriction,
				       Fpoint_min_marker (),
				       Fpoint_max_marker ()));

Just for fun I simply commented it out (since the test code doesn't use
labeled restrictions) and ran the test code, and the problem disappears:

Emacs 30.0.50
Modify after narrowing:  0.076 s elapsed, 0 GCs, 0.000 s GC, 0.076 s non-GC
Modify before narrowing: 0.070 s elapsed, 0 GCs, 0.000 s GC, 0.070 s non-GC
-- 
Andrew Cohen

bug#64391: 30.0.50; buffer narrowing slowdown regression in emacs 29

[Eli Zaretskii]

> Cc: Gregory Heytings <gregory@heytings.org>
> From: Andrew Cohen <acohen@ust.hk>
> Date: Sat, 01 Jul 2023 08:15:25 +0800
> 
> I have noticed a huge slowdown in parsing email headers in gnus. After
> some debugging with help from Mattias Engdegård, the problem has been
> traced to the narrowing code introduced in
> 
> commit ba9315b1641b483f2bf843c38dcdba0cd1643a55 (HEAD)
> Merge: aef803d6c3d a3b654e069e
> Author: Gregory Heytings <gregory@heytings.org>
> Date:   Thu Nov 24 14:21:30 2022 +0100
> 
>     Merge master into feature/improved-locked-narrowing.
> 
> Gnus populates a buffer with headers from a set of email messages and
> then parses them by: narrowing the buffer to the headers for an
> individual message; parsing the headers; widening; and then repeating
> for the next message.  As part of the parsing the headers are "unfolded"
> so that each header doesn't include line breaks. I noticed that for a
> long list of messages (10,000) this takes between one and two orders of
> magnitude more time in Emacs 30 than in Emacs 28. Unfolding all the
> headers in the full buffer before the parsing process removes most of
> the slowdown. The slowdown seems to grow quadratically with the size of
> the buffer.
> 
> The problem seems fairly general and Mattias has produced a simple test
> case to demonstrate the issue (code at the end of this message):
> 
> 1. Create a buffer with 100,000 lines each containing two characters  "ab".
> 2. Loop through the buffer narrowing to each line, and immediately widening
>     back to the full buffer (so no change is made to the buffer
>     contents).
> 3.  Loop through the buffer removing the first character of each line.
> 
> This takes a very long time compared with reversing the order of 2. and 3.

Does this problem go away if you set long-line-threshold to nil?

Can you show a profile of the processing that takes a long time?

bug#64391: 30.0.50; buffer narrowing slowdown regression in emacs 29

[Andrew Cohen]

>>>>> "EZ" == Eli Zaretskii <eliz@gnu.org> writes:

[...]

    EZ> Does this problem go away if you set long-line-threshold to nil?

No.

    EZ> Can you show a profile of the processing that takes a long time?

Not exactly sure how to answer this. The test case just goes through the
buffer narrowing and widening (without modifying the buffer).  This
takes very little time. Then the following takes a very  long time:

      (goto-char (point-min))
      (while (not (eobp))
        (delete-char 1)
        (forward-line))

-- 
Andrew Cohen

bug#64391: 30.0.50; buffer narrowing slowdown regression in emacs 29

[Eli Zaretskii]

> From: Andrew Cohen <acohen@ust.hk>
> Cc: 64391@debbugs.gnu.org,  gregory@heytings.org
> Date: Sat, 01 Jul 2023 15:31:53 +0800
> 
> >>>>> "EZ" == Eli Zaretskii <eliz@gnu.org> writes:
> 
>     EZ> Can you show a profile of the processing that takes a long time?
> 
> Not exactly sure how to answer this. The test case just goes through the
> buffer narrowing and widening (without modifying the buffer).  This
> takes very little time. Then the following takes a very  long time:
> 
>       (goto-char (point-min))
>       (while (not (eobp))
>         (delete-char 1)
>         (forward-line))

I asked for the profile produce by "M-x profiler-start RET RET" for
this very long processing.  is it feasible to produce such a profile?

bug#64391: buffer narrowing slowdown regression in emacs 29

[Mattias Engdegård]

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

1 juli 2023 kl. 06.59 skrev Andrew Cohen <acohen@ust.hk>:

> Just for fun I simply commented it out (since the test code doesn't use
> labeled restrictions) and ran the test code, and the problem disappears:

The attached patch combines narrow-to-region and internal--label-restriction into a single function, internal--narrow-to-region. (We could also add the label as an optional argument to narrow-to-region.)

As a result, the up-front consing and marker allocation disappear entirely for normal (unlabelled) narrowing, as does the expensive buffer-local `outermost-restriction` variable.

Labelled narrowing is almost as expensive as before but is, we hope, less common. Maybe we can work separately on reducing its cost.

(And to those reading the patch: yes, some work on doc strings is needed. Suggestions welcome.)


[-- Attachment #2: narrow-to-region-with-label.diff --]
[-- Type: application/octet-stream, Size: 5509 bytes --]

diff --git a/lisp/subr.el b/lisp/subr.el
index 4c462830120..37c9a422e7f 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -4077,8 +4077,7 @@ with-restriction
 (defun internal--with-restriction (start end body &optional label)
   "Helper function for `with-restriction', which see."
   (save-restriction
-    (narrow-to-region start end)
-    (if label (internal--label-restriction label))
+    (internal--narrow-to-region start end label)
     (funcall body)))
 
 (defmacro without-restriction (&rest rest)
diff --git a/src/editfns.c b/src/editfns.c
index b920857b664..c434fbd4d39 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -2682,7 +2682,7 @@ DEFUN ("delete-and-extract-region", Fdelete_and_extract_region,
    records the restriction bounds that were current when the first
    labeled restriction was entered (which may be a narrowing that was
    set by the user and is visible on display).  This alist is used
-   internally by narrow-to-region, widen, internal--label-restriction,
+   internally by internal--narrow-to-region, widen,
    internal--unlabel-restriction and save-restriction.  For efficiency
    reasons, an alist is used instead of a buffer-local variable:
    otherwise reset_outermost_restrictions, which is called during each
@@ -2868,8 +2868,7 @@ unwind_labeled_narrow_to_region (Lisp_Object label)
 labeled_narrow_to_region (Lisp_Object begv, Lisp_Object zv,
 			  Lisp_Object label)
 {
-  Fnarrow_to_region (begv, zv);
-  Finternal__label_restriction (label);
+  Finternal__narrow_to_region (begv, zv, label);
   record_unwind_protect (restore_point_unwind, Fpoint_marker ());
   record_unwind_protect (unwind_labeled_narrow_to_region, label);
 }
@@ -2885,7 +2884,6 @@ DEFUN ("widen", Fwiden, Swiden, 0, 0, "",
 `without-restriction' with the same label.  */)
   (void)
 {
-  Fset (Qoutermost_restriction, Qnil);
   Lisp_Object buf = Fcurrent_buffer ();
   Lisp_Object label = labeled_restrictions_peek_label (buf);
 
@@ -2940,6 +2938,15 @@ positions (integers or markers) bounding the text that should
 argument.  To gain access to other portions of the buffer, use
 `without-restriction' with the same label.  */)
   (Lisp_Object start, Lisp_Object end)
+{
+  return Finternal__narrow_to_region (start, end, Qnil);
+}
+
+DEFUN ("internal--narrow-to-region", Finternal__narrow_to_region,
+       Sinternal__narrow_to_region, 3, 3, 0,
+       doc: /* Labelled restriction of editing to a region.
+Internal use only.  */)
+  (Lisp_Object start, Lisp_Object end, Lisp_Object label)
 {
   EMACS_INT s = fix_position (start), e = fix_position (end);
 
@@ -2968,11 +2975,11 @@ positions (integers or markers) bounding the text that should
     }
 
   /* Record the accessible range of the buffer when narrow-to-region
-     is called, that is, before applying the narrowing.  That
-     information is used only by internal--label-restriction.  */
-  Fset (Qoutermost_restriction, list3 (Qoutermost_restriction,
-				       Fpoint_min_marker (),
-				       Fpoint_max_marker ()));
+     is called, that is, before applying the narrowing.  */
+  EMACS_INT begv = BEGV;
+  EMACS_INT begv_byte = BEGV_BYTE;
+  EMACS_INT zv = ZV;
+  EMACS_INT zv_byte = ZV_BYTE;
 
   if (BEGV != s || ZV != e)
     current_buffer->clip_changed = 1;
@@ -2986,28 +2993,21 @@ positions (integers or markers) bounding the text that should
     SET_PT (e);
   /* Changing the buffer bounds invalidates any recorded current column.  */
   invalidate_current_column ();
-  return Qnil;
-}
 
-DEFUN ("internal--label-restriction", Finternal__label_restriction,
-       Sinternal__label_restriction, 1, 1, 0,
-       doc: /* Label the current restriction with LABEL.
+  if (!NILP (label))
+    {
+      if (NILP (labeled_restrictions_peek_label (buf)))
+	labeled_restrictions_push (buf,
+				   list3 (Qoutermost_restriction,
+					  build_marker (current_buffer,
+							begv, begv_byte),
+					  build_marker (current_buffer,
+							zv, zv_byte)));
+      labeled_restrictions_push (buf, list3 (label,
+					     Fpoint_min_marker (),
+					     Fpoint_max_marker ()));
+    }
 
-This is an internal function used by `with-restriction'.  */)
-  (Lisp_Object label)
-{
-  Lisp_Object buf = Fcurrent_buffer ();
-  Lisp_Object outermost_restriction
-    = buffer_local_value (Qoutermost_restriction, buf);
-  /* If internal--label-restriction is ever called without being
-     preceded by narrow-to-region, do nothing.  */
-  if (NILP (outermost_restriction))
-    return Qnil;
-  if (NILP (labeled_restrictions_peek_label (buf)))
-    labeled_restrictions_push (buf, outermost_restriction);
-  labeled_restrictions_push (buf, list3 (label,
-					 Fpoint_min_marker (),
-					 Fpoint_max_marker ()));
   return Qnil;
 }
 
@@ -4865,10 +4865,6 @@ syms_of_editfns (void)
 it to be non-nil.  */);
   binary_as_unsigned = false;
 
-  DEFVAR_LISP ("outermost-restriction", Voutermost_restriction,
-	       doc: /* Outermost narrowing bounds, if any.  Internal use only.  */);
-  Voutermost_restriction = Qnil;
-  Fmake_variable_buffer_local (Qoutermost_restriction);
   DEFSYM (Qoutermost_restriction, "outermost-restriction");
   Funintern (Qoutermost_restriction, Qnil);
 
@@ -4963,7 +4959,7 @@ syms_of_editfns (void)
   defsubr (&Sdelete_and_extract_region);
   defsubr (&Swiden);
   defsubr (&Snarrow_to_region);
-  defsubr (&Sinternal__label_restriction);
+  defsubr (&Sinternal__narrow_to_region);
   defsubr (&Sinternal__unlabel_restriction);
   defsubr (&Ssave_restriction);
   defsubr (&Stranspose_regions);

bug#64391: buffer narrowing slowdown regression in emacs 29

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Sat, 1 Jul 2023 13:37:08 +0200
> Cc: 64391@debbugs.gnu.org,
>  Gregory Heytings <gregory@heytings.org>,
>  Eli Zaretskii <eliz@gnu.org>
> 
> 1 juli 2023 kl. 06.59 skrev Andrew Cohen <acohen@ust.hk>:
> 
> > Just for fun I simply commented it out (since the test code doesn't use
> > labeled restrictions) and ran the test code, and the problem disappears:
> 
> The attached patch combines narrow-to-region and internal--label-restriction into a single function, internal--narrow-to-region. (We could also add the label as an optional argument to narrow-to-region.)

It does more than that, so I'd appreciate a more detailed description
of the changes and their rationale.

Thanks.

bug#64391: buffer narrowing slowdown regression in emacs 29

[Mattias Engdegård]

1 juli 2023 kl. 14.08 skrev Eli Zaretskii <eliz@gnu.org>:

>> The attached patch combines narrow-to-region and internal--label-restriction into a single function, internal--narrow-to-region. (We could also add the label as an optional argument to narrow-to-region.)
> 
> It does more than that, so I'd appreciate a more detailed description
> of the changes and their rationale.

Actually that's just what it does. Here is a tentative commit message:

    Fix severe narrowing performance bug (regression from emacs 28)
    
    In Emacs 29, `narrow-to-region` conses and creates markers
    unconditionally on the offchance that a call to
    `internal--label-restriction` would need it, which is only rarely the
    case (in `with-restriction` with a :label argument).
    
    As a remedy we fuse the two functions to one,
    `internal--narrow-to-region`, and only perform the costly consing and
    marker creation for labelled narrowing. (Bug#64391)
    
    * lisp/subr.el (internal--with-restriction):
    * src/editfns.c (labeled_narrow_to_region):
    Call `internal--narrow-to-region` instead of `narrow-to-region`
    followed by `internal--label-restriction`.
    (Fwiden): Remove assignment to eliminated `outermost-restriction`.
    (Fnarrow_to_region): Reduce to a stub that calls the original
    function, now named...
    (Finternal__narrow_to_region): ...this, which takes an added
    `label` argument and includes at the end the optimised body of...
    (Finternal__label_restriction): ...this function, now removed
    since it has been entirely absorbed.
    (syms_of_editfns): Remove the `outermost-restriction` buffer-local
    variable as it was only used to convey data from `narrow-to-region`
    to `internal--label-restriction` called immediately afterwards.

And again, if anyone would prefer an optional `label` argument to `narrow-to-region` then that would be fine too. It depends a little on whether we want to expose that functionality to the user in that function, or as now keep it in `with-restriction` only.

bug#64391: buffer narrowing slowdown regression in emacs 29

[Gregory Heytings]

Thanks Andrew for the bug report, and Mattias for the patch.

I'll have a look in a few hours.

bug#64391: buffer narrowing slowdown regression in emacs 29

[Mattias Engdegård]

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

How expensive markers can be when a buffer is modified is often unappreciated. Here are the results of a benchmark to illustrate the cost (code attached).
A buffer contains "abababab...", we optionally add a marker to every other position, and then time the deletion of all the "a"s. Results:

|   size | without markers | with markers |
|--------+-----------------+--------------|
|    500 |           0.000 |        0.001 |
|   1000 |           0.001 |        0.004 |
|   5000 |           0.002 |        0.089 |
|  10000 |           0.005 |        0.363 |
|  50000 |           0.024 |        9.527 |
| 100000 |           0.049 |       58.048 |

Now there's something for the curve-fitters among you.


[-- Attachment #2: abench4.el --]
[-- Type: application/octet-stream, Size: 1519 bytes --]

;;; -*- lexical-binding: t -*-

(require 'cl-lib)

(defun abench-run ()
  (let ((buffer-undo-list t))
    (goto-char (point-min))
    (while (not (eobp))
      (delete-char 1)
      (forward-char))))

(defun abench (size use-markers)
  (with-temp-buffer
    ;; Populate buffer
    (insert (mapconcat #'identity (make-list size "ab") nil))
    ;; Insert markers
    (let ((markers nil))
      (when use-markers
        (dotimes (i size)
          (push (set-marker (make-marker) (* i 2)) markers)))
      ;; Run operation
      (garbage-collect)
      (let ((gcs0 gcs-done)
            (gc-e0 gc-elapsed)
            (t0 (float-time)))
        (abench-run)
        (let ((dt (- (float-time) t0))
              (gcs (- gcs-done gcs0))
              (gc-time (- gc-elapsed gc-e0)))
          (cl-assert
           (equal (buffer-string)
                  (mapconcat #'identity (make-list size "b") nil)))
          (list dt gcs gc-time))))))

(defun abench-main ()
  (princ (format "Emacs %s\n" emacs-version))
  (princ (format "| size | without markers | with markers |\n"))
  (princ (format "|------+-----------------+--------------|\n"))
  (dolist (size '(500 1000 5000 10000 50000 100000))
    (let ((res-without (abench size nil))
          (res-with (abench size t)))
        (princ (format "| %d | %.3f | %.3f |\n"
                       size
                       (nth 0 res-without)
                       (nth 0 res-with)))))
  (princ (format "|------+-----------------+--------------|\n"))
  )

(abench-main)

bug#64391: buffer narrowing slowdown regression in emacs 29

[Eli Zaretskii]

> Date: Sun, 02 Jul 2023 07:35:13 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: Eli Zaretskii <eliz@gnu.org>, 64391@debbugs.gnu.org, 
>     Stefan Monnier <monnier@iro.umontreal.ca>
> 
> 
> Thanks Andrew for the bug report, and Mattias for the patch.
> 
> I'll have a look in a few hours.

Any progress there?

bug#64391: buffer narrowing slowdown regression in emacs 29

[Gregory Heytings]

>> Thanks Andrew for the bug report, and Mattias for the patch.
>>
>> I'll have a look in a few hours.
>
> Any progress there?
>

Sorry for the delay, I'm looking at this right now.

bug#64391: buffer narrowing slowdown regression in emacs 29

[Gregory Heytings]

I pushed a proposed change to the scratch/bug64391 branch.

It contains three commits:

- b741dc7fcd which is the minimal possible commit to fix this bug

- 01fb898420 which moves a few statements from the callee 
(internal--label-restriction) to its only caller 
(internal--labeled-narrow-to-region), and simplifies the code accordingly

- c52ade305e which is optional, and makes the symmetrical change for the 
widening case

Eli and Mattias, could you perhaps have a look?

Thanks.

bug#64391: buffer narrowing slowdown regression in emacs 29

[Eli Zaretskii]

> Date: Thu, 06 Jul 2023 17:33:13 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: acohen@ust.hk, 64391@debbugs.gnu.org, mattias.engdegard@gmail.com, 
>     monnier@iro.umontreal.ca
> 
> 
> I pushed a proposed change to the scratch/bug64391 branch.
> 
> It contains three commits:
> 
> - b741dc7fcd which is the minimal possible commit to fix this bug
> 
> - 01fb898420 which moves a few statements from the callee 
> (internal--label-restriction) to its only caller 
> (internal--labeled-narrow-to-region), and simplifies the code accordingly
> 
> - c52ade305e which is optional, and makes the symmetrical change for the 
> widening case
> 
> Eli and Mattias, could you perhaps have a look?

How are those different from what Mattias proposed?  Is this a
completely different set of changes, or is it what Mattias suggested,
just in several separate parts?

If these are different, did you time the results in the use case(s)
where the slow-down was detected and reported, and compared the two
proposals performance-wise?

bug#64391: buffer narrowing slowdown regression in emacs 29

[Gregory Heytings]

>> I pushed a proposed change to the scratch/bug64391 branch.
>>
>> It contains three commits:
>>
>> - b741dc7fcd which is the minimal possible commit to fix this bug
>>
>> - 01fb898420 which moves a few statements from the callee 
>> (internal--label-restriction) to its only caller 
>> (internal--labeled-narrow-to-region), and simplifies the code 
>> accordingly
>>
>> - c52ade305e which is optional, and makes the symmetrical change for 
>> the widening case
>>
>> Eli and Mattias, could you perhaps have a look?
>
> How are those different from what Mattias proposed?  Is this a 
> completely different set of changes, or is it what Mattias suggested, 
> just in several separate parts?
>

It resembles what Mattias proposed of course (remove the negative effect 
of setting a buffer-local variable in all calls to narrow-to-region, and 
limit it to case when labeled narrowing is used), but it's nonetheless 
different.  The essential change is isolated in the first commit, and in 
the second commit, instead of moving the body of Fnarrow_to_region to 
another function with an additional (third) argument, I use another 
separate function which calls Fnarrow_to_region.  I believe the result is 
clearer.

>
> If these are different, did you time the results in the use case(s) 
> where the slow-down was detected and reported, and compared the two 
> proposals performance-wise?
>

Yes, there is no difference, performance-wise:

- with Mattias' patch:

Modify after narrowing:  0.114 s elapsed, 0 GCs, 0.000 s GC, 0.114 s non-GC
Modify before narrowing: 0.103 s elapsed, 0 GCs, 0.000 s GC, 0.103 s non-GC

- with the code in scratch/bug64391:

Modify after narrowing:  0.109 s elapsed, 0 GCs, 0.000 s GC, 0.109 s non-GC
Modify before narrowing: 0.102 s elapsed, 0 GCs, 0.000 s GC, 0.102 s non-GC

(The small differences are not meaningful, other runs show different 
values in the same order of magnitude.)

bug#64391: buffer narrowing slowdown regression in emacs 29

[Mattias Engdegård]

6 juli 2023 kl. 20.45 skrev Gregory Heytings <gregory@heytings.org>:

>> How are those different from what Mattias proposed?  Is this a completely different set of changes, or is it what Mattias suggested, just in several separate parts?
>> 
> 
> It resembles what Mattias proposed of course (remove the negative effect of setting a buffer-local variable in all calls to narrow-to-region, and limit it to case when labeled narrowing is used), but it's nonetheless different.  The essential change is isolated in the first commit, and in the second commit, instead of moving the body of Fnarrow_to_region to another function with an additional (third) argument, I use another separate function which calls Fnarrow_to_region.  I believe the result is clearer.

Gregory's first two patches and mine are indeed equivalent up to refactoring; either would do. The moving parts are the same, so performance shouldn't differ noticeably. Neither appears to tie our hands for future changes as the interfaces are all internal.

The third of Gregory's patches appears mainly cosmetic. Maybe that one is less important for emacs-29.

I have one question about the new `with-restriction` macro: if it is intended as a convenience macro for unlabelled narrowing as well, shouldn't it then expand to the obvious

  (save-restriction
    (narrow-to-region BEG END)
    BODY)

instead the much more expensive call to a helper function?

The documentation should also make it clear that `nil` is not a valid label.

bug#64391: buffer narrowing slowdown regression in emacs 29

[Gregory Heytings]

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


Thanks for your feedback!

>
> I have one question about the new `with-restriction` macro: if it is 
> intended as a convenience macro for unlabelled narrowing as well, 
> shouldn't it then expand to the obvious
>
>  (save-restriction
>    (narrow-to-region BEG END)
>    BODY)
>
> instead the much more expensive call to a helper function?
>

Yes.  In fact now that we have a separate function for the labeled case, 
we can remove the funcall in both cases.  Patch attached.

>
> The documentation should also make it clear that `nil` is not a valid 
> label.
>

Is that not already clear in the docstring, which says "in which LABEL is 
a symbol"?

[-- Attachment #2: Simplify-with-restriction-and-without-restriction.patch --]
[-- Type: text/x-diff, Size: 2439 bytes --]

From 4897d6ade80e9928b7f25d0fb6f970c4975c43bd Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Fri, 7 Jul 2023 09:54:32 +0000
Subject: [PATCH] Simplify 'with-restriction' and 'without-restriction'

* lisp/subr.el (with-restriction, without-restriction): Merge the
bodies of the 'internal--with-restriction' and
'internal--without-restriction' function into the macros.  The
result is more efficient than a funcall.
(internal--with-restriction, internal--without-restriction):
Remove.
---
 lisp/subr.el | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index 85adef5b689..42a6cfec514 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3973,16 +3973,10 @@ with-restriction
 \(fn START END [:label LABEL] BODY)"
   (declare (indent 2) (debug t))
   (if (eq (car rest) :label)
-      `(internal--with-restriction ,start ,end (lambda () ,@(cddr rest))
-                                 ,(cadr rest))
-    `(internal--with-restriction ,start ,end (lambda () ,@rest))))
-
-(defun internal--with-restriction (start end body &optional label)
-  "Helper function for `with-restriction', which see."
-  (save-restriction
-    (narrow-to-region start end)
-    (if label (internal--label-restriction label))
-    (funcall body)))
+      `(save-restriction
+         (internal--labeled-narrow-to-region ,start ,end ,(cadr rest))
+         ,@(cddr rest))
+    `(save-restriction (narrow-to-region ,start ,end) ,@rest)))
 
 (defmacro without-restriction (&rest rest)
   "Execute BODY without restrictions.
@@ -3996,16 +3990,8 @@ without-restriction
 \(fn [:label LABEL] BODY)"
   (declare (indent 0) (debug t))
   (if (eq (car rest) :label)
-      `(internal--without-restriction (lambda () ,@(cddr rest))
-                                    ,(cadr rest))
-    `(internal--without-restriction (lambda () ,@rest))))
-
-(defun internal--without-restriction (body &optional label)
-  "Helper function for `without-restriction', which see."
-  (save-restriction
-    (if label (internal--unlabel-restriction label))
-    (widen)
-    (funcall body)))
+      `(save-restriction (internal--labeled-widen ,(cadr rest)) ,@(cddr rest))
+    `(save-restriction (widen) ,@rest)))
 
 (defun find-tag-default-bounds ()
   "Determine the boundaries of the default tag, based on text at point.
-- 
2.39.2

bug#64391: buffer narrowing slowdown regression in emacs 29

[Eli Zaretskii]

> Date: Fri, 07 Jul 2023 10:00:48 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: acohen@ust.hk, 64391@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>, 
>     monnier@iro.umontreal.ca, bugs@gnus.org
> 
> > I have one question about the new `with-restriction` macro: if it is 
> > intended as a convenience macro for unlabelled narrowing as well, 
> > shouldn't it then expand to the obvious
> >
> >  (save-restriction
> >    (narrow-to-region BEG END)
> >    BODY)
> >
> > instead the much more expensive call to a helper function?
> 
> Yes.  In fact now that we have a separate function for the labeled case, 
> we can remove the funcall in both cases.  Patch attached.

You are killing me.

bug#64391: buffer narrowing slowdown regression in emacs 29

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Fri, 7 Jul 2023 11:30:11 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>,
>  acohen@ust.hk,
>  64391@debbugs.gnu.org,
>  monnier@iro.umontreal.ca
> 
> Gregory's first two patches and mine are indeed equivalent up to refactoring; either would do. The moving parts are the same, so performance shouldn't differ noticeably. Neither appears to tie our hands for future changes as the interfaces are all internal.

I was considering applying only the first of Gregory's patches,
without the second.  WDYT?

bug#64391: buffer narrowing slowdown regression in emacs 29

[Gregory Heytings]

>>> I have one question about the new `with-restriction` macro: if it is 
>>> intended as a convenience macro for unlabelled narrowing as well, 
>>> shouldn't it then expand to the obvious
>>>
>>>  (save-restriction
>>>    (narrow-to-region BEG END)
>>>    BODY)
>>>
>>> instead the much more expensive call to a helper function?
>>
>> Yes.  In fact now that we have a separate function for the labeled 
>> case, we can remove the funcall in both cases.  Patch attached.
>
> You are killing me.
>

???  That wasn't meant for the release branch.

bug#64391: buffer narrowing slowdown regression in emacs 29

[Mattias Engdegård]

7 juli 2023 kl. 12.27 skrev Eli Zaretskii <eliz@gnu.org>:

> I was considering applying only the first of Gregory's patches,
> without the second.  WDYT?

I don't think that makes much sense -- the division between the two is pretty much artificial.

The third patch can safely be left out in emacs-29.

bug#64391: buffer narrowing slowdown regression in emacs 29

[Mattias Engdegård]

7 juli 2023 kl. 12.00 skrev Gregory Heytings <gregory@heytings.org>:

> Yes.  In fact now that we have a separate function for the labeled case, we can remove the funcall in both cases.  Patch attached.

Thank you, but are we sure that save-restriction would be used for labelled restrictions in the future? Maybe we should just start with the unlabelled case.

>> The documentation should also make it clear that `nil` is not a valid label.
> 
> Is that not already clear in the docstring, which says "in which LABEL is a symbol"?

Well, nil is a symbol.

bug#64391: buffer narrowing slowdown regression in emacs 29

[Eli Zaretskii]

> Date: Fri, 07 Jul 2023 10:31:01 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: acohen@ust.hk, 64391@debbugs.gnu.org, mattias.engdegard@gmail.com, 
>     monnier@iro.umontreal.ca, bugs@gnus.org
> 
> 
> >>> I have one question about the new `with-restriction` macro: if it is 
> >>> intended as a convenience macro for unlabelled narrowing as well, 
> >>> shouldn't it then expand to the obvious
> >>>
> >>>  (save-restriction
> >>>    (narrow-to-region BEG END)
> >>>    BODY)
> >>>
> >>> instead the much more expensive call to a helper function?
> >>
> >> Yes.  In fact now that we have a separate function for the labeled 
> >> case, we can remove the funcall in both cases.  Patch attached.
> >
> > You are killing me.
> >
> 
> ???  That wasn't meant for the release branch.

Then what is?

bug#64391: buffer narrowing slowdown regression in emacs 29

[Gregory Heytings]

>> Yes.  In fact now that we have a separate function for the labeled 
>> case, we can remove the funcall in both cases.  Patch attached.
>
> Thank you, but are we sure that save-restriction would be used for 
> labelled restrictions in the future? Maybe we should just start with the 
> unlabelled case.
>

I'm sorry, I don't understand your question.  The only way (except by 
using internal functions) to enter a labeled restriction is to use the 
with-restriction special form with its optional label argument.  That form 
has a save-restriction since day one.

>>> The documentation should also make it clear that `nil` is not a valid 
>>> label.
>>
>> Is that not already clear in the docstring, which says "in which LABEL 
>> is a symbol"?
>
> Well, nil is a symbol.
>

Indeed, but I'd say it's clear enough from the context that "symbol" means 
a quoted symbol here.

bug#64391: buffer narrowing slowdown regression in emacs 29

[Gregory Heytings]

>> ???  That wasn't meant for the release branch.
>
> Then what is?
>

I was replying to Mattias who said, rightly, that using a funcall is not 
optimal here, and that we could do better.  I suggested a patch, but that 
optimization is independent of this bug.

bug#64391: buffer narrowing slowdown regression in emacs 29

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Fri, 7 Jul 2023 13:27:12 +0200
> Cc: gregory@heytings.org,
>  acohen@ust.hk,
>  64391@debbugs.gnu.org,
>  monnier@iro.umontreal.ca
> 
> 7 juli 2023 kl. 12.27 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > I was considering applying only the first of Gregory's patches,
> > without the second.  WDYT?
> 
> I don't think that makes much sense -- the division between the two is pretty much artificial.

It makes sense to me, because it minimizes changes in the code, which
at this stage in the pretest is something very important, IMO.

bug#64391: buffer narrowing slowdown regression in emacs 29

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

> I'm sorry, I don't understand your question.  The only way (except by using
> internal functions) to enter a labeled restriction is to use the
> with-restriction special form with its optional label argument.

Nitpick: it's not a special form, it's a macro.  There's a *big*
difference because adding a special form requires changing
`macroexpand-all`, the compiler, yadayada, and it introduces backward
incompatibilities with packages doing their own code-walks.

> Indeed, but I'd say it's clear enough from the context that "symbol" means
> a quoted symbol here.

Other nitpick: nil can also be quoted :-)
BTW, "LABEL is a symbol" sends the wrong message (a quoted
symbol evaluates to a symbol but it's not itself a symbol).
IOW the docstring should clarify that LABEL is an expression that's
evaluated at runtime (and should return a symbol).
While I'm here, is it important that LABEL evaluates to a symbol?
Or is it like `catch/throw` where we expect most uses to use a symbol
but where any other (non-nil) value works as well?


        Stefan


diff --git a/lisp/subr.el b/lisp/subr.el
index 85adef5b689..f8a34796584 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3964,11 +3964,11 @@ with-restriction
 
 The current restrictions, if any, are restored upon return.
 
-When the optional :label LABEL argument is present, in which
-LABEL is a symbol, inside BODY, `narrow-to-region' and `widen'
-can be used only within the START and END limits.  To gain access
-to other portions of the buffer, use `without-restriction' with the
-same LABEL argument.
+When the optional :label LABEL argument is present (in which
+LABEL evaluates to a non-nil symbol) inside BODY, `narrow-to-region'
+and `widen' can be used only within the START and END limits.
+To gain access to other portions of the buffer, use `without-restriction'
+with the same LABEL argument.
 
 \(fn START END [:label LABEL] BODY)"
   (declare (indent 2) (debug t))

bug#64391: buffer narrowing slowdown regression in emacs 29

[Mattias Engdegård]

7 juli 2023 kl. 14.50 skrev Eli Zaretskii <eliz@gnu.org>:

>>> I was considering applying only the first of Gregory's patches,
>>> without the second.  WDYT?
>> 
>> I don't think that makes much sense -- the division between the two is pretty much artificial.
> 
> It makes sense to me, because it minimizes changes in the code, which
> at this stage in the pretest is something very important, IMO.

Oh, I certainly appreciate the importance of conservatism in changes on the release branch, but sensibly so -- it's not a game of code golf. Only applying half of the intended change leaves the code in an ugly state, and does in fact not minimise risk at all, quite the opposite.

bug#64391: buffer narrowing slowdown regression in emacs 29

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Fri, 7 Jul 2023 18:49:56 +0200
> Cc: gregory@heytings.org,
>  acohen@ust.hk,
>  64391@debbugs.gnu.org,
>  monnier@iro.umontreal.ca
> 
> 7 juli 2023 kl. 14.50 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> >>> I was considering applying only the first of Gregory's patches,
> >>> without the second.  WDYT?
> >> 
> >> I don't think that makes much sense -- the division between the two is pretty much artificial.
> > 
> > It makes sense to me, because it minimizes changes in the code, which
> > at this stage in the pretest is something very important, IMO.
> 
> Oh, I certainly appreciate the importance of conservatism in changes
> on the release branch, but sensibly so -- it's not a game of code
> golf. Only applying half of the intended change leaves the code in
> an ugly state,

Ugly is in the eyes of the beholder.  The release of Emacs 29 was
already delayed twice by this feature, so please forgive me if my
sense of aesthetics in this regard is beginning to fade.

> and does in fact not minimise risk at all, quite the opposite.

How so?  If the patch solves the problem, the problem is solved,
period.  Where's the risk in not applying unnecessary changes?

bug#64391: buffer narrowing slowdown regression in emacs 29

[Eli Zaretskii]

> Date: Thu, 06 Jul 2023 17:33:13 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: acohen@ust.hk, 64391@debbugs.gnu.org, mattias.engdegard@gmail.com, 
>     monnier@iro.umontreal.ca
> 
> 
> I pushed a proposed change to the scratch/bug64391 branch.
> 
> It contains three commits:
> 
> - b741dc7fcd which is the minimal possible commit to fix this bug
> 
> - 01fb898420 which moves a few statements from the callee 
> (internal--label-restriction) to its only caller 
> (internal--labeled-narrow-to-region), and simplifies the code accordingly
> 
> - c52ade305e which is optional, and makes the symmetrical change for the 
> widening case

OK, please merge the first two commits to the release branch, and
thanks.

bug#64391: buffer narrowing slowdown regression in emacs 29

[Gregory Heytings]

>> I pushed a proposed change to the scratch/bug64391 branch.
>>
>> It contains three commits:
>>
>> - b741dc7fcd which is the minimal possible commit to fix this bug
>>
>> - 01fb898420 which moves a few statements from the callee 
>> (internal--label-restriction) to its only caller 
>> (internal--labeled-narrow-to-region), and simplifies the code 
>> accordingly
>>
>> - c52ade305e which is optional, and makes the symmetrical change for 
>> the widening case
>
> OK, please merge the first two commits to the release branch, and 
> thanks.
>

Thanks, now done.

I'll merge the third commit, and the other patch suggested by Mattias, to 
master.

bug#64391: buffer narrowing slowdown regression in emacs 29

[Gregory Heytings]

>> I'm sorry, I don't understand your question.  The only way (except by 
>> using internal functions) to enter a labeled restriction is to use the 
>> with-restriction special form with its optional label argument.
>
> Nitpick: it's not a special form, it's a macro.  There's a *big* 
> difference because adding a special form requires changing 
> `macroexpand-all`, the compiler, yadayada, and it introduces backward 
> incompatibilities with packages doing their own code-walks.
>

TIL.  I thought that "special form" and "macro" were more or less 
synonyms.  The manual describes lambda, prog2, setq-default, dlet, letrec, 
named-let, with-suppressed-warnings, with-no-warnings, with-restriction 
and without-restriction as "special forms", although they are in fact 
macros.  That being said, from a Elisp programmer viewpoint, special forms 
and macros are similar, and AFAIU one could say that a special form is a 
macro written in C, and a macro is a special form written in Elisp.

Should the above occurrences of "special form" be corrected in the 
manuals?

>> Indeed, but I'd say it's clear enough from the context that "symbol" 
>> means a quoted symbol here.
>
> Other nitpick: nil can also be quoted :-)
>

I knew someone would say that ;-)

>
> BTW, "LABEL is a symbol" sends the wrong message (a quoted symbol 
> evaluates to a symbol but it's not itself a symbol). IOW the docstring 
> should clarify that LABEL is an expression that's evaluated at runtime 
> (and should return a symbol). While I'm here, is it important that LABEL 
> evaluates to a symbol? Or is it like `catch/throw` where we expect most 
> uses to use a symbol but where any other (non-nil) value works as well?
>

The latter: it's like catch/throw, it's intended to use with a symbol but 
it could be any non-nil value.  So one could write something similar to 
what is found in the docstring of catch: "LABEL is evalled to get the 
label to use, it must not be nil".

bug#64391: buffer narrowing slowdown regression in emacs 29

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

>>> I'm sorry, I don't understand your question.  The only way (except by
>>> using internal functions) to enter a labeled restriction is to use the
>>> with-restriction special form with its optional label argument.
>> Nitpick: it's not a special form, it's a macro.  There's a *big*
>> difference because adding a special form requires changing
>> `macroexpand-all`, the compiler, yadayada, and it introduces backward
>> incompatibilities with packages doing their own code-walks.
> TIL.  I thought that "special form" and "macro" were more or less synonyms.
> The manual describes lambda, prog2, setq-default, dlet, letrec, named-let,
> with-suppressed-warnings, with-no-warnings, with-restriction and
> without-restriction as "special forms", although they are in fact macros.

You're right: from a programmer's stand point the distinction doesn't
really matter.  It matters only from the point of view of the language
implementer.  For some reason it tripped me, here (I went looking at the
code fearing that we were using an actual special form).

Sorry 'bout that, move along, nothing to see :-)

>> BTW, "LABEL is a symbol" sends the wrong message (a quoted symbol
>> evaluates to a symbol but it's not itself a symbol). IOW the docstring
>> should clarify that LABEL is an expression that's evaluated at runtime
>> (and should return a symbol). While I'm here, is it important that LABEL
>> evaluates to a symbol? Or is it like `catch/throw` where we expect most
>> uses to use a symbol but where any other (non-nil) value works as well?
>
> The latter: it's like catch/throw, it's intended to use with a symbol but it
> could be any non-nil value.  So one could write something similar to what is
> found in the docstring of catch: "LABEL is evalled to get the label to use,
> it must not be nil".

+1 from me,


        Stefan

bug#64391: buffer narrowing slowdown regression in emacs 29

[Gregory Heytings]

>> I thought that "special form" and "macro" were more or less synonyms. 
>> The manual describes lambda, prog2, setq-default, dlet, letrec, 
>> named-let, with-suppressed-warnings, with-no-warnings, with-restriction 
>> and without-restriction as "special forms", although they are in fact 
>> macros.
>
> You're right: from a programmer's stand point the distinction doesn't 
> really matter.  It matters only from the point of view of the language 
> implementer.  For some reason it tripped me, here (I went looking at the 
> code fearing that we were using an actual special form).
>
> Sorry 'bout that, move along, nothing to see :-)
>

Well, there's at least something that could be fixed in the manuals.  I 
admit I had never read the "Special Forms" section, and if the manual had 
been consistent about the special form vs. macro distiction, perhaps I 
wouldn't have confused these two similar, but subtly different, notions.

>> The latter: it's like catch/throw, it's intended to use with a symbol 
>> but it could be any non-nil value.  So one could write something 
>> similar to what is found in the docstring of catch: "LABEL is evalled 
>> to get the label to use, it must not be nil".
>
> +1 from me,
>

Eli, I guess that minor documentation fix is okay for emacs-29?

diff --git a/doc/lispref/positions.texi b/doc/lispref/positions.texi
index e74a165b9ed..af5e648eb9d 100644
--- a/doc/lispref/positions.texi
+++ b/doc/lispref/positions.texi
@@ -1169,9 +1169,9 @@ Narrowing

  @cindex labeled narrowing
  @cindex labeled restriction
-When the optional argument @var{label}, a symbol, is present, the
-narrowing is @dfn{labeled}.  A labeled narrowing differs from a
-non-labeled one in several ways:
+When the optional argument @var{label}, which may be any Lisp object
+except @code{nil}, is present, the narrowing is @dfn{labeled}.  A
+labeled narrowing differs from a non-labeled one in several ways:

  @itemize @bullet
  @item
diff --git a/lisp/subr.el b/lisp/subr.el
index 0b397b7bebf..b73d0e5d989 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3964,11 +3964,11 @@ with-restriction

  The current restrictions, if any, are restored upon return.

-When the optional :label LABEL argument is present, in which
-LABEL is a symbol, inside BODY, `narrow-to-region' and `widen'
-can be used only within the START and END limits.  To gain access
-to other portions of the buffer, use `without-restriction' with the
-same LABEL argument.
+When the optional :label LABEL argument, which is evalled to get
+the label to use and must not be nil, is present, inside BODY,
+`narrow-to-region' and `widen' can be used only within the START
+and END limits.  To gain access to other portions of the buffer,
+use `without-restriction' with the same LABEL argument.

  \(fn START END [:label LABEL] BODY)"
    (declare (indent 2) (debug t))

bug#64391: buffer narrowing slowdown regression in emacs 29

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

> Well, there's at least something that could be fixed in the manuals.
> I admit I had never read the "Special Forms" section, and if the manual had
> been consistent about the special form vs. macro distiction, perhaps
> I wouldn't have confused these two similar, but subtly different, notions.

At the same time, for the ELisp programmer, this distinction is just an
implementation detail (except for rare corner cases where the programmer
needs to look at the output of `macroexpand`).  What is a macro and what
is a special form has changed in the past and will likely change again
in the future (e.g. `defun`, `defmacro`, and `prog2` are now macros but
used to be special forms).


        Stefan

bug#64391: buffer narrowing slowdown regression in emacs 29

[Eli Zaretskii]

> Date: Sat, 08 Jul 2023 22:21:20 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: acohen@ust.hk, 64391@debbugs.gnu.org, mattias.engdegard@gmail.com, 
>     eliz@gnu.org, bugs@gnus.org
> 
> 
> Eli, I guess that minor documentation fix is okay for emacs-29?

Yes, but:

  . please use TAG, not LABEL for the :label's argument, since that's
    what it is;
  . please say "evaluated", not "evalled"

I also question the wisdom of telling only in the doc string that the
tag is evaluated.  I could understand if you did that the other way
around, but if the doc string says that, the manual should also say
it.

bug#64391: buffer narrowing slowdown regression in emacs 29

[Gregory Heytings]

>> Eli, I guess that minor documentation fix is okay for emacs-29?
>
> Yes, but:
>
> . please use TAG, not LABEL for the :label's argument, since that's what 
> it is;
>
> . please say "evaluated", not "evalled"
>

Okay.

>
> I also question the wisdom of telling only in the doc string that the 
> tag is evaluated.  I could understand if you did that the other way 
> around, but if the doc string says that, the manual should also say it.
>

But the patch I sent changes both the manual and the docstring?  Am I 
missing something?

bug#64391: buffer narrowing slowdown regression in emacs 29

[Eli Zaretskii]

> Date: Sun, 09 Jul 2023 08:35:29 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: acohen@ust.hk, 64391@debbugs.gnu.org, mattias.engdegard@gmail.com, 
>     monnier@iro.umontreal.ca, bugs@gnus.org
> 
> > I also question the wisdom of telling only in the doc string that the 
> > tag is evaluated.  I could understand if you did that the other way 
> > around, but if the doc string says that, the manual should also say it.
> >
> 
> But the patch I sent changes both the manual and the docstring?

It does, but only the latter says that LABEL is evaluated.

bug#64391: buffer narrowing slowdown regression in emacs 29

[Gregory Heytings]

>> Well, there's at least something that could be fixed in the manuals. I 
>> admit I had never read the "Special Forms" section, and if the manual 
>> had been consistent about the special form vs. macro distiction, 
>> perhaps I wouldn't have confused these two similar, but subtly 
>> different, notions.
>
> At the same time, for the ELisp programmer, this distinction is just an 
> implementation detail (except for rare corner cases where the programmer 
> needs to look at the output of `macroexpand`).  What is a macro and what 
> is a special form has changed in the past and will likely change again 
> in the future (e.g. `defun`, `defmacro`, and `prog2` are now macros but 
> used to be special forms).
>

Indeed, but... what would you suggest?  Leave the manual, in which that 
distinction is not always clear, as it is?  Fix the manual to make that 
distinction clearer?  Remove that distinction, which is indeed an 
implementation detail, from the manual (and perhaps mention that some 
macros are not defined in Elisp but in C, in which case they can also be 
called "special forms")?

bug#64391: buffer narrowing slowdown regression in emacs 29

[Gregory Heytings]

>>> I also question the wisdom of telling only in the doc string that the 
>>> tag is evaluated.  I could understand if you did that the other way 
>>> around, but if the doc string says that, the manual should also say 
>>> it.
>>
>> But the patch I sent changes both the manual and the docstring?
>
> It does, but only the latter says that LABEL is evaluated.
>

Oh, indeed, now I see what you mean.  Here's the updated patch.

On a second thought, I believe its better to not replace LABEL with TAG, 
because that would mean changing that word in many places, including 
places in which such a change would make the text less understandable, 
e.g. the docstring of narrow-to-region:

However, when restrictions have been set by `with-restriction' with a 
label, `narrow-to-region' can be used only within the limits of these 
restrictions.  If the START or END arguments are outside these limits, the 
corresponding limit set by `with-restriction' is used instead of the 
argument.  To gain access to other portions of the buffer, use 
`without-restriction' with the same label.

diff --git a/doc/lispref/positions.texi b/doc/lispref/positions.texi
index e74a165b9ed..183d0e39aee 100644
--- a/doc/lispref/positions.texi
+++ b/doc/lispref/positions.texi
@@ -1169,9 +1169,10 @@ Narrowing

  @cindex labeled narrowing
  @cindex labeled restriction
-When the optional argument @var{label}, a symbol, is present, the
-narrowing is @dfn{labeled}.  A labeled narrowing differs from a
-non-labeled one in several ways:
+When the optional argument @var{label}, which is evaluated to get the
+label to use and must not be @code{nil}, is present, the narrowing is
+@dfn{labeled}.  A labeled narrowing differs from a non-labeled one in
+several ways:

  @itemize @bullet
  @item
diff --git a/lisp/subr.el b/lisp/subr.el
index 0b397b7bebf..2f0144e0f11 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3964,11 +3964,11 @@ with-restriction

  The current restrictions, if any, are restored upon return.

-When the optional :label LABEL argument is present, in which
-LABEL is a symbol, inside BODY, `narrow-to-region' and `widen'
-can be used only within the START and END limits.  To gain access
-to other portions of the buffer, use `without-restriction' with the
-same LABEL argument.
+When the optional LABEL argument, which is evaluated to get the
+label to use and must not be nil, is present, inside BODY,
+`narrow-to-region' and `widen' can be used only within the START
+and END limits.  To gain access to other portions of the buffer,
+use `without-restriction' with the same LABEL argument.

  \(fn START END [:label LABEL] BODY)"
    (declare (indent 2) (debug t))
@@ -3990,9 +3990,8 @@ without-restriction

  The current restrictions, if any, are restored upon return.

-When the optional :label LABEL argument is present, the
-restrictions set by `with-restriction' with the same LABEL argument
-are lifted.
+When the optional LABEL argument is present, the restrictions set
+by `with-restriction' with the same LABEL argument are lifted.

  \(fn [:label LABEL] BODY)"
    (declare (indent 0) (debug t))

bug#64391: buffer narrowing slowdown regression in emacs 29

[Eli Zaretskii]

> Date: Sun, 09 Jul 2023 16:04:09 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: acohen@ust.hk, 64391@debbugs.gnu.org, mattias.engdegard@gmail.com, 
>     monnier@iro.umontreal.ca, bugs@gnus.org
> 
> On a second thought, I believe its better to not replace LABEL with TAG, 
> because that would mean changing that word in many places, including 
> places in which such a change would make the text less understandable, 
> e.g. the docstring of narrow-to-region:

LABEL implies a string or a symbol, whereas :label can accept "any
object that is not nil".  The description of 'catch' uses TAG.

> --- a/doc/lispref/positions.texi
> +++ b/doc/lispref/positions.texi
> @@ -1169,9 +1169,10 @@ Narrowing
> 
>   @cindex labeled narrowing
>   @cindex labeled restriction
> -When the optional argument @var{label}, a symbol, is present, the
> -narrowing is @dfn{labeled}.  A labeled narrowing differs from a
> -non-labeled one in several ways:
> +When the optional argument @var{label}, which is evaluated to get the
> +label to use and must not be @code{nil},

What "must not be nil": the label or the result of its evaluation?

bug#64391: buffer narrowing slowdown regression in emacs 29

[Gregory Heytings]

>> On a second thought, I believe its better to not replace LABEL with 
>> TAG, because that would mean changing that word in many places, 
>> including places in which such a change would make the text less 
>> understandable, e.g. the docstring of narrow-to-region:
>
> LABEL implies a string or a symbol, whereas :label can accept "any 
> object that is not nil".  The description of 'catch' uses TAG.
>

I know, and I agree that in principle TAG would be better.  However all 
the code and documentation was written with the word "label", and although 
using a non-symbol is possible, LABEL is intended to be a symbol, not an 
arbitrary Lisp object.  So perhaps the best thing to do would be this:

diff --git a/lisp/subr.el b/lisp/subr.el
index 0b397b7bebf..c2110cb4bb2 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3972,7 +3972,8 @@ with-restriction

  \(fn START END [:label LABEL] BODY)"
    (declare (indent 2) (debug t))
-  (if (eq (car rest) :label)
+  (if (and (eq (car rest) :label)
+           (symbolp (cadr rest)))
        `(internal--with-restriction ,start ,end (lambda () ,@(cddr rest))
                                   ,(cadr rest))
      `(internal--with-restriction ,start ,end (lambda () ,@rest))))

>> --- a/doc/lispref/positions.texi
>> +++ b/doc/lispref/positions.texi
>> @@ -1169,9 +1169,10 @@ Narrowing
>>
>>   @cindex labeled narrowing
>>   @cindex labeled restriction
>> -When the optional argument @var{label}, a symbol, is present, the
>> -narrowing is @dfn{labeled}.  A labeled narrowing differs from a
>> -non-labeled one in several ways:
>> +When the optional argument @var{label}, which is evaluated to get the
>> +label to use and must not be @code{nil},
>
> What "must not be nil": the label or the result of its evaluation?
>

The result of the evaluation of the label argument.  I don't know how to 
make this clearer.

bug#64391: buffer narrowing slowdown regression in emacs 29

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

>  \(fn START END [:label LABEL] BODY)"
>    (declare (indent 2) (debug t))
> -  (if (eq (car rest) :label)
> +  (if (and (eq (car rest) :label)
> +           (symbolp (cadr rest)))
>        `(internal--with-restriction ,start ,end (lambda () ,@(cddr rest))
>                                   ,(cadr rest))
>      `(internal--with-restriction ,start ,end (lambda () ,@rest))))

Doesn't look right: (cadr rest) should be an *expression* that evaluates
to a symbol, so in general it won't itself be a symbol.

>>> -When the optional argument @var{label}, a symbol, is present, the
>>> -narrowing is @dfn{labeled}.  A labeled narrowing differs from a
>>> -non-labeled one in several ways:
>>> +When the optional argument @var{label}, which is evaluated to get the
>>> +label to use and must not be @code{nil},
>> What "must not be nil": the label or the result of its evaluation?
> The result of the evaluation of the label argument.  I don't know how to
> make this clearer.

Maybe:

    When the optional argument @var{label}, which should evaluate to
    a non-nil value,


-- Stefan

bug#64391: buffer narrowing slowdown regression in emacs 29

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

> Indeed, but... what would you suggest?  Leave the manual, in which that
> distinction is not always clear, as it is?

Yup.


        Stefan

bug#64391: buffer narrowing slowdown regression in emacs 29

[Eli Zaretskii]

> Date: Sun, 09 Jul 2023 18:03:02 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: acohen@ust.hk, 64391@debbugs.gnu.org, mattias.engdegard@gmail.com, 
>     monnier@iro.umontreal.ca, bugs@gnus.org
> 
> >> +When the optional argument @var{label}, which is evaluated to get the
> >> +label to use and must not be @code{nil},
> >
> > What "must not be nil": the label or the result of its evaluation?
> >
> 
> The result of the evaluation of the label argument.  I don't know how to 
> make this clearer.

Instead of "must not be nil", say "must not yield nil".

bug#64391: buffer narrowing slowdown regression in emacs 29

[Eli Zaretskii]

> Cc: acohen@ust.hk, 64391@debbugs.gnu.org, mattias.engdegard@gmail.com,
>  monnier@iro.umontreal.ca
> Date: Sun, 09 Jul 2023 22:03:41 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> Instead of "must not be nil", say "must not yield nil".

Or rather "must yield a non-nil value".

bug#64391: buffer narrowing slowdown regression in emacs 29

[Gregory Heytings]

>>  \(fn START END [:label LABEL] BODY)"
>>    (declare (indent 2) (debug t))
>> -  (if (eq (car rest) :label)
>> +  (if (and (eq (car rest) :label)
>> +           (symbolp (cadr rest)))
>>        `(internal--with-restriction ,start ,end (lambda () ,@(cddr rest))
>>                                   ,(cadr rest))
>>      `(internal--with-restriction ,start ,end (lambda () ,@rest))))
>
> Doesn't look right: (cadr rest) should be an *expression* that evaluates 
> to a symbol, so in general it won't itself be a symbol.
>

Whoops, indeed.  (symbolp (eval (cadr rest))) would work, but somehow I 
think it's not TRT and there's a more idiomatic way to do that.

bug#64391: buffer narrowing slowdown regression in emacs 29

[Gregory Heytings]

>> Instead of "must not be nil", say "must not yield nil".
>
> Or rather "must yield a non-nil value".
>

Thanks, done (419b4d4491).

bug#64391: buffer narrowing slowdown regression in emacs 29

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

> Whoops, indeed.  (symbolp (eval (cadr rest))) would work,

No, it wouldn't work: we can't (in general) decide at macroexpansion
time whether the expression will return a symbol at runtime.

> but somehow I think it's not TRT and there's a more idiomatic way to
> do that.

Yes: don't do it, or do it via a runtime check rather than
a compile-time check.  But here I don't see any benefit to imposing that
LABEL evaluates to a symbol.


        Stefan

bug#64391: buffer narrowing slowdown regression in emacs 29

[Gregory Heytings]

>> but somehow I think it's not TRT and there's a more idiomatic way to do 
>> that.
>
> Yes: don't do it, or do it via a runtime check rather than a 
> compile-time check.  But here I don't see any benefit to imposing that 
> LABEL evaluates to a symbol.
>

Agreed.

I guess this bug can be closed for good now?