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

[Eli Zaretskii]

> Cc: address@hidden
> From: Daniel Colascione <address@hidden>
> 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.
So I'd prefer to have the test outside of the call, or even outside of
the loop.

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

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

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

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

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

Thanks.