Closed Bug 1467127 Opened 2 years ago Closed 7 months ago

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

Categories

(Core :: Widget: Gtk, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox62 --- wontfix
firefox68 --- fixed

People

(Reporter: stransky, Assigned: kennylevinsen)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 2 obsolete files)

Integrate wl_display_dispatch_queue_pending() with compositor thread event loop and remove the fixed fps here.
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.
We also should call wl_display_dispatch_queue_pending() before getting new buffer as well as elg/drivers/dri2/platform_wayland.c does.

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

Blocks: 1531756

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.

Great, I'll loot at it.

Assignee: stransky → nobody

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.

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.

Attachment #9060020 - Attachment is obsolete: true

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.

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

Attachment #9058281 - Attachment is obsolete: true
Keywords: checkin-needed

Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/c02b1c12158f
Post wl_display_dispatch_queue_pending tasks from ProcessNextNativeEvent. r=stransky

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Assignee: nobody → bugzilla
You need to log in before you can comment on or make changes to this bug.