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

[Daniel Colascione <dancol@dancol.org>]

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. Can you elaborate on what you consider a cleaner approach?

> So I'd prefer to have the test outside of the call, or even outside of
> the loop.

See below. I don't think that giving font back-ends an opportunity to do
_something_ in response to a frame being garbaged in certain ways is a
bad thing.

>>  >> +  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.

We need to do a buffer flip at the end of redisplay for each frame on
which update_end was called during redisplay.  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 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, so
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.

>> 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. I
think it's cleaner to have xterm keep track of state only xterm needs.

>>  > Also, should
>>  > we distinguish between visible and iconified frames?
>> 
>> If we do, we should do it in the code that performs the updates, not the 
>> code (the snippet immediately above) that publishes the updates we've 
>> already done.
>
> See above: I don't like such dependencies and find them in general an
> obstacle to understanding the overall logic of the display code.  I
> don't mind adding a test in update_frame and friends, but that
> shouldn't prevent us from having a similar (or even identical) test
> here.

What dependency? You're proposing adding a lot of complexity to the loop
that calls redisplay_end_hook, and I still have no idea what this
complexity is supposed to accomplish.

>
>>  >> +#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.

>
>>  >> +#ifdef HAVE_XDBE
>>  >> +  dpyinfo->supports_xdbe = false;
>>  >> +    {
>>  >> +      int xdbe_major;
>>  >> +      int xdbe_minor;
>>  >> +      if (XdbeQueryExtension (dpyinfo->display, &xdbe_major, 
>> &xdbe_minor))
>>  >> +        dpyinfo->supports_xdbe = true;
>>  >> +    }
>>  >> +#endif
>>  >
>>  > No need for braces here, since we now require a C99 compiler.
>> 
>> If we put xdbe_major and xdbe_minor at function level, the names leak 
>> into function scope. With braces, they exist only around the call to 
>> XdbeQueryExtension
>
> We use that in many other places, so I think these precautions are
> misguided and generally make our coding style less apparent.

Fine.