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

[Pip Cet <pipcet@gmail.com>]

On Fri, Jun 14, 2019 at 7:36 AM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Thu, 13 Jun 2019 20:57:08 +0000
> > Cc: 36190@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.