Bug 155706 – [JSCOnly] Implement RunLoop and remove glib dependency

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

Worth trying. When it's done, JSCOnly port only requires ICU in the system.

[Yusuke Suzuki]

Created attachment 275915 [details]
Patch

WIP

[WebKit Commit Bot]

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


ERROR: Tools/TestWebKitAPI/config.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/none/WorkQueueNone.cpp:81:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/none/WorkQueueNone.cpp:87:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/none/RunLoopNone.cpp:127:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 4 in 20 files


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

[Yusuke Suzuki]

Created attachment 276008 [details]
Patch

WIP

[WebKit Commit Bot]

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


ERROR: Tools/TestWebKitAPI/config.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/none/WorkQueueNone.cpp:81:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/none/WorkQueueNone.cpp:87:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/none/RunLoopNone.cpp:127:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 4 in 18 files


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

[Yusuke Suzuki]

Created attachment 276010 [details]
Patch

WIP

[WebKit Commit Bot]

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


ERROR: Tools/TestWebKitAPI/config.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/none/WorkQueueNone.cpp:81:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/none/WorkQueueNone.cpp:87:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/none/RunLoopNone.cpp:131:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 4 in 18 files


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

[Yusuke Suzuki]

Created attachment 276142 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 276142 [details]
Patch

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

Add comments.

> Tools/TestWebKitAPI/PlatformUtilities.h:29
> +#if !PLATFORM(JSCONLY)

Currently, PLATFORM(JSCONLY) is only used in the TestWTF test harness.

[WebKit Commit Bot]

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


ERROR: Tools/TestWebKitAPI/config.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/none/RunLoopNone.cpp:129:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 18 files


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

[Yusuke Suzuki]

Created attachment 276143 [details]
Patch

Windows Build fix

[WebKit Commit Bot]

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


ERROR: Tools/TestWebKitAPI/config.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/none/RunLoopNone.cpp:129:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 18 files


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

[Yusuke Suzuki]

Created attachment 276149 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Tools/TestWebKitAPI/config.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/none/RunLoopNone.cpp:129:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 18 files


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

[Yusuke Suzuki]

Created attachment 276242 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/platformindependent/RunLoopPlatformIndependent.cpp:129:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 20 files


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

[Yusuke Suzuki]

Comment on attachment 276242 [details]
Patch

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

Add comments.

> Source/WTF/wtf/Platform.h:1149
> +#define EVENT_LOOP(WTF_FEATURE) (defined WTF_EVENT_LOOP_##WTF_FEATURE  && WTF_EVENT_LOOP_##WTF_FEATURE)

Instead of adding these flag definition in port's build configs, we determine it in Platform.h.
This is the same style to PLATFORM(OSX) / PLATFORM(IOS).

> Source/WTF/wtf/glib/RunLoopGLib.cpp:153
> +}

Move it from WorkQueueGLib. This is better since it is RunLoop related code.

[Konstantin Tokarev]

Comment on attachment 276242 [details]
Patch

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

> Source/WTF/wtf/PlatformGTK.cmake:10
> +    platformindependent/WorkQueuePlatformIndependent.cpp

"PlatformIndependent" is overly long, in this case

> Source/WTF/wtf/RunLoop.h:167
> +#elif EVENT_LOOP(COCOA)

I think it should be EVENT_LOOP(DARWIN) or EVENT_LOOP(CF)

>> Source/WTF/wtf/glib/RunLoopGLib.cpp:153
>> +}
> 
> Move it from WorkQueueGLib. This is better since it is RunLoop related code.

I would do it in a separate patch

[Konstantin Tokarev]

Comment on attachment 276242 [details]
Patch

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

>> Source/WTF/wtf/PlatformGTK.cmake:10
>> +    platformindependent/WorkQueuePlatformIndependent.cpp
> 
> "PlatformIndependent" is overly long, in this case

...in this case I think it might be better to call it "C++11 event loop" (e.g., cxx11/WorkQueueCxx11.cpp)

[Yusuke Suzuki]

Created attachment 276246 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 276242 [details]
Patch

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

Thanks!

>>> Source/WTF/wtf/PlatformGTK.cmake:10
>>> +    platformindependent/WorkQueuePlatformIndependent.cpp
>> 
>> "PlatformIndependent" is overly long, in this case
> 
> ...in this case I think it might be better to call it "C++11 event loop" (e.g., cxx11/WorkQueueCxx11.cpp)

Changed to Cxx11. (EVENT_LOOP(CXX11).

>> Source/WTF/wtf/RunLoop.h:167
>> +#elif EVENT_LOOP(COCOA)
> 
> I think it should be EVENT_LOOP(DARWIN) or EVENT_LOOP(CF)

Changed to DARWIN. Since AppleWin port uses CF, but it uses EVENT_LOOP(WINDOWS). EVENT_LOOP(DARWIN) seems better.

>>> Source/WTF/wtf/glib/RunLoopGLib.cpp:153
>>> +}
>> 
>> Move it from WorkQueueGLib. This is better since it is RunLoop related code.
> 
> I would do it in a separate patch

Done :)

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/cxx11/RunLoopCxx11.cpp:129:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 19 files


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

[Konstantin Tokarev]

Comment on attachment 276246 [details]
Patch

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

> Source/cmake/OptionsJSCOnly.cmake:19
> +set(DEFAULT_EVENT_LOOP_TYPE "None")

Please rename "None" to "C++11" or "Cxx11"

[Yusuke Suzuki]

Comment on attachment 276246 [details]
Patch

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

>> Source/cmake/OptionsJSCOnly.cmake:19
>> +set(DEFAULT_EVENT_LOOP_TYPE "None")
> 
> Please rename "None" to "C++11" or "Cxx11"

Oops, renaming.

[Yusuke Suzuki]

Created attachment 276247 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/cxx11/RunLoopCxx11.cpp:129:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 19 files


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

[Konstantin Tokarev]

Comment on attachment 276247 [details]
Patch

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

> Source/WTF/wtf/cxx11/RunLoopCxx11.cpp:129
> +        {

I think we should avoid having clever scope there and move its contents into static inline function instead

[Yusuke Suzuki]

Created attachment 276248 [details]
Patch

[Yusuke Suzuki]

Created attachment 276249 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 276247 [details]
Patch

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

>> Source/WTF/wtf/cxx11/RunLoopCxx11.cpp:129
>> +        {
> 
> I think we should avoid having clever scope there and move its contents into static inline function instead

Changed it. But this scope highly depends on RunLoop's fields. So static inline function cannot be used here.
Instead, I added RunLoop::populateTasks() member function with ALWAYS_INLINE. And call it only from RunLoopCxx11.cpp.
And I annotated runImpl as NEVER_INLINE.

[Yusuke Suzuki]

Created attachment 276251 [details]
Patch

[Yusuke Suzuki]

Created attachment 276253 [details]
Patch

[Yusuke Suzuki]

Created attachment 276254 [details]
Patch

[Konstantin Tokarev]

Comment on attachment 276253 [details]
Patch

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

> Source/WTF/wtf/RunLoop.h:185
> +    void scheduleAndWakeUp(RefPtr<TimerBase::ScheduledTask>);

Shouldn't schedule methods take RefPtr<TimerBase::ScheduledTask> by rvalue reference?

[Yusuke Suzuki]

Created attachment 276257 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 276253 [details]
Patch

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

>> Source/WTF/wtf/RunLoop.h:185
>> +    void scheduleAndWakeUp(RefPtr<TimerBase::ScheduledTask>);
> 
> Shouldn't schedule methods take RefPtr<TimerBase::ScheduledTask> by rvalue reference?

For scheduleAndWakeUp, it should not be rvalue ref since TimerBase owns it as m_scheduledTask.
for schedule(RefPtr<TimerBase::ScheduledTask>), it is OK.

[Yusuke Suzuki]

Created attachment 276258 [details]
Patch

[Michael Catanzaro]

(In reply to comment #22)
> Comment on attachment 276246 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276246&action=review
> 
> > Source/cmake/OptionsJSCOnly.cmake:19
> > +set(DEFAULT_EVENT_LOOP_TYPE "None")
> 
> Please rename "None" to "C++11" or "Cxx11"

I would go with "Generic" personally

[Yusuke Suzuki]

Comment on attachment 276246 [details]
Patch

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

>>>> Source/cmake/OptionsJSCOnly.cmake:19
>>>> +set(DEFAULT_EVENT_LOOP_TYPE "None")
>>> 
>>> Please rename "None" to "C++11" or "Cxx11"
>> 
>> Oops, renaming.
> 
> I would go with "Generic" personally

OK, I've updated it.

[Yusuke Suzuki]

Created attachment 276265 [details]
Patch

[Michael Catanzaro]

Comment on attachment 276265 [details]
Patch

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

I've reviewed so far all but RunLoopGeneric.cpp (the most important part). I have a bunch of comments but they're all minor, mostly just nits about your comments. So far this looks like r+ to me, but I need to spend more time looking at RunLoopGeneric.cpp first.

> Source/WTF/ChangeLog:15
> +        dependency. As a result, now, JSCOnly port removes dependencies except for the system ICU.

Excellent.

I'm curious how the performance is relative to the GLib-based RunLoop.

> Source/WTF/ChangeLog:21
> +        EVENT_LOOP determination is done in Platform.h. This follows the style of WTF PLATFORM.

Missing a newline before this paragraph.

> Source/WTF/wtf/Platform.h:1147
> + * For example, GTK port in OSX, its platform is GTK, OS is DARWIN and EVENT_LOOP is GLIB.

OS X

> Source/WTF/wtf/Platform.h:1149
> +#define EVENT_LOOP(WTF_FEATURE) (defined WTF_EVENT_LOOP_##WTF_FEATURE  && WTF_EVENT_LOOP_##WTF_FEATURE)

You have an extra space character here before the &&

> Source/WTF/wtf/Platform.h:1159
> + * So that, EVENT_LOOP(WINDOWS) && USE(CF) can be true.

Some English nits here: this comma doesn't belong, and this is a sentence fragment. The easiest way to improve it is to merge it into the previous sentence:

Even if the port is AppleWin, we use the Windows message pump system for the event loop, so that EVENT_LOOP(WINDOWS) && USE(CF) can be true.

> Source/WTF/wtf/Platform.h:1161
> + * PLATFORM(WIN) should be false. And in that case, GLIB's event loop is used.

GLib's

> Source/WTF/wtf/Platform.h:1165
> +/* OSX and IOS. Use CoreFoundation & GCD abstraction. */

OS X, iOS

Another style nit: & -> "and." We normally only use & in trademarks, logos, and other special situations: https://en.wikipedia.org/wiki/Ampersand#Usage

A more substantial question: is WTF_EVENT_LOOP_DARWIN the best name for this? I'm not familiar with Apple ecosystem development, but that implies to me that the event loop abstraction uses native Darwin APIs and can be used without the higher-level CoreFoundation?

> Source/WTF/wtf/Platform.h:1168
> +/* EFL port uses GLIB. But it uses its own event loop abstraction.

GLib

> Source/WTF/wtf/Platform.h:1173
> +/* Use GLIB's event loop abstraction. Primarily GTK port uses it. */

Ditto.

> Source/WTF/wtf/RunLoop.h:77
> +    WTF_EXPORT_PRIVATE static void iterate();

A public static function that iterates every RunLoop that exists? This doesn't seem like a great interface....

> Source/WTF/wtf/WorkQueue.h:86
> +    RunLoop& runLoop() const { return *m_runLoop; }

Looks like m_runLoop is always non-null. In a follow-up patch, it would be good to convert m_runLoop to be a reference rather than a pointer.

> Source/WTF/wtf/generic/WorkQueueGeneric.cpp:42
> +        threadName = threadName.substring(size + 1);

This code is pretty confusing without the comment in WorkQueueGLib.cpp, you might want to copy that as well.

> Source/WTF/wtf/generic/WorkQueueGeneric.cpp:87
> +void WorkQueue::dispatchAfter(std::chrono::nanoseconds delay, std::function<void()> function)

So, except for this dispatchAfter function, all the code in this file looks identical to WorkQueueGLib.cpp. It's a recipe for bugs to be fixed in one port but not the other. Perhaps we could move this to be conditionally-compiled in WorkQueue.cpp? We already have a huge chunk of #if !PLATFORM(COCOA) there, so it's not really any worse to add an #if EVENT_LOOP(GLIB) || EVENT_LOOP(GENERIC) block as well.

> Tools/ChangeLog:10
> +        it is not `cxx11` (Since it includes the GLIB event loop case).

Stale "cxx11" in the changelog

> Tools/TestWebKitAPI/PlatformJSCOnly.cmake:14
> +if (LOWERCASE_EVENT_LOOP_TYPE STREQUAL "glib")

I'm not sure whether it's good idea to support the GLib event loop any more. It would be unfortunate if you run into a bug that can only be reproduced with one event loop setting or the other.

It could be useful to compile WebKit twice with different event loops for testing purposes, to confirm that they work the same, or to compare performance; but it's simple enough to restore support for the GLib event loop with a patch, so I don't think this outweighs the disadvantage.

> Tools/TestWebKitAPI/PlatformUtilities.h:29
> +#if !defined(BUILDING_JSCONLY__)

You could use #ifndef here.

[Michael Catanzaro]

(In reply to comment #40)
> > Source/WTF/wtf/RunLoop.h:77
> > +    WTF_EXPORT_PRIVATE static void iterate();
> 
> A public static function that iterates every RunLoop that exists? This
> doesn't seem like a great interface....

Ah I see it only iterates the current RunLoop... well that was my guess, based on the declaration in the header file; perhaps worth adding a comment there.

[Konstantin Tokarev]

Comment on attachment 276265 [details]
Patch

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

>> Source/WTF/wtf/Platform.h:1159
>> + * So that, EVENT_LOOP(WINDOWS) && USE(CF) can be true.
> 
> Some English nits here: this comma doesn't belong, and this is a sentence fragment. The easiest way to improve it is to merge it into the previous sentence:
> 
> Even if the port is AppleWin, we use the Windows message pump system for the event loop, so that EVENT_LOOP(WINDOWS) && USE(CF) can be true.

BTW, I think this whole passage is superfluous:
1. Anyone slightly aware of how Windows ports work knows that they have USE(CF), and if it is not self-evident, there may be a better place to discuss it
2. It's absolutely natural to use WTF_EVENT_LOOP_WINDOWS with PLATFORM(WIN)

>> Source/WTF/wtf/Platform.h:1165
>> +/* OSX and IOS. Use CoreFoundation & GCD abstraction. */
> 
> OS X, iOS
> 
> Another style nit: & -> "and." We normally only use & in trademarks, logos, and other special situations: https://en.wikipedia.org/wiki/Ampersand#Usage
> 
> A more substantial question: is WTF_EVENT_LOOP_DARWIN the best name for this? I'm not familiar with Apple ecosystem development, but that implies to me that the event loop abstraction uses native Darwin APIs and can be used without the higher-level CoreFoundation?

Currenly existing cannot be used without CF

> Source/WTF/wtf/Platform.h:1169
> + * Thus, EVENT_LOOP(EFL) && USE(GLIB) can be true.

These words would be better expressed as code, if we had in one place

#if PLATFORM(EFL)
#define WTF_EVENT_LOOP_EFL 1
#define USE_GLIB 1
...

>> Tools/TestWebKitAPI/PlatformJSCOnly.cmake:14
>> +if (LOWERCASE_EVENT_LOOP_TYPE STREQUAL "glib")
> 
> I'm not sure whether it's good idea to support the GLib event loop any more. It would be unfortunate if you run into a bug that can only be reproduced with one event loop setting or the other.
> 
> It could be useful to compile WebKit twice with different event loops for testing purposes, to confirm that they work the same, or to compare performance; but it's simple enough to restore support for the GLib event loop with a patch, so I don't think this outweighs the disadvantage.

I think it's a good idea, because using native event loop may be very useful for those trying to use JSC in application which already runs some event loop. And if, for example, there is a bug in JSCOnly with GLib it almost certainly means there is the same bug in GTK port.

[Yusuke Suzuki]

Comment on attachment 276265 [details]
Patch

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

>> Source/WTF/ChangeLog:15
>> +        dependency. As a result, now, JSCOnly port removes dependencies except for the system ICU.
> 
> Excellent.
> 
> I'm curious how the performance is relative to the GLib-based RunLoop.

I've just measured the time. The test invokes the event loop 100000000 times. https://gist.github.com/Constellation/e27d023de983af2cee917200b10c34e6
Roughly speaking, each loop is 7.6x faster than glib's one.
But I think it's not a problem since each loop is still very fast enough.

$ time WebKitBuild/runloop/Release/bin/TestWebKitAPI/WTF/TestWTF --gtest_filter="?*RunLoop?*ManyTimes?*"
**PASS** WTF_RunLoop.ManyTimes
WebKitBuild/runloop/Release/bin/TestWebKitAPI/WTF/TestWTF   12.42s user 0.00s system 100% cpu 12.416 total

$ time WebKitBuild/runloop/Release/bin/TestWebKitAPI/WTF/TestWTF --gtest_filter="?*RunLoop?*ManyTimes?*"
**PASS** WTF_RunLoop.ManyTimes
WebKitBuild/runloop/Release/bin/TestWebKitAPI/WTF/TestWTF   93.01s user 42.65s system 100% cpu 2:15.54 total

I don't investigate it carefully, but we can consider the following reason.

1. RunLoop interface is limited compared to glib's main loop. Our generic RunLoop design is suitable for the current RunLoop's interface. So it is highly optimized. (For example, our main loop allocation is done in C stack, while glib allocates it by using g_main_loop_new.)
2. We use WTF's lock and condition. They are highly optimized compared to system's one. It first spins. And after some durations, it becomes usual lock.
3. Our RunLoop is designed for CPU works. So our implementation does not consider about async I/O usage. But glib's one is not. It may call select / epoll / kqueue in its implementation. But we don't.

>> Source/WTF/ChangeLog:21
>> +        EVENT_LOOP determination is done in Platform.h. This follows the style of WTF PLATFORM.
> 
> Missing a newline before this paragraph.

Added.

>> Source/WTF/wtf/Platform.h:1147
>> + * For example, GTK port in OSX, its platform is GTK, OS is DARWIN and EVENT_LOOP is GLIB.
> 
> OS X

Thanks, fixed.

>> Source/WTF/wtf/Platform.h:1149
>> +#define EVENT_LOOP(WTF_FEATURE) (defined WTF_EVENT_LOOP_##WTF_FEATURE  && WTF_EVENT_LOOP_##WTF_FEATURE)
> 
> You have an extra space character here before the &&

Oops, thanks.

>>> Source/WTF/wtf/Platform.h:1159
>>> + * So that, EVENT_LOOP(WINDOWS) && USE(CF) can be true.
>> 
>> Some English nits here: this comma doesn't belong, and this is a sentence fragment. The easiest way to improve it is to merge it into the previous sentence:
>> 
>> Even if the port is AppleWin, we use the Windows message pump system for the event loop, so that EVENT_LOOP(WINDOWS) && USE(CF) can be true.
> 
> BTW, I think this whole passage is superfluous:
> 1. Anyone slightly aware of how Windows ports work knows that they have USE(CF), and if it is not self-evident, there may be a better place to discuss it
> 2. It's absolutely natural to use WTF_EVENT_LOOP_WINDOWS with PLATFORM(WIN)

I think it is necessary. USE(CF) makes us confused; we could misunderstand that CF event loop can be used in PLATFORM(WIN). This description clarify that PLATFORM(WIN) uses Windows message pump.

>> Source/WTF/wtf/Platform.h:1161
>> + * PLATFORM(WIN) should be false. And in that case, GLIB's event loop is used.
> 
> GLib's

Fixed.

>>> Source/WTF/wtf/Platform.h:1165
>>> +/* OSX and IOS. Use CoreFoundation & GCD abstraction. */
>> 
>> OS X, iOS
>> 
>> Another style nit: & -> "and." We normally only use & in trademarks, logos, and other special situations: https://en.wikipedia.org/wiki/Ampersand#Usage
>> 
>> A more substantial question: is WTF_EVENT_LOOP_DARWIN the best name for this? I'm not familiar with Apple ecosystem development, but that implies to me that the event loop abstraction uses native Darwin APIs and can be used without the higher-level CoreFoundation?
> 
> Currenly existing cannot be used without CF

We cannot use these event loops without GCD (libdispatch) since WorkQueueCocoa uses libdispatch. I'm not sure, but I think libdispatch is not included in CF.
If we choose EVENT_LOOP_CF, it is confusing for AppleWin. We could misconsider that AppleWin port may use this EVENT_LOOP_CF since it uses CF.
That's why I take DARWIN for this name.

>> Source/WTF/wtf/Platform.h:1168
>> +/* EFL port uses GLIB. But it uses its own event loop abstraction.
> 
> GLib

Fixed.

>> Source/WTF/wtf/Platform.h:1169
>> + * Thus, EVENT_LOOP(EFL) && USE(GLIB) can be true.
> 
> These words would be better expressed as code, if we had in one place
> 
> #if PLATFORM(EFL)
> #define WTF_EVENT_LOOP_EFL 1
> #define USE_GLIB 1
> ...

Hm, I don't think it's good idea; scattering EVENT_LOOP definitions.
It makes us difficult to understand which EVENT_LOOP is used for the given platform.
And it makes WTF_DEFAULT_EVENT_LOOP difficult.
Everytime you define WTF_EVENT_LOOP_XXX, you need to check WTF_DEFAULT_EVENT_LOOP.

>> Source/WTF/wtf/Platform.h:1173
>> +/* Use GLIB's event loop abstraction. Primarily GTK port uses it. */
> 
> Ditto.

Fixed.

>> Source/WTF/wtf/RunLoop.h:77
>> +    WTF_EXPORT_PRIVATE static void iterate();
> 
> A public static function that iterates every RunLoop that exists? This doesn't seem like a great interface....

The TestWTF uses this interface. (And it is only the user).
In the other port (like GTK / EFL), they call the raw platform-dependent iteration operation (For example, EFL calls ecore_main_loop_iterate()).

>> Source/WTF/wtf/WorkQueue.h:86
>> +    RunLoop& runLoop() const { return *m_runLoop; }
> 
> Looks like m_runLoop is always non-null. In a follow-up patch, it would be good to convert m_runLoop to be a reference rather than a pointer.

Sounds nice.

>> Source/WTF/wtf/generic/WorkQueueGeneric.cpp:42
>> +        threadName = threadName.substring(size + 1);
> 
> This code is pretty confusing without the comment in WorkQueueGLib.cpp, you might want to copy that as well.

Thanks, copied.

>> Source/WTF/wtf/generic/WorkQueueGeneric.cpp:87
>> +void WorkQueue::dispatchAfter(std::chrono::nanoseconds delay, std::function<void()> function)
> 
> So, except for this dispatchAfter function, all the code in this file looks identical to WorkQueueGLib.cpp. It's a recipe for bugs to be fixed in one port but not the other. Perhaps we could move this to be conditionally-compiled in WorkQueue.cpp? We already have a huge chunk of #if !PLATFORM(COCOA) there, so it's not really any worse to add an #if EVENT_LOOP(GLIB) || EVENT_LOOP(GENERIC) block as well.

WorkQueueGLib's dispatchAfter (creating GSource and registering it) should be moved to RunLoop since it interacts with the platform-dependent run loop abstraction.
So my proposed patch was,

1. Move WorkQueueGLib::dispatchAfter's content to RunLoopGLib::dispatchAfter
2. Remove WorkQueueGLib.cpp and use generic/WorkQueueGeneric.cpp instead.

But it enlarges the patch. So once it was done in the previous patch (https://bugs.webkit.org/attachment.cgi?id=276242&action=review), now it is removed.
This will be done in the subsequent patch :)

>> Tools/ChangeLog:10
>> +        it is not `cxx11` (Since it includes the GLIB event loop case).
> 
> Stale "cxx11" in the changelog

Thanks. Fixed.

>> Tools/TestWebKitAPI/PlatformUtilities.h:29
>> +#if !defined(BUILDING_JSCONLY__)
> 
> You could use #ifndef here.

Fixed.

[Yusuke Suzuki]

Comment on attachment 276265 [details]
Patch

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

>>> Tools/TestWebKitAPI/PlatformJSCOnly.cmake:14
>>> +if (LOWERCASE_EVENT_LOOP_TYPE STREQUAL "glib")
>> 
>> I'm not sure whether it's good idea to support the GLib event loop any more. It would be unfortunate if you run into a bug that can only be reproduced with one event loop setting or the other.
>> 
>> It could be useful to compile WebKit twice with different event loops for testing purposes, to confirm that they work the same, or to compare performance; but it's simple enough to restore support for the GLib event loop with a patch, so I don't think this outweighs the disadvantage.
> 
> I think it's a good idea, because using native event loop may be very useful for those trying to use JSC in application which already runs some event loop. And if, for example, there is a bug in JSCOnly with GLib it almost certainly means there is the same bug in GTK port.

I think it's good. We can easily compare the generic RunLoop with the GLib's one.
And maintenance cost is very small.

[Yusuke Suzuki]

(In reply to comment #41)
> (In reply to comment #40)
> > > Source/WTF/wtf/RunLoop.h:77
> > > +    WTF_EXPORT_PRIVATE static void iterate();
> > 
> > A public static function that iterates every RunLoop that exists? This
> > doesn't seem like a great interface....
> 
> Ah I see it only iterates the current RunLoop... well that was my guess,
> based on the declaration in the header file; perhaps worth adding a comment
> there.

Yup, I'll comment it here :)

[Yusuke Suzuki]

Created attachment 276373 [details]
Patch

[Konstantin Tokarev]

Comment on attachment 276265 [details]
Patch

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

>>>> Source/WTF/wtf/Platform.h:1165
>>>> +/* OSX and IOS. Use CoreFoundation & GCD abstraction. */
>>> 
>>> A more substantial question: is WTF_EVENT_LOOP_DARWIN the best name for this? I'm not familiar with Apple ecosystem development, but that implies to me that the event loop abstraction uses native Darwin APIs and can be used without the higher-level CoreFoundation?
>> 
>> Currenly existing cannot be used without CF
> 
> We cannot use these event loops without GCD (libdispatch) since WorkQueueCocoa uses libdispatch. I'm not sure, but I think libdispatch is not included in CF.
> If we choose EVENT_LOOP_CF, it is confusing for AppleWin. We could misconsider that AppleWin port may use this EVENT_LOOP_CF since it uses CF.
> That's why I take DARWIN for this name.

Well, similar situation exists in EFL port which has USE(GLIB) bun not EVENT_LOOP(GLIB), which was the whole point of EVENT_LOOP macto introduction.

But I think it should be correct to call it EVENT_LOOP(LIBDISPATCH) if you don't like EVENT_LOOP(CF).

[Yusuke Suzuki]

Comment on attachment 276265 [details]
Patch

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

>>>>> Source/WTF/wtf/Platform.h:1165
>>>>> +/* OSX and IOS. Use CoreFoundation & GCD abstraction. */
>>>> 
>>>> OS X, iOS
>>>> 
>>>> Another style nit: & -> "and." We normally only use & in trademarks, logos, and other special situations: https://en.wikipedia.org/wiki/Ampersand#Usage
>>>> 
>>>> A more substantial question: is WTF_EVENT_LOOP_DARWIN the best name for this? I'm not familiar with Apple ecosystem development, but that implies to me that the event loop abstraction uses native Darwin APIs and can be used without the higher-level CoreFoundation?
>>> 
>>> Currenly existing cannot be used without CF
>> 
>> We cannot use these event loops without GCD (libdispatch) since WorkQueueCocoa uses libdispatch. I'm not sure, but I think libdispatch is not included in CF.
>> If we choose EVENT_LOOP_CF, it is confusing for AppleWin. We could misconsider that AppleWin port may use this EVENT_LOOP_CF since it uses CF.
>> That's why I take DARWIN for this name.
> 
> Well, similar situation exists in EFL port which has USE(GLIB) bun not EVENT_LOOP(GLIB), which was the whole point of EVENT_LOOP macto introduction.
> 
> But I think it should be correct to call it EVENT_LOOP(LIBDISPATCH) if you don't like EVENT_LOOP(CF).

Can we call GCD as a part of CF? If so, we can call it as EVENT_LOOP(GCD) / EVENT_LOOP(LIBDISPATCH).

[Anders Carlsson]

I don't like this. I don't think we should have our own run loop implementation in WTF.

[Yusuke Suzuki]

(In reply to comment #49)
> I don't like this. I don't think we should have our own run loop
> implementation in WTF.

I think adding this simple run loop for JSCOnly port's fallback has good tradeoff in comparison with adding the whole glib dependency to JSCOnly port.

The goal of this patch is *not* replacing the existing run loop (like, RunLoopWin, RunLoopCF, etc.) with our own run loop.
This patch just adds the super simple fallback implementation to JSCOnly port.
Of course, it does not affect on OSX, IOS, and other ports.
I think adding such a simple generic run loop fallback for JSCOnly port is good
in comparison with adding the whole glib dependencies to JSCOnly port,
since JSCOnly port doesn't depend on any glib specific features except for this run loop.
The run loop itself is easy to be written if we don't consider about platform-dependent async I/O (abstracting select, epoll, kqueue, etc. If we require such features, it makes the run loop implementation very complicated).
WTF RunLoop does not support such async I/O interfaces.
This fact makes the run loop fallback implementation simple.

Furthermore, I think the added run loop does not incur maintenance burden.
This is because of the following reasons.

1. RunLoop's common interface is much stable since it is not changed for a very long time (from when we moved RunLoop from WebCore to WTF at 2013). So once it works, we need not to change it anymore.
2. The patch's logic part is completely limited in RunLoopGeneric.cpp. While the uploaded patch incldues generic/WorkQueueGeneric.cpp, but it is almost the same to glib/WorkQueueGLib.cpp. The subsequent patch will drop WorkQueueGLib to use this generic WorkQueue instead. After that, generic run loop specific code is confined in RunLoopGeneric.cpp. (Since including this change enlarges the patch, we splitted out)

The goal of JSCOnly port[1] is providing JSC only build configuration with very limited dependencies (currently, ICU).
It makes building JSC easily on any platforms with super aggressive features (Since JSCOnly port is not production port).
And it also makes the testing easiter, especially on Linux.
For example, since we have no dependencies except for ICU, we can easily build the old JSC without rebuilding dependent libraries for the old revision.

One alternative approach is moving RunLoop from WTF to some platform specific library set, like PAL[2].
This also seems good to me.

[1]: https://lists.webkit.org/pipermail/webkit-dev/2015-November/027785.html
[2]: https://bugs.webkit.org/show_bug.cgi?id=143358

[Michael Catanzaro]

(In reply to comment #49)
> I don't like this. I don't think we should have our own run loop
> implementation in WTF.

Why not; where would you rather put it? Some new directory seems like overkill?

(In reply to comment #50)
> One alternative approach is moving RunLoop from WTF to some platform
> specific library set, like PAL[2].

Once PAL exists, that would be fine IMO; for now, WTF is a better place than WebCore/platform.

[Alex Christensen]

(In reply to comment #51)
> (In reply to comment #49)
> > I don't like this. I don't think we should have our own run loop
> > implementation in WTF.
> 
> Why not; where would you rather put it? Some new directory seems like
> overkill?
I see where Anders is coming from, but I must say I think being able to build JavaScriptCore with only ICU as a dependency will have large benefits to us.
> 
> (In reply to comment #50)
> > One alternative approach is moving RunLoop from WTF to some platform
> > specific library set, like PAL[2].
> 
> Once PAL exists, that would be fine IMO; for now, WTF is a better place than
> WebCore/platform.
I do not think this should go in WebCore.  The whole point of JSCOnly is that it does not have any dependencies on WebCore.

[Alex Christensen]

Comment on attachment 276373 [details]
Patch

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

> Source/WTF/wtf/Platform.h:1151
> +#define EVENT_LOOP(WTF_FEATURE) (defined WTF_EVENT_LOOP_##WTF_FEATURE && WTF_EVENT_LOOP_##WTF_FEATURE)

I don't think we should add another macro for this.  Couldn't we just use USE(GLIB) etc?

> Tools/TestWebKitAPI/PlatformJSCOnly.cmake:26
> +    ${TESTWEBKITAPI_DIR}/Tests/WTF/RunLoop.cpp

We may as well run all WTF API tests for JSCOnly.

[Konstantin Tokarev]

Comment on attachment 276373 [details]
Patch

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

>> Source/WTF/wtf/Platform.h:1151
>> +#define EVENT_LOOP(WTF_FEATURE) (defined WTF_EVENT_LOOP_##WTF_FEATURE && WTF_EVENT_LOOP_##WTF_FEATURE)
> 
> I don't think we should add another macro for this.  Couldn't we just use USE(GLIB) etc?

I proposed this change in order to clarify preprocessor logic in event loop headers. Currenlty it has two issues:

1) it depends on ordering, because PLATFORM(EFL) needs to preceed USE(GLIB) so that EFL port, which enables USE(GLIB), could use its own event loop; if WorkQueue USE(GLIB) must precede OS(DARWIN), otherwise GTK port would break

2) it is not consistent: USE(GLIB), PLATFORM(COCOA,EFL,WIN), OS(DARWIN) are all used for choosing event loop implementation. I think using new macro will make intention clear, and will allow  JSCOnly to use all those event loops on platforms where they are available.

[Darin Adler]

Comment on attachment 276373 [details]
Patch

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

>>> Source/WTF/wtf/Platform.h:1151
>>> +#define EVENT_LOOP(WTF_FEATURE) (defined WTF_EVENT_LOOP_##WTF_FEATURE && WTF_EVENT_LOOP_##WTF_FEATURE)
>> 
>> I don't think we should add another macro for this.  Couldn't we just use USE(GLIB) etc?
> 
> I proposed this change in order to clarify preprocessor logic in event loop headers. Currenlty it has two issues:
> 
> 1) it depends on ordering, because PLATFORM(EFL) needs to preceed USE(GLIB) so that EFL port, which enables USE(GLIB), could use its own event loop; if WorkQueue USE(GLIB) must precede OS(DARWIN), otherwise GTK port would break
> 
> 2) it is not consistent: USE(GLIB), PLATFORM(COCOA,EFL,WIN), OS(DARWIN) are all used for choosing event loop implementation. I think using new macro will make intention clear, and will allow  JSCOnly to use all those event loops on platforms where they are available.

We should definitely not add a new macro like this.

[Darin Adler]

Comment on attachment 276373 [details]
Patch

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

>>>> Source/WTF/wtf/Platform.h:1151
>>>> +#define EVENT_LOOP(WTF_FEATURE) (defined WTF_EVENT_LOOP_##WTF_FEATURE && WTF_EVENT_LOOP_##WTF_FEATURE)
>>> 
>>> I don't think we should add another macro for this.  Couldn't we just use USE(GLIB) etc?
>> 
>> I proposed this change in order to clarify preprocessor logic in event loop headers. Currenlty it has two issues:
>> 
>> 1) it depends on ordering, because PLATFORM(EFL) needs to preceed USE(GLIB) so that EFL port, which enables USE(GLIB), could use its own event loop; if WorkQueue USE(GLIB) must precede OS(DARWIN), otherwise GTK port would break
>> 
>> 2) it is not consistent: USE(GLIB), PLATFORM(COCOA,EFL,WIN), OS(DARWIN) are all used for choosing event loop implementation. I think using new macro will make intention clear, and will allow  JSCOnly to use all those event loops on platforms where they are available.
> 
> We should definitely not add a new macro like this.

The correct WebKit pattern would be:

    USE(WINDOWS_EVENT_LOOP)
    USE(DARWIN_EVENT_LOOP)
    USE(EFL_EVENT_LOOP)
    USE(GLIB_EVENT_LOOP)

It’s fine with me if we want to add those.

[Yusuke Suzuki]

Comment on attachment 276373 [details]
Patch

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

Thanks!

>>>>> Source/WTF/wtf/Platform.h:1151
>>>>> +#define EVENT_LOOP(WTF_FEATURE) (defined WTF_EVENT_LOOP_##WTF_FEATURE && WTF_EVENT_LOOP_##WTF_FEATURE)
>>>> 
>>>> I don't think we should add another macro for this.  Couldn't we just use USE(GLIB) etc?
>>> 
>>> I proposed this change in order to clarify preprocessor logic in event loop headers. Currenlty it has two issues:
>>> 
>>> 1) it depends on ordering, because PLATFORM(EFL) needs to preceed USE(GLIB) so that EFL port, which enables USE(GLIB), could use its own event loop; if WorkQueue USE(GLIB) must precede OS(DARWIN), otherwise GTK port would break
>>> 
>>> 2) it is not consistent: USE(GLIB), PLATFORM(COCOA,EFL,WIN), OS(DARWIN) are all used for choosing event loop implementation. I think using new macro will make intention clear, and will allow  JSCOnly to use all those event loops on platforms where they are available.
>> 
>> We should definitely not add a new macro like this.
> 
> The correct WebKit pattern would be:
> 
>     USE(WINDOWS_EVENT_LOOP)
>     USE(DARWIN_EVENT_LOOP)
>     USE(EFL_EVENT_LOOP)
>     USE(GLIB_EVENT_LOOP)
> 
> It’s fine with me if we want to add those.

Darin's suggestion sounds fine to me. I'll update the patch with this form :)

>> Tools/TestWebKitAPI/PlatformJSCOnly.cmake:26
>> +    ${TESTWEBKITAPI_DIR}/Tests/WTF/RunLoop.cpp
> 
> We may as well run all WTF API tests for JSCOnly.

Yeah. This is list(APPEND), So TestWTF_SOURCES already contains TestWTF test cases in Tools/TestWebKitAPI/CMakeLists.txt.
So we can already run TestWTF :)

[Yusuke Suzuki]

Created attachment 276493 [details]
Patch

[Darin Adler]

Comment on attachment 276493 [details]
Patch

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

> Source/WTF/wtf/Platform.h:1163
> +/* OS X and IOS. Use CoreFoundation & GCD abstraction. */
> +#define USE_DARWIN_EVENT_LOOP 1

Reading the description here, I think the appropriate name is COCOA_EVENT_LOOP since CoreFoundation is not part of “Darwin”.

> Source/WTF/wtf/Platform.h:1173
> +#define USE_GENERIC_EVENT_LOOP 1

What are the properties of the “generic” event loop? I might have a different idea for the name, depending on what it does. After looking at the implementation a bit I am getting the impression that it’s based on WTF::Timer.

[Yusuke Suzuki]

Comment on attachment 276493 [details]
Patch

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

>> Source/WTF/wtf/Platform.h:1163
>> +#define USE_DARWIN_EVENT_LOOP 1
> 
> Reading the description here, I think the appropriate name is COCOA_EVENT_LOOP since CoreFoundation is not part of “Darwin”.

OK, COCOA includes CF and GCD. I've renamed it to USE_COCOA_EVENT_LOOP.

>> Source/WTF/wtf/Platform.h:1173
>> +#define USE_GENERIC_EVENT_LOOP 1
> 
> What are the properties of the “generic” event loop? I might have a different idea for the name, depending on what it does. After looking at the implementation a bit I am getting the impression that it’s based on WTF::Timer.

The event loop relys on WTF::Condition; when the tasks are queued, the condition notifies to the run loop.
And RunLoop::Timer is implemented on the top of these event loops.
The properties of the "generic" is that it depends on WTF threading primitives (WTF::Condition), so it is platform-independent.

How about USE_WTF_EVENT_LOOP?

[Konstantin Tokarev]

Comment on attachment 276493 [details]
Patch

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

>>> Source/WTF/wtf/Platform.h:1173
>>> +#define USE_GENERIC_EVENT_LOOP 1
>> 
>> What are the properties of the “generic” event loop? I might have a different idea for the name, depending on what it does. After looking at the implementation a bit I am getting the impression that it’s based on WTF::Timer.
> 
> The event loop relys on WTF::Condition; when the tasks are queued, the condition notifies to the run loop.
> And RunLoop::Timer is implemented on the top of these event loops.
> The properties of the "generic" is that it depends on WTF threading primitives (WTF::Condition), so it is platform-independent.
> 
> How about USE_WTF_EVENT_LOOP?

And WTF::Condition relies on C++11 threading primitives, hence my initial proposal to dub it "Cxx11" event loop.

[Yusuke Suzuki]

Created attachment 276552 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 276493 [details]
Patch

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

>>>> Source/WTF/wtf/Platform.h:1173
>>>> +#define USE_GENERIC_EVENT_LOOP 1
>>> 
>>> What are the properties of the “generic” event loop? I might have a different idea for the name, depending on what it does. After looking at the implementation a bit I am getting the impression that it’s based on WTF::Timer.
>> 
>> The event loop relys on WTF::Condition; when the tasks are queued, the condition notifies to the run loop.
>> And RunLoop::Timer is implemented on the top of these event loops.
>> The properties of the "generic" is that it depends on WTF threading primitives (WTF::Condition), so it is platform-independent.
>> 
>> How about USE_WTF_EVENT_LOOP?
> 
> And WTF::Condition relies on C++11 threading primitives, hence my initial proposal to dub it "Cxx11" event loop.

Personally, "WTF", "Generic" or "PlatformIndependent" seems good names to me.
Currently, WTF::Condition relies on C++11 threading primitives. But I think it is implementation details of the WTF::Condition.
If we change WTF::Condition to use non C++11 threading primitives, the run loop still relies on WTF::Condition, but the loop should not be called as C++11 loop.

So I think directly dependent primitives by the run loop is important; in this case, that is WTF::Condition. That's why I suggested "WTF".
But personally, I think "Generic" is also a good name. The run loop depends on the platform-independent part, so it is "generic".

[Michael Catanzaro]

I prefer "Generic" but I'd be fine with "WTF" as well.

[Michael Catanzaro]

Comment on attachment 276552 [details]
Patch

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

> Source/WTF/wtf/generic/RunLoopGeneric.cpp:84
> +    bool isActive()

This function really ought to be const. Are you having trouble with using the lock in a const function? You should be able to make m_isActiveLock mutable, right?

> Source/WTF/wtf/generic/RunLoopGeneric.cpp:214
> +void RunLoop::wakeUp(const LockHolder&)

The unused parameter is to ensure the lock is always held when the function is called?

> Source/WTF/wtf/glib/WorkQueueGLib.cpp:33
> +#include <wtf/glib/GRefPtr.h>

Adding this include can't be right, because this file hasn't been modified.

[Yusuke Suzuki]

Comment on attachment 276552 [details]
Patch

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

Thanks!

>> Source/WTF/wtf/generic/RunLoopGeneric.cpp:84
>> +    bool isActive()
> 
> This function really ought to be const. Are you having trouble with using the lock in a const function? You should be able to make m_isActiveLock mutable, right?

Or, I think std::atomic<bool> is good here. Changing.

>> Source/WTF/wtf/generic/RunLoopGeneric.cpp:214
>> +void RunLoop::wakeUp(const LockHolder&)
> 
> The unused parameter is to ensure the lock is always held when the function is called?

Yup. This is a common idiom and it is used in JSC (ConcurrentJITLocker).

>> Source/WTF/wtf/glib/WorkQueueGLib.cpp:33
>> +#include <wtf/glib/GRefPtr.h>
> 
> Adding this include can't be right, because this file hasn't been modified.

Ah, thanks. Dropping.

[Yusuke Suzuki]

Committed r199694: <http://trac.webkit.org/changeset/199694>