| Summary: | [JSCOnly] Implement RunLoop and remove glib dependency | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||||||||||||||||||||
| Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | achristensen, andersca, annulen, bugs-noreply, commit-queue, koivisto, mcatanzaro, ossy | ||||||||||||||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||||||
|
Description
Yusuke Suzuki
2016-03-20 16:52:45 PDT
Created attachment 275915 [details]
Patch
WIP
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.
Created attachment 276008 [details]
Patch
WIP
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.
Created attachment 276010 [details]
Patch
WIP
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.
Created attachment 276142 [details]
Patch
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. 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.
Created attachment 276143 [details]
Patch
Windows Build fix
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.
Created attachment 276149 [details]
Patch
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.
Created attachment 276242 [details]
Patch
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.
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. 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 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) Created attachment 276246 [details]
Patch
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 :) 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.
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" 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. Created attachment 276247 [details]
Patch
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.
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 Created attachment 276248 [details]
Patch
Created attachment 276249 [details]
Patch
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. Created attachment 276251 [details]
Patch
Created attachment 276253 [details]
Patch
Created attachment 276254 [details]
Patch
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? Created attachment 276257 [details]
Patch
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. Created attachment 276258 [details]
Patch
(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 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. Created attachment 276265 [details]
Patch
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. (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. 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. 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. 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. (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 :) Created attachment 276373 [details]
Patch
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). 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). I don't like this. I don't think we should have our own run loop implementation in WTF. (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 (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. (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. 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. 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. 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. 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. 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 :) Created attachment 276493 [details]
Patch
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. 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? 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. Created attachment 276552 [details]
Patch
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". I prefer "Generic" but I'd be fine with "WTF" as well. 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. 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. Committed r199694: <http://trac.webkit.org/changeset/199694> |