Bug 137513 – [GLib] Add a simple GMainLoopSource variant

• Status: NEW
• Component: WebKit Misc.

[Zan Dobersek]

Add a simpler GMainLoopSource implementation that creates only one GSource during its lifetime. That GSource then has its ready time updated as required in order for the dispatch to occur.

[Zan Dobersek]

Created attachment 239460 [details]
WIP

This is a work-in-progress, showing the basic idea.

Adds the GMainLoopSource::Simple class which can for now only handle void callbacks. The source has its ready time updated only when a new dispatch is scheduled. The source remains attached to the context throughout its lifetime (which equates the lifetime of the GMainLoopSource::Simple object).

The shared timer and layer flush timer in LayerTreeHostGtk are converted to using GMainLoopSource::Simple as an example.

[Carlos Garcia Campos]

Comment on attachment 239460 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=239460&action=review

This is independent of the split in any case, we would still need cancellables and mutexes for thread safe sources, and we don't need that for sources used by the same thread. Instead of creating a new class, we could create a new GSource that is created on demand when scheduled is called for a non repeating source with no destroy func, for example.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:475
> +    g_source_set_ready_time(m_source.get(), g_get_monotonic_time() + delay.count() * 1000);

My main concern of this approach is that it works because the current glib implementation allows it, but it hasn't been thought to be used this way, I think

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:502
> +    [](GSource*, GSourceFunc callback, gpointer userData) -> gboolean
> +    {
> +        return callback(userData);
> +    },

This will not work for repeating sources that will be marked as destroyed if they return Stop. I think you could call set_ready_time here, and return G_SOURCE_CONTINUE unconditionally here as well to make it clear this source will not be destroyed.

> Source/WebCore/platform/gtk/SharedTimerGtk.cpp:36
> +static void (*sharedTimerFiredFunction)() = nullptr;

I don't think you need to initialize global static vars to 0.

[Zan Dobersek]

(In reply to comment #2)
> > Source/WTF/wtf/gobject/GMainLoopSource.cpp:475
> > +    g_source_set_ready_time(m_source.get(), g_get_monotonic_time() + delay.count() * 1000);
> 
> My main concern of this approach is that it works because the current glib implementation allows it, but it hasn't been thought to be used this way, I think

It's essentially a timeout source with a dynamic interval that's updated on demand. But whatever the case, we can ask about it with GLib developers on their mailing list.

[Carlos Garcia Campos]

Created attachment 239526 [details]
Same optimization on top of patch to split GMainLoopSource

This is how I would apply your optimization on top of the split.

[Carlos Garcia Campos]

Comment on attachment 239460 [details]
WIP

I'm more and more convinced about this approach of a separate class, however, I would not make a subclass, but a different class with a different name. Since in the end it's quite different to GMainLoopSource (it's restricted to non repeating timeout sources, without destroy functions, non thread safe, etc.). It could be something like GPersistentSource, for example. And instead of schedule, I would use fire, for example, and isPending().