While debugging bug #143261, looking at what ports do differently in JSC I've noticed that GTK+ port doesn't implement the GC timers at all. Ports using CF and EFL port, using their own timers, do implement them, but other ports using GLib don't. I have no idea why we have never implemented them, but we should.
Created attachment 265749 [details] Patch
Comment on attachment 265749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265749&action=review Great work! I noticed this was missing yesterday too, so I'm very happy that you fixed it. I just have a couple quick questions before I feel comfortable giving an r+... > Source/JavaScriptCore/heap/EdenGCActivityCallback.cpp:33 > -#if USE(CF) || PLATFORM(EFL) > +#if USE(CF) || USE(GLIB) Is USE(GLIB) also true for EFL? I wonder if it wouldn't be better to be more explicit here for the sake of code readability and leave this as: #if USE(CF) || PLATFORM(EFL) || USE(GLIB) > Source/JavaScriptCore/heap/HeapTimer.cpp:155 > + if (g_source_get_ready_time(source) == -1) > + return G_SOURCE_CONTINUE; In what circumstances can this happen?
Comment on attachment 265749 [details] Patch It's a shame to have so much platform-specific timer code here. Would be nice just to migrate our shared timer abstraction from WebCore down into WTF.
Maybe you should take a look at https://bugs.webkit.org/show_bug.cgi?id=118067
(In reply to comment #2) > Is USE(GLIB) also true for EFL? Yes. Otherwise, they wouldn't have much fun trying to use libsoup etc. ;)
(In reply to comment #2) > Comment on attachment 265749 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=265749&action=review > > Great work! I noticed this was missing yesterday too, so I'm very happy that > you fixed it. I just have a couple quick questions before I feel comfortable > giving an r+... > > > Source/JavaScriptCore/heap/EdenGCActivityCallback.cpp:33 > > -#if USE(CF) || PLATFORM(EFL) > > +#if USE(CF) || USE(GLIB) > > Is USE(GLIB) also true for EFL? I wonder if it wouldn't be better to be more > explicit here for the sake of code readability and leave this as: Yes, it's true. > #if USE(CF) || PLATFORM(EFL) || USE(GLIB) Looks redundant to me, the fewer || the better IMO. > > Source/JavaScriptCore/heap/HeapTimer.cpp:155 > > + if (g_source_get_ready_time(source) == -1) > > + return G_SOURCE_CONTINUE; > > In what circumstances can this happen? If the timer is cancelled in a different thread right before being dispatched by the main context.
(In reply to comment #3) > Comment on attachment 265749 [details] > Patch > > It's a shame to have so much platform-specific timer code here. Would be > nice just to migrate our shared timer abstraction from WebCore down into WTF. I thought exactly the same when writing the patch. I think RunLoop::Timer would work for us.
(In reply to comment #4) > Maybe you should take a look at > https://bugs.webkit.org/show_bug.cgi?id=118067 I didn't know there was a bug for this already. The patch there has a lot of issues in any case, I can review it if you want, though.
Comment on attachment 265749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265749&action=review >>> Source/JavaScriptCore/heap/EdenGCActivityCallback.cpp:33 >>> +#if USE(CF) || USE(GLIB) >> >> Is USE(GLIB) also true for EFL? I wonder if it wouldn't be better to be more explicit here for the sake of code readability and leave this as: >> >> #if USE(CF) || PLATFORM(EFL) || USE(GLIB) > > Yes, it's true. Why USE(GLIB) then? It's true for both EFL and GTK, but the implementations of HeapTimer differ for the two ports. Since all these classes are based on HeapTimer, I think the PLATFORM(EFL) || PLATFORM(GTK) should be used throughout this patch, instead of bundling them under USE(GLIB). > Source/JavaScriptCore/heap/HeapTimer.cpp:41 > #if PLATFORM(EFL) > #include <Ecore.h> > +#elif USE(GLIB) > +#include <glib.h> > #endif Just one example, you end up doing this. It's counter-intuitive because PLATFORM(EFL) falls under USE(GLIB), but you still have to special-case it because of the different implementations. > Source/JavaScriptCore/heap/HeapTimer.cpp:174 > + g_source_attach(m_timer.get(), g_main_context_get_thread_default()); As it stands these timers only fire on main threads, since worker threads aren't run via a main loop.
(In reply to comment #9) > Comment on attachment 265749 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=265749&action=review > > >>> Source/JavaScriptCore/heap/EdenGCActivityCallback.cpp:33 > >>> +#if USE(CF) || USE(GLIB) > >> > >> Is USE(GLIB) also true for EFL? I wonder if it wouldn't be better to be more explicit here for the sake of code readability and leave this as: > >> > >> #if USE(CF) || PLATFORM(EFL) || USE(GLIB) > > > > Yes, it's true. > > Why USE(GLIB) then? It's true for both EFL and GTK, but the implementations > of HeapTimer differ for the two ports. > > Since all these classes are based on HeapTimer, I think the PLATFORM(EFL) || > PLATFORM(GTK) should be used throughout this patch, instead of bundling them > under USE(GLIB). Because this would work for any other port using glib, like WebKit4Wayland, EFL is the exception here, not glib. > > Source/JavaScriptCore/heap/HeapTimer.cpp:41 > > #if PLATFORM(EFL) > > #include <Ecore.h> > > +#elif USE(GLIB) > > +#include <glib.h> > > #endif > > Just one example, you end up doing this. It's counter-intuitive because > PLATFORM(EFL) falls under USE(GLIB), but you still have to special-case it > because of the different implementations. Yes, because EFL uses GLIB but its own timer implementation. The EFL implementation is specific to EFL, but the GLIB one is not specific to GTK. > > Source/JavaScriptCore/heap/HeapTimer.cpp:174 > > + g_source_attach(m_timer.get(), g_main_context_get_thread_default()); > > As it stands these timers only fire on main threads, since worker threads > aren't run via a main loop. g_main_context_get_thread_default() will return the main thread context from threads not using a main loop in any case. But I could use the main context here to make it explicit.
(In reply to comment #10) > (In reply to comment #9) > > Comment on attachment 265749 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=265749&action=review > > > > >>> Source/JavaScriptCore/heap/EdenGCActivityCallback.cpp:33 > > >>> +#if USE(CF) || USE(GLIB) > > >> > > >> Is USE(GLIB) also true for EFL? I wonder if it wouldn't be better to be more explicit here for the sake of code readability and leave this as: > > >> > > >> #if USE(CF) || PLATFORM(EFL) || USE(GLIB) > > > > > > Yes, it's true. > > > > Why USE(GLIB) then? It's true for both EFL and GTK, but the implementations > > of HeapTimer differ for the two ports. > > > > Since all these classes are based on HeapTimer, I think the PLATFORM(EFL) || > > PLATFORM(GTK) should be used throughout this patch, instead of bundling them > > under USE(GLIB). > > Because this would work for any other port using glib, like WebKit4Wayland, > EFL is the exception here, not glib. > Makes me wish for USE(ECORE) then.
Comment on attachment 265749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265749&action=review As the next step, WebCore::GCController should be updated so that PLATFORM(GTK) matches USE(CF), primarily to move the GCController's timer away from MainThreadSharedTimer. > Source/JavaScriptCore/heap/HeapTimer.cpp:171 > + static_cast<HeapTimer*>(userData)->timerDidFire(); What do you think of just writing out ::timerDidFire() in this lambda? > Source/JavaScriptCore/heap/IncrementalSweeper.cpp:75 > + g_source_set_ready_time(m_timer.get(), g_get_monotonic_time() + (sweepTimeSlice * sweepTimeMultiplier)); You're adding 0.1 seconds to g_get_monotonic_time(), which returns microseconds.
Comment on attachment 265749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265749&action=review >> Source/JavaScriptCore/heap/HeapTimer.cpp:171 >> + static_cast<HeapTimer*>(userData)->timerDidFire(); > > What do you think of just writing out ::timerDidFire() in this lambda? We can't capture this in a lambda wrapping a c function pointer. >> Source/JavaScriptCore/heap/IncrementalSweeper.cpp:75 >> + g_source_set_ready_time(m_timer.get(), g_get_monotonic_time() + (sweepTimeSlice * sweepTimeMultiplier)); > > You're adding 0.1 seconds to g_get_monotonic_time(), which returns microseconds. Good catch!
(In reply to comment #12) > Comment on attachment 265749 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=265749&action=review > > As the next step, WebCore::GCController should be updated so that > PLATFORM(GTK) matches USE(CF), primarily to move the GCController's timer > away from MainThreadSharedTimer. > Right, good point, we can do that in a follow up, though.
Created attachment 266086 [details] Updated patch Fixed the seconds/microseconds usage in IncrementalSweeper
Comment on attachment 266086 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=266086&action=review > Source/JavaScriptCore/heap/GCActivityCallback.cpp:125 > +void GCActivityCallback::scheduleTimer(double newDelay) This method should assert that the passed-in delay is positive or zero. > Source/JavaScriptCore/heap/IncrementalSweeper.cpp:77 > + gint64 targetTime = currentTime + std::min<gint64>(G_MAXINT64 - currentTime, delayDuration.count()); You still need to cast the duration to microseconds.
Comment on attachment 266086 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=266086&action=review >> Source/JavaScriptCore/heap/IncrementalSweeper.cpp:77 >> + gint64 targetTime = currentTime + std::min<gint64>(G_MAXINT64 - currentTime, delayDuration.count()); > > You still need to cast the duration to microseconds. Oh, right, I forgot the cast. Thanks for the review.
Created attachment 266175 [details] Updated patch Addressed review comments
Comment on attachment 266175 [details] Updated patch r=me
Committed r192773: <http://trac.webkit.org/changeset/192773>
(In reply to comment #10) > Because this would work for any other port using glib, like WebKit4Wayland, > EFL is the exception here, not glib. > > > > Source/JavaScriptCore/heap/HeapTimer.cpp:41 > > > #if PLATFORM(EFL) > > > #include <Ecore.h> > > > +#elif USE(GLIB) > > > +#include <glib.h> > > > #endif > > > > Just one example, you end up doing this. It's counter-intuitive because > > PLATFORM(EFL) falls under USE(GLIB), but you still have to special-case it > > because of the different implementations. > > Yes, because EFL uses GLIB but its own timer implementation. The EFL > implementation is specific to EFL, but the GLIB one is not specific to GTK. > EFL port has been used GLIB to use libsoup, gstreamer and other minor functionalities. However our policy is to consider to use EFL library first if EFL supports the library we need to use. In this case we should keep PLATFORM(EFL) for timer because we implements it using ECore_Timer.
*** Bug 118067 has been marked as a duplicate of this bug. ***