GNU bug report logs - #36190
27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

Previous Next

Package: emacs;

Reported by: Pip Cet <pipcet <at> gmail.com>

Date: Thu, 13 Jun 2019 13:50:02 UTC

Severity: normal

Found in version 27.0.50

Done: Eli Zaretskii <eliz <at> gnu.org>

Bug is archived. No further changes may be made.

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

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

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Thu, 13 Jun 2019 13:50:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Pip Cet <pipcet <at> gmail.com>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Thu, 13 Jun 2019 13:50:02 GMT) Full text and rfc822 format available.

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

From: Pip Cet <pipcet <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.0.50; `put-text-property' etc. with buffer argument calls current
 buffer's `after-change-functions'
Date: Thu, 13 Jun 2019 13:48:40 +0000
In emacs -Q, evaluating:

(let ((buffer1 (generate-new-buffer "A"))
      (buffer2 (generate-new-buffer "B")))
  (with-current-buffer buffer2
    (insert "BBB"))
  (with-current-buffer buffer1
    (add-hook 'after-change-functions
          (lambda (beg end len)
        (message "%S %S %S"
             beg end len))
          nil t)
    (put-text-property 1 4 'read-only t buffer2)))

results in a "1 4 3" message. I would have expected no message, as
buffer2 was modified and buffer1, whose after-change-functions I'd
set, wasn't.

I've looked at the code, and it appears no particular provisions are
being made to make sure we switch to the modified buffer before
calling signal_after_change().

As far as I can tell, this makes `put-text-property' with a buffer
argument pretty useless. Am I missing something? Is this expected
behavior somehow?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Thu, 13 Jun 2019 16:37:01 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> gmail.com>
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50;
 `put-text-property' etc. with buffer argument calls current buffer's
 `after-change-functions'
Date: Thu, 13 Jun 2019 19:36:09 +0300
> From: Pip Cet <pipcet <at> gmail.com>
> Date: Thu, 13 Jun 2019 13:48:40 +0000
> 
> I've looked at the code, and it appears no particular provisions are
> being made to make sure we switch to the modified buffer before
> calling signal_after_change().
> 
> As far as I can tell, this makes `put-text-property' with a buffer
> argument pretty useless.

Only if you have a buffer-local value of after-change-functions.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Thu, 13 Jun 2019 18:50:01 GMT) Full text and rfc822 format available.

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

From: Pip Cet <pipcet <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument
 calls current buffer's `after-change-functions'
Date: Thu, 13 Jun 2019 18:48:23 +0000
[Message part 1 (text/plain, inline)]
On Thu, Jun 13, 2019 at 4:36 PM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > From: Pip Cet <pipcet <at> gmail.com>
> > Date: Thu, 13 Jun 2019 13:48:40 +0000
> >
> > I've looked at the code, and it appears no particular provisions are
> > being made to make sure we switch to the modified buffer before
> > calling signal_after_change().
> >
> > As far as I can tell, this makes `put-text-property' with a buffer
> > argument pretty useless.
>
> Only if you have a buffer-local value of after-change-functions.

I'm not sure what you're saying. I'm seeing weird behavior in these cases:
 - buffer-local value of after-change-functions
 - global value of after-change functions (current-buffer is wrong!)
 - overlay property modification-hooks

(let ((buffer1 (generate-new-buffer "A"))
      (buffer2 (generate-new-buffer "B")))
  (with-current-buffer buffer2
    (insert "BBB"))
  (with-current-buffer buffer1
    (insert "AAA")
    (overlay-put (make-overlay 1 4 buffer1) 'modification-hooks
         (list (lambda (&rest args)
           (message "%S"
                args))))
    (goto-char 2)
    (insert "AAA")
    (put-text-property 1 4 'read-only t buffer2)))

produces three calls to the lambda, but should produce only two. (The
last_overlay_modification_hooks logic is a bit weird, thus the second
insertion).

That seems pretty wrong to me. In which cases do you think we're
seeing the right behavior?

Here's a first patch, which adds a "buffer" argument to
signal_after_change, to be explicit about where the change happens. It
should be pretty cheap in the case where we don't switch buffers.
[0001-Switch-to-modified-buffer-in-signal_after_change.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Thu, 13 Jun 2019 19:07:02 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> gmail.com>
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument
 calls current buffer's `after-change-functions'
Date: Thu, 13 Jun 2019 22:05:48 +0300
> From: Pip Cet <pipcet <at> gmail.com>
> Date: Thu, 13 Jun 2019 18:48:23 +0000
> Cc: 36190 <at> debbugs.gnu.org
> 
> > > As far as I can tell, this makes `put-text-property' with a buffer
> > > argument pretty useless.
> >
> > Only if you have a buffer-local value of after-change-functions.
> 
> I'm not sure what you're saying.

I'm saying that the buffer argument to put-text-property is pretty
useless only if you consider after-change-functions.  The primary
purpose of put-text-property is to modify text properties, not to call
after-change-functions.  For that primary purpose, the buffer argument
is not useless.

> That seems pretty wrong to me. In which cases do you think we're
> seeing the right behavior?

Where did I say that this behavior was right?

> Here's a first patch, which adds a "buffer" argument to
> signal_after_change, to be explicit about where the change happens. It
> should be pretty cheap in the case where we don't switch buffers.

Not sure I have a clear idea of how you intend to use that additional
argument.  Are you suggesting that we switch to that buffer?  If so,
how is that different from not using the buffer argument at all, and
instead wrapping the call to put-text-property with
with-current-buffer?

Also, passing current_buffer sounds redundant to me anyway, because in
that case signal_after_change will not need to do anything that it
doesn't already do.  I would pass NULL instead.

Thanks.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Thu, 13 Jun 2019 19:29:01 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: pipcet <at> gmail.com
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50;
 `put-text-property' etc. with buffer argument calls current buffer's
 `after-change-functions'
Date: Thu, 13 Jun 2019 22:27:56 +0300
> Date: Thu, 13 Jun 2019 22:05:48 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> Cc: 36190 <at> debbugs.gnu.org
> 
> > Here's a first patch, which adds a "buffer" argument to
> > signal_after_change, to be explicit about where the change happens. It
> > should be pretty cheap in the case where we don't switch buffers.
> 
> Not sure I have a clear idea of how you intend to use that additional
> argument.

Found it:

> +  record_unwind_current_buffer ();
> +  set_buffer_internal (buffer);




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Thu, 13 Jun 2019 19:44:02 GMT) Full text and rfc822 format available.

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

From: Pip Cet <pipcet <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument
 calls current buffer's `after-change-functions'
Date: Thu, 13 Jun 2019 19:42:29 +0000
On Thu, Jun 13, 2019 at 7:06 PM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > From: Pip Cet <pipcet <at> gmail.com>
> > Date: Thu, 13 Jun 2019 18:48:23 +0000
> > Cc: 36190 <at> debbugs.gnu.org
> >
> > > > As far as I can tell, this makes `put-text-property' with a buffer
> > > > argument pretty useless.
> > >
> > > Only if you have a buffer-local value of after-change-functions.
> >
> > I'm not sure what you're saying.
>
> I'm saying that the buffer argument to put-text-property is pretty
> useless only if you consider after-change-functions.  The primary
> purpose of put-text-property is to modify text properties, not to call
> after-change-functions.  For that primary purpose, the buffer argument
> is not useless.

Thanks for clarifying. I suppose you could say that it's
after-change-functions (local, global, plus overlay modification
hooks) that have become useless, or will be called spuriously and with
potentially nonsensical arguments.

For example, this would break:

(push (lambda (beg end len)
        (message "%S" (buffer-substring beg end)))
      after-change-functions)

> > That seems pretty wrong to me. In which cases do you think we're
> > seeing the right behavior?
>
> Where did I say that this behavior was right?

You said "only if", so I assumed you were asserting the contrapositive.

> > Here's a first patch, which adds a "buffer" argument to
> > signal_after_change, to be explicit about where the change happens. It
> > should be pretty cheap in the case where we don't switch buffers.
>
> Not sure I have a clear idea of how you intend to use that additional
> argument.  Are you suggesting that we switch to that buffer?

Yes:

@@ -2183,6 +2184,9 @@ signal_after_change (ptrdiff_t charpos,
ptrdiff_t lendel, ptrdiff_t lenins)
   if (inhibit_modification_hooks)
     return;

+  record_unwind_current_buffer ();
+  set_buffer_internal (buffer);
+
   /* If we are deferring calls to the after-change functions
      and there are no before-change functions,
      just record the args that we were going to use.  */

> If so,
> how is that different from not using the buffer argument at all, and
> instead wrapping the call to put-text-property with
> with-current-buffer?

I don't think they're usefully different, but put-text-property
doesn't appear to check the buffer is still live.

> Also, passing current_buffer sounds redundant to me anyway, because in
> that case signal_after_change will not need to do anything that it
> doesn't already do.  I would pass NULL instead.

May I ask why? I think passing current_buffer is the clearest signal
we can send to someone reusing the code that they might have to change
this if they're dealing with more than one buffer.

As a practical matter, it's hard to change the text property functions
to use NULL when passed a nil argument, so we'd have functions using
current_buffer and others using NULL, and that seems needlessly
inconsistent.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Thu, 13 Jun 2019 20:02:02 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> gmail.com>
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument
 calls current buffer's `after-change-functions'
Date: Thu, 13 Jun 2019 23:01:03 +0300
> From: Pip Cet <pipcet <at> gmail.com>
> Date: Thu, 13 Jun 2019 19:42:29 +0000
> Cc: 36190 <at> debbugs.gnu.org
> 
> > Not sure I have a clear idea of how you intend to use that additional
> > argument.  Are you suggesting that we switch to that buffer?
> 
> Yes:
> 
> @@ -2183,6 +2184,9 @@ signal_after_change (ptrdiff_t charpos,
> ptrdiff_t lendel, ptrdiff_t lenins)
>    if (inhibit_modification_hooks)
>      return;
> 
> +  record_unwind_current_buffer ();
> +  set_buffer_internal (buffer);

Ugh! switching buffers just to run a hook!  This will kill performance
in some cases.  We had something similar with JSON parsing a few
months ago.  I wish we had a better alternative.  Maybe we should warn
in the documentation that calling these functions with BUFFER being
other than the current buffer might hurt performance when
after-change-functions is non-nil.

> > Also, passing current_buffer sounds redundant to me anyway, because in
> > that case signal_after_change will not need to do anything that it
> > doesn't already do.  I would pass NULL instead.
> 
> May I ask why?

To make the code speak for itself.  With passing current_buffer, you
now rely on subroutines of set_buffer_internal two or 3 levels down to
test whether we are already in that buffer and do nothing.  Meanwhile,
you wasted cycles on 2 or 3 function calls, and forced someone who
reads the code to go down that rabbit hole if they want to understand
what happens in that particular case.

> I think passing current_buffer is the clearest signal we can send to
> someone reusing the code that they might have to change this if
> they're dealing with more than one buffer.

Each function has commentary, where you can say that NULL means not to
switch buffers because we are already there.  That is a more clear
signal, IME.

> As a practical matter, it's hard to change the text property functions
> to use NULL when passed a nil argument

How is it harder than passing current_buffer?

> so we'd have functions using current_buffer and others using NULL,
> and that seems needlessly inconsistent.

Sorry, I don't see any inconsistency.  We do such things (for other
kinds of arguments) all over the place.

It's really a matter of stylistic preferences, but you did ask why...




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Thu, 13 Jun 2019 20:58:02 GMT) Full text and rfc822 format available.

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

From: Pip Cet <pipcet <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument
 calls current buffer's `after-change-functions'
Date: Thu, 13 Jun 2019 20:57:08 +0000
On Thu, Jun 13, 2019 at 8:01 PM Eli Zaretskii <eliz <at> gnu.org> wrote:
> > From: Pip Cet <pipcet <at> gmail.com>
> > Date: Thu, 13 Jun 2019 19:42:29 +0000
> > Cc: 36190 <at> debbugs.gnu.org
> >
> > > Not sure I have a clear idea of how you intend to use that additional
> > > argument.  Are you suggesting that we switch to that buffer?
> >
> > Yes:
> >
> > @@ -2183,6 +2184,9 @@ signal_after_change (ptrdiff_t charpos,
> > ptrdiff_t lendel, ptrdiff_t lenins)
> >    if (inhibit_modification_hooks)
> >      return;
> >
> > +  record_unwind_current_buffer ();
> > +  set_buffer_internal (buffer);
>
> Ugh! switching buffers just to run a hook!
>  This will kill performance
> in some cases.

I really don't think it will have a noticeable impact on performance,
but if you can think of a scenario, we could try to fix it.

> We had something similar with JSON parsing a few
> months ago.
> I wish we had a better alternative.

(Such as not calling regular modification hooks for text property changes?)

>  Maybe we should warn
> in the documentation that calling these functions with BUFFER being
> other than the current buffer might hurt performance when
> after-change-functions is non-nil.

It'll hurt performance even when after-change-functions is nil, so
such a warning would be overspecific.

> > As a practical matter, it's hard to change the text property functions
> > to use NULL when passed a nil argument
>
> How is it harder than passing current_buffer?

The code path goes through

  if (NILP (object))
    XSETBUFFER (object, current_buffer);

> It's really a matter of stylistic preferences, but you did ask why...

It was out of genuine interest, because passing NULL to implicitly
specify a default argument is something that people advocate against,
and that C in particular has a history of avoiding (stdout isn't NULL,
and post-VAX NULL isn't an empty string, for example). Thank you for
responding, and I'll change my patch accordingly.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Thu, 13 Jun 2019 21:39:01 GMT) Full text and rfc822 format available.

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

From: Pip Cet <pipcet <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument
 calls current buffer's `after-change-functions'
Date: Thu, 13 Jun 2019 21:37:40 +0000
[Message part 1 (text/plain, inline)]
Here's a simplified patch (I'm unsure about the right tab convention
to use; I hope I got this right).
[0001-Switch-to-correct-buffer-in-put-text-property-etc.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Fri, 14 Jun 2019 07:38:01 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> gmail.com>
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument
 calls current buffer's `after-change-functions'
Date: Fri, 14 Jun 2019 10:36:57 +0300
> From: Pip Cet <pipcet <at> gmail.com>
> Date: Thu, 13 Jun 2019 20:57:08 +0000
> Cc: 36190 <at> debbugs.gnu.org
> 
> > > +  record_unwind_current_buffer ();
> > > +  set_buffer_internal (buffer);
> >
> > Ugh! switching buffers just to run a hook!  This will kill
> >  performance in some cases.
> 
> I really don't think it will have a noticeable impact on performance,
> but if you can think of a scenario, we could try to fix it.

Switching buffers means rebinding values of all the buffer-local
variables, of which there could be quite a few.  Or am I missing
something?

One scenario where this could be painful could be reading a stream of
data that results in many changes in text properties, such as
fontifying a buffer of program source by using syntactical analysis
data received from a language server.  If you read and apply the input
one object at a time, this will result in many buffer switches.

> > I wish we had a better alternative.
> 
> (Such as not calling regular modification hooks for text property changes?)

I thought about that, but I don't think this would be acceptable.

> >  Maybe we should warn
> > in the documentation that calling these functions with BUFFER being
> > other than the current buffer might hurt performance when
> > after-change-functions is non-nil.
> 
> It'll hurt performance even when after-change-functions is nil, so
> such a warning would be overspecific.

We could avoid switching buffers if the hook is nil, at least in
principle.  If not, it's even worse than I feared.

> > > As a practical matter, it's hard to change the text property functions
> > > to use NULL when passed a nil argument
> >
> > How is it harder than passing current_buffer?
> 
> The code path goes through
> 
>   if (NILP (object))
>     XSETBUFFER (object, current_buffer);

I meant in the cases where you pass the literal current_buffer.

But even the above is not a problem:

  struct buffer *b;
  if (NILP (object))
    {
      XSETBUFFER (object, current_buffer);
      b = NULL;
    }
  else if (BUFFERP (object))
    b = XBUFFER (object);
  [...]
  signal_after_change (b, ...);

> It was out of genuine interest, because passing NULL to implicitly
> specify a default argument is something that people advocate against,

Not to specify the default, but to indicate that no action is needed
at all wrt the buffer.  It is similar to the last argument to
'strtol', for example.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Fri, 14 Jun 2019 07:42:02 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> gmail.com>
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument
 calls current buffer's `after-change-functions'
Date: Fri, 14 Jun 2019 10:41:45 +0300
> From: Pip Cet <pipcet <at> gmail.com>
> Date: Thu, 13 Jun 2019 21:37:40 +0000
> Cc: 36190 <at> debbugs.gnu.org
> 
> +/* Signal a change immediately after it happens.
> +   BUFFER is the buffer in which the change happened.
> +   CHARPOS is the character position of the start of the changed text.
> +   LENDEL is the number of characters of the text before the change.
> +   (Not the whole buffer; just the part that was changed.)
> +   LENINS is the number of characters in that part of the text
> +   after the change.  */

I would just say "Like signal_after_change, but ..." and describe only
the BUFFER argument.

> +void
> +signal_after_change_in_buffer (struct buffer *buffer, ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
> +{
> +  ptrdiff_t count = SPECPDL_INDEX ();
> +
> +  record_unwind_current_buffer ();
> +  set_buffer_internal (buffer);
> +  signal_after_change (charpos, lendel, lenins);
> +  unbind_to (count, Qnil);
> +}

I still think we should explicitly detect the current_buffer case here
and if so, avoid the calls to everything else except
signal_after_change itself.

But I indeed like this patch better, although the concerns over the
performance hit are still present.  Thanks.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Fri, 14 Jun 2019 11:16:02 GMT) Full text and rfc822 format available.

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

From: Pip Cet <pipcet <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument
 calls current buffer's `after-change-functions'
Date: Fri, 14 Jun 2019 11:14:53 +0000
[Message part 1 (text/plain, inline)]
On Fri, Jun 14, 2019 at 7:41 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > From: Pip Cet <pipcet <at> gmail.com>
> > Date: Thu, 13 Jun 2019 21:37:40 +0000
> > Cc: 36190 <at> debbugs.gnu.org
> >
> > +/* Signal a change immediately after it happens.
> > +   BUFFER is the buffer in which the change happened.
> > +   CHARPOS is the character position of the start of the changed text.
> > +   LENDEL is the number of characters of the text before the change.
> > +   (Not the whole buffer; just the part that was changed.)
> > +   LENINS is the number of characters in that part of the text
> > +   after the change.  */
>
> I would just say "Like signal_after_change, but ..." and describe only
> the BUFFER argument.
>
> > +void
> > +signal_after_change_in_buffer (struct buffer *buffer, ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
> > +{
> > +  ptrdiff_t count = SPECPDL_INDEX ();
> > +
> > +  record_unwind_current_buffer ();
> > +  set_buffer_internal (buffer);
> > +  signal_after_change (charpos, lendel, lenins);
> > +  unbind_to (count, Qnil);
> > +}
>
> I still think we should explicitly detect the current_buffer case here
> and if so, avoid the calls to everything else except
> signal_after_change itself.
>
> But I indeed like this patch better, although the concerns over the
> performance hit are still present.  Thanks.

Okay, I fixed those two issues. As for the performance problem, should
we amend the documentation to state that if many changes are made,
it's better to use `with-current-buffer' instead of repeatedly calling
put-text-property with a buffer argument?
[0001-Switch-to-correct-buffer-in-put-text-property-etc.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Fri, 14 Jun 2019 12:12:01 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> gmail.com>
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument
 calls current buffer's `after-change-functions'
Date: Fri, 14 Jun 2019 15:10:52 +0300
> From: Pip Cet <pipcet <at> gmail.com>
> Date: Fri, 14 Jun 2019 11:14:53 +0000
> Cc: 36190 <at> debbugs.gnu.org
> 
> Okay, I fixed those two issues.

Thanks.

> As for the performance problem, should we amend the documentation to
> state that if many changes are made, it's better to use
> `with-current-buffer' instead of repeatedly calling
> put-text-property with a buffer argument?

I think so, yes.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Sat, 15 Jun 2019 15:16:01 GMT) Full text and rfc822 format available.

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

From: Pip Cet <pipcet <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument
 calls current buffer's `after-change-functions'
Date: Sat, 15 Jun 2019 15:14:43 +0000
[Message part 1 (text/plain, inline)]
On Fri, Jun 14, 2019 at 12:11 PM Eli Zaretskii <eliz <at> gnu.org> wrote:
> > From: Pip Cet <pipcet <at> gmail.com>
> > Date: Fri, 14 Jun 2019 11:14:53 +0000
> > Cc: 36190 <at> debbugs.gnu.org
> >
> > Okay, I fixed those two issues.
>
> Thanks.

Except that calls set_buffer_internal twice for a single call to
put-text-property. That's too slow, right?

How about simply emulating (with-current-buffer buffer
(put-text-property ...)), as the attached patch does? That should fix
the immediate problem.

Thanks!
[0001-Update-current-buffer-when-changing-text-properties.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Sat, 15 Jun 2019 15:24:02 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> gmail.com>
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument
 calls current buffer's `after-change-functions'
Date: Sat, 15 Jun 2019 18:23:15 +0300
> From: Pip Cet <pipcet <at> gmail.com>
> Date: Sat, 15 Jun 2019 15:14:43 +0000
> Cc: 36190 <at> debbugs.gnu.org
> 
> Except that calls set_buffer_internal twice for a single call to
> put-text-property. That's too slow, right?
> 
> How about simply emulating (with-current-buffer buffer
> (put-text-property ...)), as the attached patch does? That should fix
> the immediate problem.

Looks better to me, but I'd suggest a comment explaining why we do
such an unusual thing.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Sat, 15 Jun 2019 19:29:01 GMT) Full text and rfc822 format available.

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

From: Pip Cet <pipcet <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument
 calls current buffer's `after-change-functions'
Date: Sat, 15 Jun 2019 19:27:30 +0000
[Message part 1 (text/plain, inline)]
On Sat, Jun 15, 2019 at 3:23 PM Eli Zaretskii <eliz <at> gnu.org> wrote:
> > From: Pip Cet <pipcet <at> gmail.com>
> > Date: Sat, 15 Jun 2019 15:14:43 +0000
> > Cc: 36190 <at> debbugs.gnu.org
> >
> > How about simply emulating (with-current-buffer buffer
> > (put-text-property ...)), as the attached patch does? That should fix
> > the immediate problem.
>
> Looks better to me, but I'd suggest a comment explaining why we do
> such an unusual thing.

Sounds good to me. The attached patch contains the comments and a hint
in the documentation. (Maybe the comments should be a FIXME, because
there's really no need to switch buffers at all when there are no
applicable hooks?)

Obviously, feel free to reword as you consider appropriate.
[0001-Update-current-buffer-when-changing-text-properties.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Mon, 17 Jun 2019 11:40:02 GMT) Full text and rfc822 format available.

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

From: Pip Cet <pipcet <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument
 calls current buffer's `after-change-functions'
Date: Mon, 17 Jun 2019 11:38:38 +0000
On Fri, Jun 14, 2019 at 7:36 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
> > From: Pip Cet <pipcet <at> gmail.com>
> > Date: Thu, 13 Jun 2019 20:57:08 +0000
> > Cc: 36190 <at> debbugs.gnu.org
> >
> > > > +  record_unwind_current_buffer ();
> > > > +  set_buffer_internal (buffer);
> > >
> > > Ugh! switching buffers just to run a hook!  This will kill
> > >  performance in some cases.
> >
> > I really don't think it will have a noticeable impact on performance,
> > but if you can think of a scenario, we could try to fix it.
>
> Switching buffers means rebinding values of all the buffer-local
> variables, of which there could be quite a few.  Or am I missing
> something?

I just don't see how the requirement to switch buffers for modifying
text properties is so different, performance-wise, from the case of
modifying buffer text; in the latter case, we simply accept we can do
so only for the current buffer.

In any case, the current code already switches buffers, it's just a
question of doing so twice rather than once.

> One scenario where this could be painful could be reading a stream of
> data that results in many changes in text properties, such as
> fontifying a buffer of program source by using syntactical analysis
> data received from a language server.  If you read and apply the input
> one object at a time, this will result in many buffer switches.

Yes, I agree. However, half of those buffer switches are probably
because the language server output would be directed into a buffer;
you could avoid those using a filter function, I suppose.

> > > I wish we had a better alternative.
> >
> > (Such as not calling regular modification hooks for text property changes?)
>
> I thought about that, but I don't think this would be acceptable.

It's certainly not something to be done on the spur of the moment, but
it is something I feel Emacs did wrongly, perhaps because XEmacs did
things differently, if I understand correctly. I'm not sure I'm aware
of even a single place where text properties are used for something
that's integrally part of buffer text.

> > >  Maybe we should warn
> > > in the documentation that calling these functions with BUFFER being
> > > other than the current buffer might hurt performance when
> > > after-change-functions is non-nil.
> >
> > It'll hurt performance even when after-change-functions is nil, so
> > such a warning would be overspecific.
>
> We could avoid switching buffers if the hook is nil, at least in
> principle.  If not, it's even worse than I feared.

We could. I've looked at the code and I think the right thing to do,
when someone has time to test things properly, is to rewrite all
buffer-modifying functions to look like this:

  Lisp_Object hooks = run_before_change_hooks (...);
  modify_buffer ();
  run_after_change_hooks (hooks, ...);

where run_before_change_hooks runs the before-change hooks but
collects the modification hooks to be run after the modification in
the same iteration. Right now, we're using global variables to achieve
something similar, but, among other problems, that means modification
hooks aren't reentrant. Modifying buffer B from buffer A's
modification hooks sounds like it should be safe to me even when B has
modification hooks, but it isn't. (In fact, I don't see why
inhibit-modification-hooks isn't buffer-local).

> > > > As a practical matter, it's hard to change the text property functions
> > > > to use NULL when passed a nil argument
> > >
> > > How is it harder than passing current_buffer?
> >
> > The code path goes through
> >
> >   if (NILP (object))
> >     XSETBUFFER (object, current_buffer);
>
> I meant in the cases where you pass the literal current_buffer.
>
> But even the above is not a problem:
>
>   struct buffer *b;
>   if (NILP (object))
>     {
>       XSETBUFFER (object, current_buffer);
>       b = NULL;
>     }
>   else if (BUFFERP (object))
>     b = XBUFFER (object);
>   [...]
>   signal_after_change (b, ...);

I find the above much less readable than the current version, I must say.

> > It was out of genuine interest, because passing NULL to implicitly
> > specify a default argument is something that people advocate against,
>
> Not to specify the default, but to indicate that no action is needed
> at all wrt the buffer.  It is similar to the last argument to
> 'strtol', for example.

The `base' argument, you mean? If that's what you're saying, I agree
that using 0 as a short-hand for "use implicit base" is an odd
decision for C to have made, but I'm not sure I see the similarity to
the current argument.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Mon, 17 Jun 2019 16:00:02 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> gmail.com>
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument
 calls current buffer's `after-change-functions'
Date: Mon, 17 Jun 2019 18:59:05 +0300
> From: Pip Cet <pipcet <at> gmail.com>
> Date: Mon, 17 Jun 2019 11:38:38 +0000
> Cc: 36190 <at> debbugs.gnu.org
> 
> > Switching buffers means rebinding values of all the buffer-local
> > variables, of which there could be quite a few.  Or am I missing
> > something?
> 
> I just don't see how the requirement to switch buffers for modifying
> text properties is so different, performance-wise, from the case of
> modifying buffer text; in the latter case, we simply accept we can do
> so only for the current buffer.

It isn't different.  It's just that (a) modifying another buffer's
text is relatively rare, and (b) this is one more such switch.

> In any case, the current code already switches buffers, it's just a
> question of doing so twice rather than once.

Yes.  IOW, we get hit by that one more time.

> > > > I wish we had a better alternative.
> > >
> > > (Such as not calling regular modification hooks for text property changes?)
> >
> > I thought about that, but I don't think this would be acceptable.
> 
> It's certainly not something to be done on the spur of the moment, but
> it is something I feel Emacs did wrongly, perhaps because XEmacs did
> things differently, if I understand correctly. I'm not sure I'm aware
> of even a single place where text properties are used for something
> that's integrally part of buffer text.

I don't think this i a part of the problem: applications that don't
want the side effects of text properties can use overlays instead.

> when someone has time to test things properly, is to rewrite all
> buffer-modifying functions to look like this:
> 
>   Lisp_Object hooks = run_before_change_hooks (...);
>   modify_buffer ();
>   run_after_change_hooks (hooks, ...);

I think that'd be a welcome refactoring, if indeed this paradigm
doesn't break in some subtle use case (Emacs internals are frequently
like that).

> >   struct buffer *b;
> >   if (NILP (object))
> >     {
> >       XSETBUFFER (object, current_buffer);
> >       b = NULL;
> >     }
> >   else if (BUFFERP (object))
> >     b = XBUFFER (object);
> >   [...]
> >   signal_after_change (b, ...);
> 
> I find the above much less readable than the current version, I must say.

I guess we will have to disagree then, because this is boilerplate C
ion Emacs sources.

> > > It was out of genuine interest, because passing NULL to implicitly
> > > specify a default argument is something that people advocate against,
> >
> > Not to specify the default, but to indicate that no action is needed
> > at all wrt the buffer.  It is similar to the last argument to
> > 'strtol', for example.
> 
> The `base' argument, you mean?

Sorry, meant the penultimate argument, ENDPTR.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Tue, 18 Jun 2019 17:15:01 GMT) Full text and rfc822 format available.

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

From: Pip Cet <pipcet <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument
 calls current buffer's `after-change-functions'
Date: Tue, 18 Jun 2019 17:14:09 +0000
On Mon, Jun 17, 2019 at 3:58 PM Eli Zaretskii <eliz <at> gnu.org> wrote:
> > From: Pip Cet <pipcet <at> gmail.com>
> > Date: Mon, 17 Jun 2019 11:38:38 +0000
> > Cc: 36190 <at> debbugs.gnu.org
> > > > > I wish we had a better alternative.
> > > >
> > > > (Such as not calling regular modification hooks for text property changes?)
> > >
> > > I thought about that, but I don't think this would be acceptable.
> >
> > It's certainly not something to be done on the spur of the moment, but
> > it is something I feel Emacs did wrongly, perhaps because XEmacs did
> > things differently, if I understand correctly. I'm not sure I'm aware
> > of even a single place where text properties are used for something
> > that's integrally part of buffer text.
>
> I don't think this i a part of the problem: applications that don't
> want the side effects of text properties can use overlays instead.

I've seen performance problems with overlays, and the subtle API
differences made switching, for me, difficult. You're right, though,
that this is certainly not part of this bug report, which I think the
latest patch I sent would fix for now (although I'd be happy to see it
fixed any other way, too).

> > when someone has time to test things properly, is to rewrite all
> > buffer-modifying functions to look like this:
> >
> >   Lisp_Object hooks = run_before_change_hooks (...);
> >   modify_buffer ();
> >   run_after_change_hooks (hooks, ...);
>
> I think that'd be a welcome refactoring, if indeed this paradigm
> doesn't break in some subtle use case (Emacs internals are frequently
> like that).

It would almost certainly be an observable change in behavior, but I'm
testing something and it doesn't seem to fall apart entirely, at
least.

> > > > It was out of genuine interest, because passing NULL to implicitly
> > > > specify a default argument is something that people advocate against,
> > >
> > > Not to specify the default, but to indicate that no action is needed
> > > at all wrt the buffer.  It is similar to the last argument to
> > > 'strtol', for example.
> >
> > The `base' argument, you mean?
>
> Sorry, meant the penultimate argument, ENDPTR.

Thanks again for explaining, I'll think about it more.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Sat, 06 Jul 2019 08:09:02 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> gmail.com>
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument
 calls current buffer's `after-change-functions'
Date: Sat, 06 Jul 2019 11:08:41 +0300
> From: Pip Cet <pipcet <at> gmail.com>
> Date: Sat, 15 Jun 2019 19:27:30 +0000
> Cc: 36190 <at> debbugs.gnu.org
> 
> > Looks better to me, but I'd suggest a comment explaining why we do
> > such an unusual thing.
> 
> Sounds good to me. The attached patch contains the comments and a hint
> in the documentation. (Maybe the comments should be a FIXME, because
> there's really no need to switch buffers at all when there are no
> applicable hooks?)

I'm okay with adding such a FIXME.

> Obviously, feel free to reword as you consider appropriate.

LGTM, with a couple of minor comments below.  Feel free to push after
fixing those.

> +  /* Ensure we run the modification hooks for the right buffer,
> +     without switching buffers twice (bug 36190). */
                                                   ^^
Here and elsewhere please keep 2 spaces between the period and "*/"

> +  if (BUFFERP (object) && XBUFFER (object) != current_buffer)
> +    {
> +      ptrdiff_t count = SPECPDL_INDEX ();
> +      record_unwind_current_buffer ();
> +      set_buffer_internal (XBUFFER (object));
> +      return unbind_to (count, Fremove_list_of_text_properties (start, end,
> +								list_of_properties,
> +								object));
> +    }

The indentation here is too deep; suggest reformatting, e.g., by
inserting a newline before Fremove_list_of_text_properties, and then
reindenting the rest.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36190; Package emacs. (Sat, 06 Jul 2019 15:29:02 GMT) Full text and rfc822 format available.

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

From: Pip Cet <pipcet <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 36190 <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument
 calls current buffer's `after-change-functions'
Date: Sat, 6 Jul 2019 15:27:41 +0000
[Message part 1 (text/plain, inline)]
On Sat, Jul 6, 2019 at 8:08 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
> > Sounds good to me. The attached patch contains the comments and a hint
> > in the documentation. (Maybe the comments should be a FIXME, because
> > there's really no need to switch buffers at all when there are no
> > applicable hooks?)
>
> I'm okay with adding such a FIXME.

Added.


>
> > Obviously, feel free to reword as you consider appropriate.
>
> LGTM, with a couple of minor comments below.  Feel free to push after
> fixing those.

I don't have push access, so I hope it's okay just to send the new patch.

Thanks for the review!
[0001-Update-current-buffer-when-changing-text-properties.patch (text/x-patch, attachment)]

Reply sent to Eli Zaretskii <eliz <at> gnu.org>:
You have taken responsibility. (Sat, 06 Jul 2019 16:23:02 GMT) Full text and rfc822 format available.

Notification sent to Pip Cet <pipcet <at> gmail.com>:
bug acknowledged by developer. (Sat, 06 Jul 2019 16:23:02 GMT) Full text and rfc822 format available.

Message #70 received at 36190-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> gmail.com>
Cc: 36190-done <at> debbugs.gnu.org
Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument
 calls current buffer's `after-change-functions'
Date: Sat, 06 Jul 2019 19:22:32 +0300
> From: Pip Cet <pipcet <at> gmail.com>
> Date: Sat, 6 Jul 2019 15:27:41 +0000
> Cc: 36190 <at> debbugs.gnu.org
> 
> > LGTM, with a couple of minor comments below.  Feel free to push after
> > fixing those.
> 
> I don't have push access, so I hope it's okay just to send the new patch.

Thanks, pushed.  Please in the future mention the bug number in the
log message, and be sure NOT to end with a period the header line of
the log message, per CONTRIBUTE.




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Sun, 04 Aug 2019 11:24:07 GMT) Full text and rfc822 format available.

This bug report was last modified 4 years and 238 days ago.

Previous Next


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