From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: RFC: flicker-free double-buffered Emacs under X11 Date: Fri, 21 Oct 2016 20:43:21 +0300 Message-ID: <831sz9sime.fsf@gnu.org> References: <9e8ad090-a6a0-c807-95ae-7ec7c3f391cb@dancol.org> <83k2d2rssf.fsf@gnu.org> Reply-To: Eli Zaretskii NNTP-Posting-Host: blaine.gmane.org X-Trace: blaine.gmane.org 1477071863 20602 195.159.176.226 (21 Oct 2016 17:44:23 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 21 Oct 2016 17:44:23 +0000 (UTC) Cc: emacs-devel@gnu.org To: Daniel Colascione Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Oct 21 19:44:19 2016 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bxdrZ-0003zr-QL for ged-emacs-devel@m.gmane.org; Fri, 21 Oct 2016 19:44:13 +0200 Original-Received: from localhost ([::1]:33575 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxdrc-0005aR-6K for ged-emacs-devel@m.gmane.org; Fri, 21 Oct 2016 13:44:16 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:55647) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxdqx-0005Z8-1L for emacs-devel@gnu.org; Fri, 21 Oct 2016 13:43:36 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bxdqt-0001nh-UU for emacs-devel@gnu.org; Fri, 21 Oct 2016 13:43:35 -0400 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:54188) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxdqt-0001nY-RB; Fri, 21 Oct 2016 13:43:31 -0400 Original-Received: from 84.94.185.246.cable.012.net.il ([84.94.185.246]:3555 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.82) (envelope-from ) id 1bxdqs-0008NM-8q; Fri, 21 Oct 2016 13:43:30 -0400 In-reply-to: (message from Daniel Colascione on Fri, 21 Oct 2016 04:04:21 -0700) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2001:4830:134:3::e X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:208583 Archived-At: > Cc: emacs-devel@gnu.org > From: Daniel Colascione > 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.