Re: RFC: flicker-free double-buffered Emacs under X11

[Eli Zaretskii <eliz@gnu.org>]

> From: Daniel Colascione <dancol@dancol.org>
> Cc: emacs-devel@gnu.org
> Date: Fri, 21 Oct 2016 11:27:29 -0700
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Cc: emacs-devel@gnu.org
> >> From: Daniel Colascione <dancol@dancol.org>
> >> Date: Fri, 21 Oct 2016 04:04:21 -0700
> >> 
> >>  > Also, the call to font_flush_frame_caches is unconditional, although
> >>  > only one font back-end supports it.  That seems to incur a gratuitous
> >>  > overhead of a function call for the other font back-ends.
> >> 
> >> We're testing whether the hook function is non-NULL before calling into 
> >> it, so only that one backend gets the call. In any case, the overhead is 
> >> trivial --- it's one indirect function call compared to all of 
> >> redisplay. (Every call into a shared library is the same indirect jump.)
> >
> > The code is more clear with the test before the call, otherwise the
> > code reader needs to look in the hook to understand when it's a no-op.
> 
> But the test (of whether the hook exists) _is_ before the call. What are
> you proposing?
> 
>     some_backend_needs_flush = False
>     for backend in font_back_ends:
>       if has_flush_hook(backend):
>         some_back_end_needs_flush = True
>     if some_back_end_needs_flush:
>       for backend in font_back_ends:
>         if has_flush_hook(backend):
>           backend.flush_hook()
>     
> That's silly.

Let's assume that neither of us makes silly proposals, okay?

> Can you elaborate on what you consider a cleaner approach?

I meant the test for the lone font backend that needs this kludge, not
for the existence of the method.  And also a comment that tells it's a
kludge whose need we understand only empirically.  I'd even prefer a
direct call to the function when the font backend is the one which
needs it, rather than having a method created for the benefit of a
single backend, whose necessity we don't really understand.

When I read the code which tests whether a backend has a method, I
need to look up the methods of the backend(s) I'm interested in to
know whether they have such a method.  When a method is defined for a
single backend, most of those searches will produce a trivial result,
i.e. a waste of my time.  I prefer that we make this evident at the
place of the call instead.

> >>  >> +  FOR_EACH_FRAME (tail, frame)
> >>  >> +    {
> >>  >> +      struct frame *f = XFRAME (frame);
> >>  >> +      if (FRAME_TERMINAL (f)->redisplay_end_hook)
> >>  >> +        (*FRAME_TERMINAL (f)->redisplay_end_hook) (f);
> >>  >> +    }
> >>  >
> >>  > This will call the hook for each frame, every redisplay cycle.  By
> >>  > contrast, redisplay_internal many times updates only the selected
> >>  > window of the selected frame.  Is calling the hook for all the frames
> >>  > really needed, or should we only call it for the selected frame in the
> >>  > latter case, or maybe just for frames that got updated?
> >> 
> >> The hook only does something in the case where someone called update_end 
> >> and we demurred on actually flipping the buffer because we knew we were 
> >> in redisplay and would be getting redisplay_end_hook shortly. That is, 
> >> if we update only one frame, we're only going to do one buffer flip.
> >
> > That might be so, but what you say above is in no way apparent from
> > looking at the code.  IME, it is very important to have the idea when
> > something is being done and when not just by looking at the code in
> > redisplay_internal; having to find that out by realizing that some
> > flag is set in update_end and then tested in the hook makes the code
> > more subtle and its maintenance harder.  It's not like keeping this
> > detail from redisplay_internal makes this detail local to some
> > functions, or somesuch, so there's really no reason to conceal it,
> > IMO.
> 
> This sentiment strikes me as being analogous to the old "no use of hooks
> in Emacs internals" line --- yet we have facilities like syntax-ppss
> that rely on hooks in core, and it's worked out fine.

I'm not saying the code won't work.  I'm saying it could make its
purpose more evident, which is better for long-term maintainability.

> We need to do a buffer flip at the end of redisplay for each frame on
> which update_end was called during redisplay.

Even if update_end was called for a frame whose update was "paused",
i.e. whose redisplay was interrupted by incoming input?

> If someone calls update_end _outside_ redisplay, we should do a
> buffer flip immediately. The code I've sent is the cleanest way of
> implementing this model short of changing how update_begin and
> update_end work.

I'm asking whether flipping the buffer for a frame that wasn't updated
or was updated partially can do some harm, like produce incorrect
display.

> I think thhat what you're proposing is a layering violation. It will
> make maintenance harder. The only facility that cares about the
> has-a-frame-been-updated state is the X11 double-buffered back-end,

My problems start with the name of the hook, which doesn't hint at all
that only double-buffered X11 back-end cares about that.  If the hook
was called something like double_buffer_flip_hook, or was compiled
only if HAVE_XDBE is defined, this issue would go away.

> making xdisp track this state makes everything more complicated,
> especially because xdisp already has a "needs redisplay" flag and
> shouldn't need to keep track of a separate "needs buffer flip" flag.
> It shouldn't have to care. That's not its job.

I don't know why you decided that it isn't xdisp.c's job.  It already
has a lot of flags that other parts set to get xdisp.c do or not do
certain parts of its job.  Why not let it test a flag here as well?
Such a test will tell the reader that the call to the hook is only
done when a flag (with hopefully some descriptive name) is set, so the
reader can understand what the hook is doing and under what
circumstances without actually looking at the hook, which involves
first finding out which frame types support it, and what exactly does
it do.

IOW, if the code speaks for itself, it makes maintenance of this
hyper-convoluted piece of Emacs easier, less time consuming, and less
error prone.

> >> Or are you worried about the function call overhead? That, as I 
> >> mentioned above, is trivial.
> >
> > No, I worry about maintainability of the display code and about
> > lowering the risk of bugs introduced due to such subtleties.
> >
> 
> I'm also worried about maintainability: that's why I don't want to make
> redisplay_internal any more of a big ball of mud than it already is.

It's too late for that.  It's already an extremely complex and hard to
understand piece of code.  Hiding more information from its logic
makes the code harder to maintain, not easier.  We are not talking
about highly-modular package, where the logic is kept local to each
module, and the interfaces between them are kept to the absolute
minimum.  xdisp.c _knows_ a lot about how xterm.c and xfns.c work.
You cannot disengage them without a complete rewrite.

So the principles you are trying to apply, while a good idea in
general, in this particular case make code harder to understand and
develop.

Of course, it's not a catastrophe, so if you are going to go to the
barricades over this, I won't fight you.  I just hope you take my gray
hair from many readings of redisplay_internal as some evidence that I
know what I'm talking about.  The number of times I needed to go deep
into the called functions just to realize that the code does something
completely different from what it looked on the redisplay_internal
level is beyond imagination.

> >>  >> +#ifdef HAVE_XDBE
> >>  >> +  if (FRAME_DISPLAY_INFO (f)->supports_xdbe)
> >>  >> +    {
> >>  >> +      /* If allocating a back buffer fails, just use single-buffered
> >>  >> +         rendering.  */
> >>  >> +      x_sync (f);
> >>  >> +      x_catch_errors (FRAME_X_DISPLAY (f));
> >>  >> +      FRAME_X_DRAWABLE (f) = XdbeAllocateBackBufferName (
> >>  >> +        FRAME_X_DISPLAY (f),
> >>  >> +        FRAME_X_WINDOW (f),
> >>  >> +        XdbeCopied);
> >>  >> +      if (x_had_errors_p (FRAME_X_DISPLAY (f)))
> >>  >> +        FRAME_X_DRAWABLE (f) = FRAME_X_WINDOW (f);
> >>  >
> >>  > Shouldn't we turn off the supports_xdbe flag in the case of failure?
> >> 
> >> supports_xdbe is whether XDBE is supported on a connection at all. What 
> >> if XdbeAllocateBackBufferName fails transiently?
> >
> > How can it fail transiently?  And why turning off supports_xdbe in
> > that case would mean trouble?
> 
> It's an allocation. Allocations can fail. And XDBE isn't guaranteed to
> work for all visuals.

How much memory is being allocated, and correspondingly what is the
probability this allocation could fail on a healthy system?

And what about my second question?

The downside of your proposed code is that we will try to turn on XDBE
each time hence, and each attempt means a potentially costly series of
calls to catch X errors, and the XdbeAllocateBackBufferName itself.
I'm asking whether it wouldn't be better to simply give up on XDBE in
such a case for that frame.  It could be much cheaper, no?  Just a
thought.