After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 550942 - [patch] Rework of gdkeventloop-quartz.c
[patch] Rework of gdkeventloop-quartz.c
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Backend: Quartz
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-quartz maintainers
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2008-09-05 04:41 UTC by Owen Taylor
Modified: 2008-11-12 13:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch reworking event loop handling (37.17 KB, patch)
2008-09-05 04:41 UTC, Owen Taylor
none Details | Review
Resulting gdkeventloop-quartz.c for convenient reading (29.87 KB, text/plain)
2008-09-05 04:42 UTC, Owen Taylor
  Details
Test program (3.49 KB, text/plain)
2008-09-05 04:43 UTC, Owen Taylor
  Details

Description Owen Taylor 2008-09-05 04:41:05 UTC
The patch that I'll attach here is a major rework of gdkeventloop-quartz.c.

The biggest change in the patch is that it makes use of CFRunLoopObserver
to iterate the GLib main loop when a native OS X run loop is running, as
during a modal operation like window resizing or drag and drop. This means
that windows redraw during resize and DND "drag under" highlighting works.

Another major change is in the communication with the select thread. Rather
than doing synchronous communication where it asks the select thread to
start polling, waits for it to start polling, then after waiting for
events, wakes up the select thread, now it just submits new sets of 
poll descriptors to the select thread asynchronously. 

Doing the thread polling asynchronously is enabled by always doing 
a timeout=0 poll in the main thread first. The timeout 0 poll is needed
in any case to get the behavior of idle functions correct: a low
priority idle function should not fire if there is data waiting on a file 
descriptor.

The asynchronous polling is already a win in reducing context switches,
but the technique becomes even more powerful because we can simply leave
the existing poll going if the new set of file descriptors is the same
as the old one; there are some subtle aspects to this documented in
the comments.

Finally, I added a lot of comments to the existing code as well as the
new code and reordered things a bit, putting the final nail in the
coffin of the usefulness of reviewing this as a diff. (I'll attach
the entire file as well.)

There are also a couple of small changes worth noting:

 * I removed the call to nextEventMatchingMask in gdk_event_prepare();
   I think it was only there by false analogy to the GDK code where
   you have to call XPending() before polling the X file descriptor
   to avoid blocking if there are events in the queue.

 * I removed the code to try and reuse the slot with the dummy
   file descriptor for our wakeup pipe; it was complexity for no
   performance gain. (The GLib poll function used on OS X handles
   -1 fd's fine. This might need rethinking of OS X ever gains a
   native poll() system call.)

 * I made the code always try to pull a Cocoa event, even if the
   dummy poll FD wasn't in the FD set. The dummy poll FD not in
   the set only occurs in the rare case where a higher than default
   sources *has already* said that is ready to dispatch. It should
   be fine to queue up another event in that case too.

One thing to note is that while the Quartz run loop is running, 
GLib timeouts, idles, and IO watches fire and synthetic
events like CONFIGURE events are delivered, but mouse and keyboard 
events are not delivered to GTK+ apps... nothing is pulling them
off the event queue. For window resizing and other "grab" type
applications of manual polling of the event loop this is right;
there may be other situations where we want to deliver all sorts
of events. Investigating run loop modes in detail is likely needed
to figure out exactly what to do.

I'm pretty happy with how this all works... window resizing works
and is pretty smooth, DND is a lot better. It seems robust with
a test program I wrote (will attach), gtk-demo, and my own small 
application. I would not be surprised if there are lurking bugs 
revealed by other larger apps with more Quartz/Cocoa integration.
I've left in the debug prints guarded by a new GDK debug facility
GDK_DEBUG=eventloop.

That all being said, while I think the patch is a good thing and
useful for various reasons, I'm not sure it's the best way to handle 
window resizing. On X, doing layout and repaint during idle during 
window is important because otherwise the window manager can get
arbitrarily far ahead of the application. With Quartz, the frame
is being drawn *by* the application, so doing layout and resize
synchronously in windowDidResize may be best - that will result
in the frame and contents being redrawn as a single unit with
no lag between them. This would require some window system #ifdefs in
gtkwindow.c.
Comment 1 Owen Taylor 2008-09-05 04:41:51 UTC
Created attachment 118071 [details] [review]
Patch reworking event loop handling
Comment 2 Owen Taylor 2008-09-05 04:42:35 UTC
Created attachment 118072 [details]
Resulting gdkeventloop-quartz.c for convenient reading
Comment 3 Owen Taylor 2008-09-05 04:43:05 UTC
Created attachment 118073 [details]
Test program
Comment 4 Bob 2008-09-09 01:14:05 UTC
This looks very interesting.  Do you think this will fix the issues of qtk-quartz losing keypress events when under load?  Here is a description of that problem I submitted to the Imendio mailing list:

KeyPressEvents seem to be handled differently using the X11 backend and the Mac backend. Using X11 ALL keypress events are queued and handled by the main loop when it gets around to them. So if I have a big computation going and hit two key presses, the main loop will see them when the big computation stops and the main loop runs. But with the Mac backend key presses are not be seen by the main loop if they are pressed while a big computation is running. They just disappear. This greatly changes the behavior of my application (making it very hard to use reliable).

Richard Hult was able to confirm this but AFAIK has not been able to fix it.  (I stopped using gtk-quartz because of this issue.)  It sounds like you know a lot about main loop issues and could tackle it.
Comment 5 Richard Hult 2008-09-09 05:53:47 UTC
Trunk should not lose any events, it was fixed a while ago.

Comment 6 Bob 2008-09-09 12:21:25 UTC
(In reply to comment #5)
> Trunk should not lose any events, it was fixed a while ago.
> 

OK.  I'll give that try.  Thanks and sorry for the noise...
Comment 7 Bob 2008-09-09 21:55:34 UTC
(In reply to comment #5)
> Trunk should not lose any events, it was fixed a while ago.
> 

OK.  I've tried the recently released Mono Framework 2.0_3 release candidate and the KeyPressEvent problem described above is still present.  The framework lists its gtk+2 version as 2.15.0-0, so I don't know if you would expect it to be fixed in that version (if not then I will request that the mono folks use a newer version).  Also, the issue with the menubar not being redrawn when returning from fullscreen is also present.  Again thanks and sorry about the noise on this bug report (let me know if it should go off list).
Comment 8 Richard Hult 2008-09-10 09:35:04 UTC
If they call their GTK+ version 2.15 then it's probably pretty old, since the unstable branch was numbered that way for a short while before it was changed to 2.13. So my guess is that the fix is not in their version.
Comment 9 Richard Hult 2008-09-24 11:44:03 UTC
I've tested this patch for a while now and it works fine for all my test apps (and fixes an annoying deadlock in events with gimp too). I plan to commit this to trunk to get some more widespread testing, but it seems no matter what it's better than the current version.

Very nicely commented too, thanks a lot!

Comment 10 Richard Hult 2008-11-12 13:32:01 UTC
Committed to trunk to get this more widely used and tested. So far I have not seen any issues.