Bug 138691 – [GTK] Add persistent GSource wrapper

• Component: Web Template Framework
• Status: RESOLVED WONTFIX

[Zan Dobersek]

Unlike GMainLoopSource or GThreadSafeLoopSource, the new class will only manage one GSource object during its lifetime. This avoids attaching and scheduling new GSource objects every time a new scheduling is performed, which can hinder performance in case of highly frequent task scheduling.

The first iteration of this work won't yet provide thread-safe implementation.

A couple of GMainLoopSource objects I'd like to convert for a start, listed by their name:
[WebKit] layerFlushTimer - schedules layer flushing, is scheduled up to 60 times per second
[WebKit] sharedTimerTimeoutCallback - schedules shared timer, is scheduled up to 250 times per second
[WebKit] RunLoop work - scheduled during IPC processing in WK2
[WebKit] RunLoop::Timer - scheduled via the ResponsivenessTimer in WK2
[WebKit] WorkQueue::dispatch - scheduled during IPC processing in WK2

[Zan Dobersek]

Created attachment 241857 [details]
WIP

Work in progress.

[Carlos Garcia Campos]

Comment on attachment 241857 [details]
WIP

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

I got lost with all the templates mess :-P Could you explain what the templates are for and what's the difference between the static and dynamic thing?

> Source/WTF/wtf/gobject/GPersistentMainLoopSource.cpp:42
> +        return callback(data);

The callback return value is ignored, because always want to continue here. I would move the g_source_set_ready_time() here and the return continue.

g_source_set_ready_time(source, -1);
callback(data);
return G_SOURCE_CONTINUE;

> Source/WTF/wtf/gobject/GPersistentMainLoopSource.cpp:50
> +gboolean GPersistentMainLoopSource::Base<GPersistentMainLoopSource::StaticType>::sourceCallback(Base* source)

And this would be a void function that simply calls source->m_function(); setting the status accordingly.

> Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:111
>  void WorkQueue::dispatch(std::function<void ()> function)

Is this called that often? have you taken any measurements? In this particular case the code is more complex, adding a queue and a mutex.

[Zan Dobersek]

Comment on attachment 241857 [details]
WIP

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

>> Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:111
>>  void WorkQueue::dispatch(std::function<void ()> function)
> 
> Is this called that often? have you taken any measurements? In this particular case the code is more complex, adding a queue and a mutex.

This is used extensively in IPC handling. including handling all the input events, which there can be a lot of.

The mutex is required, but overall using a persistent GSource avoids locking the GMainContext's mutex every time the source is detached and destroyed.

[Zan Dobersek]

(In reply to comment #2)
> Comment on attachment 241857 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241857&action=review
> 
> I got lost with all the templates mess :-P Could you explain what the
> templates are for and what's the difference between the static and dynamic
> thing?
> 

This exercise in templates was interesting, but not necessary. The next iteration of the patch doesn't use them at all.

The difference between the static and dynamic type of the GSource wrapper is that the static one has its callback defined when it is initialized, while the dynamic one has the callback defined every time it's scheduled.

[Zan Dobersek]

Created attachment 242249 [details]
WIP #2

[Zan Dobersek]

Created attachment 242250 [details]
Proper WIP #2

[Zan Dobersek]

(In reply to comment #6)
> Created attachment 242250 [details]
> Proper WIP #2

This approach avoids storing the callback function in the GSource wrapper and instead pushes the callback onto the heap and uses it as the direct callback that's to be called from the GSource dispach function.

The static and dynamic types are preserved. Given that scheduling and cancelling is now done directly on the GSource object, we can now rely on thread safety as provided by the GMainContext to which the certain GSource object is attached.

The repeating callback (i.e. the bool() callback) is supported in the dynamic wrapper, but I think it should be implemented in a separate type of the wrapper. GSocket callback should also be handled via that type.

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #6)
> > Created attachment 242250 [details]
> > Proper WIP #2
> 
> This approach avoids storing the callback function in the GSource wrapper
> and instead pushes the callback onto the heap and uses it as the direct
> callback that's to be called from the GSource dispach function.
> 
> The static and dynamic types are preserved. Given that scheduling and
> cancelling is now done directly on the GSource object, we can now rely on
> thread safety as provided by the GMainContext to which the certain GSource
> object is attached.
> 
> The repeating callback (i.e. the bool() callback) is supported in the
> dynamic wrapper, but I think it should be implemented in a separate type of
> the wrapper. GSocket callback should also be handled via that type.

Do we really need the repeating callback? GSocket sources are usually persistent by themselves, because the same source is used all the time, instad of being re-scheduled using a new source. I think this persisten wrapper should be as simple as possible and to handle only the caes where we need it, instead of the GMainLoopSource that is mor a general purpose wrapper.

[Carlos Garcia Campos]

Why the change name btw? GSourceWrap sounds too generic to me for something so specific. I liked the GPersisentMainLoopSource, but I agree it's too long even for a WebKit class :-)

[Zan Dobersek]

(In reply to comment #9)
> Why the change name btw? GSourceWrap sounds too generic to me for something
> so specific. I liked the GPersisentMainLoopSource, but I agree it's too long
> even for a WebKit class :-)

At the point of writing GSourceWrap I think I already had three different classes in WTF that were handling GSource. I started from scratch, so I used a new name as well.

But it should definitely change, maybe back to GMainLoopSource once we can streamline all the use-cases.

[Zan Dobersek]

Created attachment 254639 [details]
WIP

Updated GSourceWrap implementation, with converted uses of GMainLoopSource in WTF and WebCore.

[WebKit Commit Bot]

Attachment 254639 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/gtk/SharedTimerGtk.cpp:42:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:80:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:81:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:91:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:92:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:98:  The parameter name "function" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:98:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:98:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:102:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:108:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:113:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:129:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:136:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:5:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:14:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:23:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:34:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:50:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:67:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:167:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:172:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:177:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:198:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:201:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:206:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:209:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:223:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:231:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:239:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:250:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:277:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1052:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/glib/MainThreadGLib.cpp:44:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WTF/wtf/glib/WorkQueueGLib.cpp:95:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/glib/WorkQueueGLib.cpp:124:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 36 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

It needs to be added to PlatformEFL.cmake:

[3179/3277] Linking CXX shared library lib/libewebkit2.so.1.11.0
FAILED: : && /usr/lib/ccache/c++  -fPIC  -std=c++11 -O3 -DNDEBUG -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-rtti  -Wl,--no-undefined  -L/home/gyuyoung/eflews/WebKit/WebKitBuild/DependenciesEFL/Root/lib  -fuse-ld=gold -Wl,--disable-new-dtags -fuse-ld=gold -Wl,--disable-new-dtags  -Wl,--gc-sections -shared -Wl,-soname,libewebkit2.so.1 -o lib/libewebkit2.so.1.11.0 @CMakeFiles/WebKit2.rsp  && :
lib/libwebcore_efl.a(lib/../Source/WebCore/CMakeFiles/WebCore.dir/platform/network/soup/ResourceHandleSoup.cpp.o):ResourceHandleSoup.cpp:function WebCore::cleanupSoupRequestOperation(WebCore::ResourceHandle*, bool): error: undefined reference to 'WTF::GSourceWrap::Static::cancel()'
lib/libwebcore_efl.a(lib/../Source/WebCore/CMakeFiles/WeFailed to run "['Tools/Scripts/build-webkit', '--release', '--efl', '--update-efl', '--makeargs="-j8"']" exit_code: 1

Why do you prefer reinterpret_cast to static_cast when casting from a void*? I thought standard convention was to use static_cast.

Most importantly, the header file is fairly intimidating, so it would be good to have some documentation for the public classes Static, Dynamic, OneShot, Socket, and Queue. E.g. "The difference between the static and dynamic type of the GSource wrapper is that the static one has its callback defined when it is initialized, while the dynamic one has the callback defined every time it's scheduled" is obvious to me now that I've read the comment where you point it out, and I can see the std::function parameters are in different places, but I didn't notice that at first and was left thinking "what's the difference?"

[Carlos Garcia Campos]

(In reply to comment #13)
> Most importantly, the header file is fairly intimidating, so it would be
> good to have some documentation for the public classes Static, Dynamic,
> OneShot, Socket, and Queue.

Yes, or try to re-think the API to make it simpler. I'll try to find some time to review this and think about the API.

[Zan Dobersek]

Will update PlatformEFL.cmake later today.

I as the author of course don't see a better possible API, not until I get some review critique. I should be able to add comments.

I'll go and convert the rest of G(ThreadSafe)MainLoopSource uses to GSourceWrap as well, to showcase the new API better (and to test it out). Also, unit tests are required, but I haven't gotten around to writing those yet. Same for ChangeLogs.

[Carlos Garcia Campos]

(In reply to comment #15)
> Will update PlatformEFL.cmake later today.
> 
> I as the author of course don't see a better possible API, not until I get
> some review critique. I should be able to add comments.

Sure, maybe there's no better API :-)

> I'll go and convert the rest of G(ThreadSafe)MainLoopSource uses to
> GSourceWrap as well, to showcase the new API better (and to test it out).
> Also, unit tests are required, but I haven't gotten around to writing those
> yet. Same for ChangeLogs.

[Zan Dobersek]

Created attachment 254662 [details]
Patch

Updated patch, adds the new file to the EFL build and converts most G(ThreadSafe)MainLoopSource cases to GSourceWrap.

[WebKit Commit Bot]

Attachment 254662 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:80:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:81:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:91:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:92:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:98:  The parameter name "function" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:98:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:98:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:102:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:108:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:113:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:129:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.h:136:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/gtk/TestRunnerGtk.cpp:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:5:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:14:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:23:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:34:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:50:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:67:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:167:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:172:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:177:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:198:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:201:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:206:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:209:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:223:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:231:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:239:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:250:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/gobject/GSourceWrap.cpp:277:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/gtk/SharedTimerGtk.cpp:42:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1052:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/glib/MainThreadGLib.cpp:44:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WTF/wtf/glib/WorkQueueGLib.cpp:95:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/glib/WorkQueueGLib.cpp:124:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/gtk/GestureController.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 40 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

I'm not sure this is equivalent to GMainLoopSource, or at least the idea that I had when I wrote it. I decided to write GMainLoopSource after fixing several common mistakes over and over again for the same usage patterns of GSource. The common use was something like this:

static gboolean sourceCallback(gpointer userData)
{
    Foo* foo = static_cast<Foo*>(userData);
    foo->bar();
    return FALSE;
}

Foo::bar()
{
    m_sourceID = 0;
    ....
}

Foo::~Foo()
{
    if (m_sourceID)
        g_source_remove(m_sourceID);
}

if (m_sourceID)
    g_source_remove(m_sourceID);
m_sourceID = g_idle_add_full(G_PRIORITY_DEFAULT, sourceCallback, g_ojbect_ref(something), g_object_unref);

And some variant of this, like not cancelling the previous one, or not tracking the id and use g_idle directly, only schedule if there isn't one already, etc.

The most common mistakes of this pattern were:

 - Not cancelling the previous source ("leaking" the previous id), before scheduling a new one. That's why GMainLoopSource does a cancel() before scheduoling a new source, and has a isScheduled() for the cases when you don't want to cancel the previous one.
 - Not cancelling the source in the destructor. GMainLoopSource does automatic cleanup on its destructor.
 - Not setting the id to 0 in the callback. GMainLoopSource updates its internal state in the callbacks.

But also took advantage of C++ to make its use more convenient:

 - Use lambda functions, instead of having to define static cabllbacks.
 - Automatic detection of repeating/non-repeating functions. It's also common to forget the return FALSE, but in this case at least the compiler warns you.
 - Default values for the common parameters like priority, context, etc.
 - Destroy function to do cleanups.

Looking at your patch I'm not sure all these cases are covered, like for example the automatic cancellation before scheduling a new source.

[Zan Dobersek]

(In reply to comment #19)
> The most common mistakes of this pattern were:
> 
>  - Not cancelling the previous source ("leaking" the previous id), before
> scheduling a new one. That's why GMainLoopSource does a cancel() before
> scheduoling a new source, and has a isScheduled() for the cases when you
> don't want to cancel the previous one.

GSourceWrap only ever operates on one GSource object, instead of creating, attaching and scheduling a new GSource object every time a new invocation of callback is required.
'Cancelling' a GSource object controlled by GSourceWrap is as simple as setting the ready time to -1, ensuring it's not to be fired.

>  - Not cancelling the source in the destructor. GMainLoopSource does
> automatic cleanup on its destructor.

Same for GSourceWrap.

>  - Not setting the id to 0 in the callback. GMainLoopSource updates its
> internal state in the callbacks.

GSourceWrap doesn't use source IDs at all.

> 
> But also took advantage of C++ to make its use more convenient:
> 
>  - Use lambda functions, instead of having to define static cabllbacks.

Same for GSourceWrap.

>  - Automatic detection of repeating/non-repeating functions. It's also
> common to forget the return FALSE, but in this case at least the compiler
> warns you.

GSourceWrap::Dynamic enables that, though it's not used anywhere (at the moment). I'm not a fan of supporting repeatable functions in GSourceWrap (or of any use case that would require GSourceWrap::Dynamic, for that matter), mostly because I'm not satisfied with how it's implemented and because I'm not sure there's a better way to do it.

For instance, RunLoop::TimerBase uses a repeatable GMainLoopSource in upstream, with a std::function<bool()> callback. The patch switches to using GSourceWrap::Static, with the GSource wrapper being rescheduled manually if required.

>  - Default values for the common parameters like priority, context, etc.

Same for GSourceWrap.

>  - Destroy function to do cleanups.

In most cases, if not all, these destroy functions just deref a ref-counted object that's used in the main callback. A much better approach would be to just capture a RefPtr<> into the main lambda and do away with destroy functions -- which GSourceWrap does. 

> 
> Looking at your patch I'm not sure all these cases are covered, like for
> example the automatic cancellation before scheduling a new source.

[Michael Catanzaro]

(In reply to comment #20)
> 'Cancelling' a GSource object controlled by GSourceWrap is as simple as
> setting the ready time to -1, ensuring it's not to be fired.

Maybe an explicit cancel() function would be better?

[Zan Dobersek]

(In reply to comment #21)
> (In reply to comment #20)
> > 'Cancelling' a GSource object controlled by GSourceWrap is as simple as
> > setting the ready time to -1, ensuring it's not to be fired.
> 
> Maybe an explicit cancel() function would be better?

There's already GSourceWrap::Static::cancel(), GSourceWrap::Dynamic::cancel() and GSourceWrap::Socket::cancel(), if that's what you're thinking of.

[Carlos Garcia Campos]

(In reply to comment #20)
> (In reply to comment #19)
> > The most common mistakes of this pattern were:
> > 
> >  - Not cancelling the previous source ("leaking" the previous id), before
> > scheduling a new one. That's why GMainLoopSource does a cancel() before
> > scheduoling a new source, and has a isScheduled() for the cases when you
> > don't want to cancel the previous one.
> 
> GSourceWrap only ever operates on one GSource object, instead of creating,
> attaching and scheduling a new GSource object every time a new invocation of
> callback is required.
> 'Cancelling' a GSource object controlled by GSourceWrap is as simple as
> setting the ready time to -1, ensuring it's not to be fired.

Yes, that was probably what made GMainLoopSource more complex than I expected, so I like the idea of reusing the source.

> >  - Not cancelling the source in the destructor. GMainLoopSource does
> > automatic cleanup on its destructor.
> 
> Same for GSourceWrap.
> 
> >  - Not setting the id to 0 in the callback. GMainLoopSource updates its
> > internal state in the callbacks.
> 
> GSourceWrap doesn't use source IDs at all.
> 
> > 
> > But also took advantage of C++ to make its use more convenient:
> > 
> >  - Use lambda functions, instead of having to define static cabllbacks.
> 
> Same for GSourceWrap.
> 
> >  - Automatic detection of repeating/non-repeating functions. It's also
> > common to forget the return FALSE, but in this case at least the compiler
> > warns you.
> 
> GSourceWrap::Dynamic enables that, though it's not used anywhere (at the
> moment). I'm not a fan of supporting repeatable functions in GSourceWrap (or
> of any use case that would require GSourceWrap::Dynamic, for that matter),
> mostly because I'm not satisfied with how it's implemented and because I'm
> not sure there's a better way to do it.
> 
> For instance, RunLoop::TimerBase uses a repeatable GMainLoopSource in
> upstream, with a std::function<bool()> callback. The patch switches to using
> GSourceWrap::Static, with the GSource wrapper being rescheduled manually if
> required.
> 
> >  - Default values for the common parameters like priority, context, etc.
> 
> Same for GSourceWrap.
> 
> >  - Destroy function to do cleanups.
> 
> In most cases, if not all, these destroy functions just deref a ref-counted
> object that's used in the main callback. A much better approach would be to
> just capture a RefPtr<> into the main lambda and do away with destroy
> functions -- which GSourceWrap does. 

Yes, that was not possible with GMainLoopSource, I think, the lambda could be freed to early, but I don't remember exactly why we had to add explicit destroy functions.

> > 
> > Looking at your patch I'm not sure all these cases are covered, like for
> > example the automatic cancellation before scheduling a new source.

[Zan Dobersek]

Created attachment 256302 [details]
Patch

Added some documentation, removed GSourceWrap::Dynamic, some minor fixes, still lacks ChangeLogs.

[WebKit Commit Bot]

Attachment 256302 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/gtk/TestRunnerGtk.cpp:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/gtk/SharedTimerGtk.cpp:42:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1058:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/glib/GSourceWrap.cpp:5:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/glib/GSourceWrap.cpp:14:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WTF/wtf/glib/GSourceWrap.cpp:23:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/glib/GSourceWrap.cpp:85:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/glib/GSourceWrap.cpp:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/glib/GSourceWrap.cpp:102:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/glib/GSourceWrap.cpp:145:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/glib/GSourceWrap.cpp:153:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/glib/GSourceWrap.cpp:162:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/glib/GSourceWrap.cpp:173:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/glib/GSourceWrap.cpp:201:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/glib/MainThreadGLib.cpp:44:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WTF/wtf/glib/WorkQueueGLib.cpp:95:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/glib/WorkQueueGLib.cpp:124:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/glib/GSourceWrap.h:55:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/glib/GSourceWrap.h:56:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/glib/GSourceWrap.h:79:  The parameter name "function" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/glib/GSourceWrap.h:79:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/glib/GSourceWrap.h:79:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/glib/GSourceWrap.h:83:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/glib/GSourceWrap.h:91:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/glib/GSourceWrap.h:96:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/glib/GSourceWrap.h:114:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/glib/GSourceWrap.h:121:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/gtk/GestureController.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 31 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 263833 [details]
Different approach

Instead of creating a new class that works for all possible cases, I think we can implement the RunLoop using a custom persistent source, and use the RunLoop for the cases when we need to schedule tasks with the default priority, and used very often. For the cases that are not used that often like WorjQueue::dispatchAfter we can keep using GMainLoopSource. And for the cases where we need different priorities we can use a custom implementation like I did in bug #150392. For the shared timer we can do the same. For now I have switched to use this in the WorkQueue, and scheduleDispatchFunctionsOnMainThread(), but we could use it in other cases too.
What do you think? I think it's a simpler and compatible approach.

[WebKit Commit Bot]

Attachment 263833 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/glib/WorkQueueGLib.cpp:116:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/glib/WorkQueueGLib.cpp:123:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 263834 [details]
Try to fix EFL build

[WebKit Commit Bot]

Attachment 263834 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/glib/WorkQueueGLib.cpp:116:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/glib/WorkQueueGLib.cpp:123:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

I'm thinking that for the cases where we need to se a different priority we could probably add RunLoop::Timer::setPriority() and use RunLoop::Timer. I think it would work at least for the shared timer and the layer flush.

[Zan Dobersek]

(In reply to comment #26)
> Created attachment 263833 [details]
> Different approach
> 
> Instead of creating a new class that works for all possible cases, I think
> we can implement the RunLoop using a custom persistent source, and use the
> RunLoop for the cases when we need to schedule tasks with the default
> priority, and used very often. For the cases that are not used that often
> like WorjQueue::dispatchAfter we can keep using GMainLoopSource. And for the
> cases where we need different priorities we can use a custom implementation
> like I did in bug #150392. For the shared timer we can do the same. For now
> I have switched to use this in the WorkQueue, and
> scheduleDispatchFunctionsOnMainThread(), but we could use it in other cases
> too.
> What do you think? I think it's a simpler and compatible approach.

This is tangential to the previous work and should be uploaded into a separate bug.

The idea sounds good, but I don't like WorkQueue switching over to using RunLoop.

[Carlos Garcia Campos]

(In reply to comment #31)
> (In reply to comment #26)
> > Created attachment 263833 [details]
> > Different approach
> > 
> > Instead of creating a new class that works for all possible cases, I think
> > we can implement the RunLoop using a custom persistent source, and use the
> > RunLoop for the cases when we need to schedule tasks with the default
> > priority, and used very often. For the cases that are not used that often
> > like WorjQueue::dispatchAfter we can keep using GMainLoopSource. And for the
> > cases where we need different priorities we can use a custom implementation
> > like I did in bug #150392. For the shared timer we can do the same. For now
> > I have switched to use this in the WorkQueue, and
> > scheduleDispatchFunctionsOnMainThread(), but we could use it in other cases
> > too.
> > What do you think? I think it's a simpler and compatible approach.
> 
> This is tangential to the previous work and should be uploaded into a
> separate bug.

Not really, because with this we don't need the new wrapper to use persistent sources. The Queue wrapper, for example, is mostly duplicating the queue used by the Runloop.

> The idea sounds good, but I don't like WorkQueue switching over to using
> RunLoop.

Could you elaborate? What's what you don't like? With this approach, I think the threaded compositor could simply use a WorkQueue instead of the current custom implementation, for example.

[Zan Dobersek]

(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #26)
> > > Created attachment 263833 [details]
> > > Different approach
> > > 
> > > Instead of creating a new class that works for all possible cases, I think
> > > we can implement the RunLoop using a custom persistent source, and use the
> > > RunLoop for the cases when we need to schedule tasks with the default
> > > priority, and used very often. For the cases that are not used that often
> > > like WorjQueue::dispatchAfter we can keep using GMainLoopSource. And for the
> > > cases where we need different priorities we can use a custom implementation
> > > like I did in bug #150392. For the shared timer we can do the same. For now
> > > I have switched to use this in the WorkQueue, and
> > > scheduleDispatchFunctionsOnMainThread(), but we could use it in other cases
> > > too.
> > > What do you think? I think it's a simpler and compatible approach.
> > 
> > This is tangential to the previous work and should be uploaded into a
> > separate bug.
> 
> Not really, because with this we don't need the new wrapper to use
> persistent sources. The Queue wrapper, for example, is mostly duplicating
> the queue used by the Runloop.
> 

I'm proposing switching all the sources over to the persistent mode using a few generic classes that are built on top of the GLib API. You're for now changing two sources, falling back to using the GLib API directly.

Yep, a Queue object keeps a queue of functions, just like RunLoop, but doesn't care about nested run loops or control over the main loop. It was tailored for WorkQueue.

> > The idea sounds good, but I don't like WorkQueue switching over to using
> > RunLoop.
> 
> Could you elaborate? What's what you don't like? With this approach, I think
> the threaded compositor could simply use a WorkQueue instead of the current
> custom implementation, for example.

TC could likely use WorkQueue instead of RunLoop without the WorkQueue implementation using RunLoop. If that's not the case, then a RunLoop shoul be used instead.

[Carlos Garcia Campos]

(In reply to comment #33)
> (In reply to comment #32)
> > (In reply to comment #31)
> > > (In reply to comment #26)
> > > > Created attachment 263833 [details]
> > > > Different approach
> > > > 
> > > > Instead of creating a new class that works for all possible cases, I think
> > > > we can implement the RunLoop using a custom persistent source, and use the
> > > > RunLoop for the cases when we need to schedule tasks with the default
> > > > priority, and used very often. For the cases that are not used that often
> > > > like WorjQueue::dispatchAfter we can keep using GMainLoopSource. And for the
> > > > cases where we need different priorities we can use a custom implementation
> > > > like I did in bug #150392. For the shared timer we can do the same. For now
> > > > I have switched to use this in the WorkQueue, and
> > > > scheduleDispatchFunctionsOnMainThread(), but we could use it in other cases
> > > > too.
> > > > What do you think? I think it's a simpler and compatible approach.
> > > 
> > > This is tangential to the previous work and should be uploaded into a
> > > separate bug.
> > 
> > Not really, because with this we don't need the new wrapper to use
> > persistent sources. The Queue wrapper, for example, is mostly duplicating
> > the queue used by the Runloop.
> > 
> 
> I'm proposing switching all the sources over to the persistent mode using a
> few generic classes that are built on top of the GLib API. You're for now
> changing two sources, falling back to using the GLib API directly.

Yes, the idea is to only use persistent sources for the ones that would have an impact in performance. This approach allows that without adding new complex API.

> Yep, a Queue object keeps a queue of functions, just like RunLoop, but
> doesn't care about nested run loops or control over the main loop. It was
> tailored for WorkQueue.

WorkQueues never use nested main loops, so it doesn't have any effect when using Runloop in WorkQueue, it simply creates the GMainLoop, and uses the functions queue to dispatch sources. 

> > > The idea sounds good, but I don't like WorkQueue switching over to using
> > > RunLoop.
> > 
> > Could you elaborate? What's what you don't like? With this approach, I think
> > the threaded compositor could simply use a WorkQueue instead of the current
> > custom implementation, for example.
> 
> TC could likely use WorkQueue instead of RunLoop without the WorkQueue
> implementation using RunLoop. If that's not the case, then a RunLoop shoul
> be used instead.

Yes, that's right. I still don't see what's wrong using RunLoop in WorkQueue.

[Zan Dobersek]

(In reply to comment #34)
> (In reply to comment #33)
> > (In reply to comment #32)
> > > (In reply to comment #31)
> > > > (In reply to comment #26)
> > > > > Created attachment 263833 [details]
> > > > > Different approach
> > > > > 
> > > > > Instead of creating a new class that works for all possible cases, I think
> > > > > we can implement the RunLoop using a custom persistent source, and use the
> > > > > RunLoop for the cases when we need to schedule tasks with the default
> > > > > priority, and used very often. For the cases that are not used that often
> > > > > like WorjQueue::dispatchAfter we can keep using GMainLoopSource. And for the
> > > > > cases where we need different priorities we can use a custom implementation
> > > > > like I did in bug #150392. For the shared timer we can do the same. For now
> > > > > I have switched to use this in the WorkQueue, and
> > > > > scheduleDispatchFunctionsOnMainThread(), but we could use it in other cases
> > > > > too.
> > > > > What do you think? I think it's a simpler and compatible approach.
> > > > 
> > > > This is tangential to the previous work and should be uploaded into a
> > > > separate bug.
> > > 
> > > Not really, because with this we don't need the new wrapper to use
> > > persistent sources. The Queue wrapper, for example, is mostly duplicating
> > > the queue used by the Runloop.
> > > 
> > 
> > I'm proposing switching all the sources over to the persistent mode using a
> > few generic classes that are built on top of the GLib API. You're for now
> > changing two sources, falling back to using the GLib API directly.
> 
> Yes, the idea is to only use persistent sources for the ones that would have
> an impact in performance. This approach allows that without adding new
> complex API.
> 

What's complex in GSourceWrap interfaces?
And what's complex in GSourceWrap interfaces that's not complex in GMainLoopSource?

> > Yep, a Queue object keeps a queue of functions, just like RunLoop, but
> > doesn't care about nested run loops or control over the main loop. It was
> > tailored for WorkQueue.
> 
> WorkQueues never use nested main loops, so it doesn't have any effect when
> using Runloop in WorkQueue, it simply creates the GMainLoop, and uses the
> functions queue to dispatch sources. 
> 
> > > > The idea sounds good, but I don't like WorkQueue switching over to using
> > > > RunLoop.
> > > 
> > > Could you elaborate? What's what you don't like? With this approach, I think
> > > the threaded compositor could simply use a WorkQueue instead of the current
> > > custom implementation, for example.
> > 
> > TC could likely use WorkQueue instead of RunLoop without the WorkQueue
> > implementation using RunLoop. If that's not the case, then a RunLoop shoul
> > be used instead.
> 
> Yes, that's right. I still don't see what's wrong using RunLoop in WorkQueue.

RunLoop and WorkQueue are two different and exclusive ways of managing a running thread. I disagree with one having to depend on the other. Furthermore, you already have to add a platform-specific method on RunLoop just to expose the GMainContext object to the WorkQueue.

[Carlos Garcia Campos]

(In reply to comment #35)
> 
> What's complex in GSourceWrap interfaces?
> And what's complex in GSourceWrap interfaces that's not complex in
> GMainLoopSource?

Maybe it's just me that I'm used to GMainLoopSource, but I find the GSourceWrap API difficult to use. You need to think everytime if you want a static, queue or timer based wrapper.

> RunLoop and WorkQueue are two different and exclusive ways of managing a
> running thread. I disagree with one having to depend on the other.
> Furthermore, you already have to add a platform-specific method on RunLoop
> just to expose the GMainContext object to the WorkQueue.

I don't think they are exclusive. WorkQueue uses a worker thread to run a main loop, so using RunLoop there seems natural to me. We are currently duplicating what RunLoop does in WorkQueue. And yes, mainContext() is needed to be able to schedule other kind of sources in a particular RunLoop, it's not specific to WorkQueue.

[Zan Dobersek]

(In reply to comment #36)
> (In reply to comment #35)
> > 
> > What's complex in GSourceWrap interfaces?
> > And what's complex in GSourceWrap interfaces that's not complex in
> > GMainLoopSource?
> 
> Maybe it's just me that I'm used to GMainLoopSource, but I find the
> GSourceWrap API difficult to use. You need to think everytime if you want a
> static, queue or timer based wrapper.
> 

We can remove Socket and Queue and roll both into WorkQueue, or at least Socket if RunLoop is used there. That would leave two.

For all I care GMainLoopSource can be incrementally slimmed down to how GSourceWrap is implemented.

> > RunLoop and WorkQueue are two different and exclusive ways of managing a
> > running thread. I disagree with one having to depend on the other.
> > Furthermore, you already have to add a platform-specific method on RunLoop
> > just to expose the GMainContext object to the WorkQueue.
> 
> I don't think they are exclusive. WorkQueue uses a worker thread to run a
> main loop, so using RunLoop there seems natural to me. We are currently
> duplicating what RunLoop does in WorkQueue. And yes, mainContext() is needed
> to be able to schedule other kind of sources in a particular RunLoop, it's
> not specific to WorkQueue.

IMO this is low-level library code, and we can afford being explicit in the implementation, directly using platform-specific APIs instead of relying on a neighbouring abstraction API. Unfortunately we don't have specific APIs available that we could simply map to, like Cocoa-based platforms for instance.

[Carlos Garcia Campos]

(In reply to comment #37)
> (In reply to comment #36)
> > (In reply to comment #35)
> > > 
> > > What's complex in GSourceWrap interfaces?
> > > And what's complex in GSourceWrap interfaces that's not complex in
> > > GMainLoopSource?
> > 
> > Maybe it's just me that I'm used to GMainLoopSource, but I find the
> > GSourceWrap API difficult to use. You need to think everytime if you want a
> > static, queue or timer based wrapper.
> > 
> 
> We can remove Socket and Queue and roll both into WorkQueue, or at least
> Socket if RunLoop is used there. That would leave two.
> 
> For all I care GMainLoopSource can be incrementally slimmed down to how
> GSourceWrap is implemented.

I think 99% of the cases can be converted to RunLoop::dispatch() and RunLoop::Timer.

> > > RunLoop and WorkQueue are two different and exclusive ways of managing a
> > > running thread. I disagree with one having to depend on the other.
> > > Furthermore, you already have to add a platform-specific method on RunLoop
> > > just to expose the GMainContext object to the WorkQueue.
> > 
> > I don't think they are exclusive. WorkQueue uses a worker thread to run a
> > main loop, so using RunLoop there seems natural to me. We are currently
> > duplicating what RunLoop does in WorkQueue. And yes, mainContext() is needed
> > to be able to schedule other kind of sources in a particular RunLoop, it's
> > not specific to WorkQueue.
> 
> IMO this is low-level library code, and we can afford being explicit in the
> implementation, directly using platform-specific APIs instead of relying on
> a neighbouring abstraction API. Unfortunately we don't have specific APIs
> available that we could simply map to, like Cocoa-based platforms for
> instance.

The neighbouring abstraction API is a platform implementation of a RunLoop, and WorkQueue needs a RunLoop implementation. I don't see the problem of WTF classes depending on each other. Or maybe I don't understand the reasoning.

[Zan Dobersek]

(In reply to comment #38)
> (In reply to comment #37)
> > (In reply to comment #36)
> > > (In reply to comment #35)
> > > > 
> > > > What's complex in GSourceWrap interfaces?
> > > > And what's complex in GSourceWrap interfaces that's not complex in
> > > > GMainLoopSource?
> > > 
> > > Maybe it's just me that I'm used to GMainLoopSource, but I find the
> > > GSourceWrap API difficult to use. You need to think everytime if you want a
> > > static, queue or timer based wrapper.
> > > 
> > 
> > We can remove Socket and Queue and roll both into WorkQueue, or at least
> > Socket if RunLoop is used there. That would leave two.
> > 
> > For all I care GMainLoopSource can be incrementally slimmed down to how
> > GSourceWrap is implemented.
> 
> I think 99% of the cases can be converted to RunLoop::dispatch() and
> RunLoop::Timer.
> 

Some of them could be switched over. But then you have cases in platform-specific code (GStreamer-related stuff mostly) where you don't have any control over the main loop. I wouldn't use RunLoop::Timer there for the sake of clean distinction between the thread management that's done by WebKit via RunLoop (which is at this point the case with the main and composition threads) and the thread management done by external dependencies.

> > > > RunLoop and WorkQueue are two different and exclusive ways of managing a
> > > > running thread. I disagree with one having to depend on the other.
> > > > Furthermore, you already have to add a platform-specific method on RunLoop
> > > > just to expose the GMainContext object to the WorkQueue.
> > > 
> > > I don't think they are exclusive. WorkQueue uses a worker thread to run a
> > > main loop, so using RunLoop there seems natural to me. We are currently
> > > duplicating what RunLoop does in WorkQueue. And yes, mainContext() is needed
> > > to be able to schedule other kind of sources in a particular RunLoop, it's
> > > not specific to WorkQueue.
> > 
> > IMO this is low-level library code, and we can afford being explicit in the
> > implementation, directly using platform-specific APIs instead of relying on
> > a neighbouring abstraction API. Unfortunately we don't have specific APIs
> > available that we could simply map to, like Cocoa-based platforms for
> > instance.
> 
> The neighbouring abstraction API is a platform implementation of a RunLoop,
> and WorkQueue needs a RunLoop implementation. I don't see the problem of WTF
> classes depending on each other. Or maybe I don't understand the reasoning.

WorkQueue doesn't need a RunLoop implementation. It can use it, but it doesn't need timers or nested looping support. It's also entirely possible to implement WorkQueue without a GMainLoop running the whole thing.

What your patch does improve, though it's not perfect, is addressing the remaining race conditions in WorkQueue construction and invalidation.

[Carlos Garcia Campos]

(In reply to comment #39)
> (In reply to comment #38)
> > (In reply to comment #37)
> > > (In reply to comment #36)
> > > > (In reply to comment #35)
> > > > > 
> > > > > What's complex in GSourceWrap interfaces?
> > > > > And what's complex in GSourceWrap interfaces that's not complex in
> > > > > GMainLoopSource?
> > > > 
> > > > Maybe it's just me that I'm used to GMainLoopSource, but I find the
> > > > GSourceWrap API difficult to use. You need to think everytime if you want a
> > > > static, queue or timer based wrapper.
> > > > 
> > > 
> > > We can remove Socket and Queue and roll both into WorkQueue, or at least
> > > Socket if RunLoop is used there. That would leave two.
> > > 
> > > For all I care GMainLoopSource can be incrementally slimmed down to how
> > > GSourceWrap is implemented.
> > 
> > I think 99% of the cases can be converted to RunLoop::dispatch() and
> > RunLoop::Timer.
> > 
> 
> Some of them could be switched over. But then you have cases in
> platform-specific code (GStreamer-related stuff mostly) where you don't have
> any control over the main loop. I wouldn't use RunLoop::Timer there for the
> sake of clean distinction between the thread management that's done by
> WebKit via RunLoop (which is at this point the case with the main and
> composition threads) and the thread management done by external dependencies.

Sure, I agree, I'm not saying we can replace all cases. In GST code what we want is to send tasks to the main thread from the GST threads, but RunLoop::dispatch() wouldn't work because we want to be able to cancel a source scheduled or check if it was already scheduled, for example. And I agree that using a Runloop::Timer there, even thought it might work, it's not ideal at all. My point is that if we can switch to use RunLoop for the cases where it makes sense, we might reduce the uses of GMainLoopSource to a very few cases where it would be better to rework GMainLoopSource to use persistent sources, instead of adding a new wrapper. 

So, I'm going to try this in a different way, instead of first moving to persistent source in RunLoop, I'll first move to use RunLoop in the cases I think make sense, and then we can see the cases where we are still using GMainLoopSource to see if we really need a new wrapper or not. I agree this doesn't belong to this bug.

> > > > > RunLoop and WorkQueue are two different and exclusive ways of managing a
> > > > > running thread. I disagree with one having to depend on the other.
> > > > > Furthermore, you already have to add a platform-specific method on RunLoop
> > > > > just to expose the GMainContext object to the WorkQueue.
> > > > 
> > > > I don't think they are exclusive. WorkQueue uses a worker thread to run a
> > > > main loop, so using RunLoop there seems natural to me. We are currently
> > > > duplicating what RunLoop does in WorkQueue. And yes, mainContext() is needed
> > > > to be able to schedule other kind of sources in a particular RunLoop, it's
> > > > not specific to WorkQueue.
> > > 
> > > IMO this is low-level library code, and we can afford being explicit in the
> > > implementation, directly using platform-specific APIs instead of relying on
> > > a neighbouring abstraction API. Unfortunately we don't have specific APIs
> > > available that we could simply map to, like Cocoa-based platforms for
> > > instance.
> > 
> > The neighbouring abstraction API is a platform implementation of a RunLoop,
> > and WorkQueue needs a RunLoop implementation. I don't see the problem of WTF
> > classes depending on each other. Or maybe I don't understand the reasoning.
> 
> WorkQueue doesn't need a RunLoop implementation. It can use it, but it
> doesn't need timers or nested looping support. It's also entirely possible
> to implement WorkQueue without a GMainLoop running the whole thing.

I'm not discussing whether WorkQueue needs a RunLoop or not, the fact is that our current implementation uses a GMainLoop (which I think is a good idea, but still that's a different discussion), and we already have a GMainLoop abstraction in WTF, so I don't see any problem in using it anywhere we need a GMainLoop.

> What your patch does improve, though it's not perfect, is addressing the
> remaining race conditions in WorkQueue construction and invalidation.

Yes, this was needed, since now the run loop has to be created in the thread. If you tell me how it can be perfect I'll be happy to make it perfect :-)

[Carlos Garcia Campos]

Comment on attachment 263834 [details]
Try to fix EFL build

Removing flags, this doesn't fit here, I agreed a new plan with Zan.

[Michael Catanzaro]

Comment on attachment 256302 [details]
Patch

Seems this patch should not be on request queue.

[Zan Dobersek]

There's now a good-enough solution in RunLoop::Timer<>.