1467127 - [Wayland] Integrate wl_display_dispatch_queue_pending() with compositor thread event loop

Status: RESOLVED FIXED

(Core :: Widget: Gtk, enhancement)

Description

[Martin Stránský [:stransky] (ni? me)]

Integrate wl_display_dispatch_queue_pending() with compositor thread event loop and remove the fixed fps here.

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Some more context from Jonas Adahl:

> I'll try to look at it but I'm afraid I'd need to call the
> wl_display_dispatch_queue_pending() in some fps-based loop anyway.

It is possible to integrate the libwayland-client dispatch code into
other main loops. See gtk+/gdk/wayland/gdkeventsource.c for how it's
done for GLib; I imagine it should be possible to do it in the one used
in Firefox too in a similar way.

> - make sure we don't miss Wayland events (call
> wl_display_dispatch_queue_pending() or use the code you posted).

You should always use the prepare_read/read/dispatch_pending API
otherwise you may end up with frame drops and unnecessary latency as 1)
You rely on the main thread to read the events, 2) You have no way to
know when new events will come.

Comment 2

[Martin Stránský [:stransky] (ni? me)]

We also should call wl_display_dispatch_queue_pending() before getting new buffer as well as elg/drivers/dri2/platform_wayland.c does.

Comment 3

[Kenny Levinsen :kennylevinsen]

Would there be anything wrong with simply dedicating a thread per display to wl_display_dispatch_queue?

Firing it from the compositor, which is to the best of my understanding only running on Vsync, would be troublesome with regards firing composition on frame callbacks (e.g. through a surface-specific vsync source).

Comment 4

[Kenny Levinsen :kennylevinsen]

Attachment: 0001-WIP-poll-queue-on-dedicated-thread.patch

Naïve implementation using a dedicated thread per additional queue, and the relevant wl_proxy magic to get it to work. It appears to work just fine during my quick testing.

This implementation gives us both better response time and lower CPU utilization than the existing busy-loop with sleep approach. A fancier implementation could easily share a thread for all additional queues, polling the display fd's and dispatching as necessary.

One could also use a poll thread to monitor the fd's and dispatch Runnable's with wl_display_dispatch_queue_pending to the owning thread, but this solution is both simpler and cheaper, so if it works, I see no reason to complicate matters.

Note that we need to update the description for EVENT_LOOP_DELAY, which is now only used by WindowSurfaceWayland.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Great, I'll loot at it.

Comment 6

[Kenny Levinsen :kennylevinsen]

Attachment: Bug 1467127 - Run wl_display_dispatch_queue on dedicated thread.

Instead of dispatching the wl_display queues in a non-blocking manner at fixed
intervals, we now instead dispatch display queues blocking on a dedicated
thread.

This ensures that we do not waste cycles checking an empty queue, as well as
granting us immediate response anything put in the queue.

Comment 7

[Kenny Levinsen :kennylevinsen]

Sorry, I had apparently failed to add the bugzilla ID to the phabricator review with the most recent patch.

I thought I did, but oh well. I guess that's an argument for using moz-phab over arc.

Comment 8

[Kenny Levinsen :kennylevinsen]

The attempt at using a dedicated thread caused too many issues. They could of course be solved, but would involve a relatively large amount of work.

Instead, I had an idea: Simply push dispatch_queue_pending tasks from the same spot we service the main queue (nsAppShell::ProcessNextNativeEvent). I believe that will be the least complicated solution, although the nsWaylandDisplay lifetime has to be modified slightly so that we don't try to dispatch to dead compositor/render threads on shutdown.

Patch will be posted soon.

Comment 9

[Kenny Levinsen :kennylevinsen]

Attachment: Bug 1467127 - Post wl_display_dispatch_queue_pending tasks from ProcessNextNativeEvent.

Send tasks to dispatch our other wayland event queues from their respective threads whenever we service the GTK main event loop.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c02b1c12158f
Post wl_display_dispatch_queue_pending tasks from ProcessNextNativeEvent. r=stransky

Comment 11

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/c02b1c12158f