Bug 151391 – [GLIB] Implement garbage collector timers

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 265749 [details]
Patch

[Martin Robinson]

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?

[Geoffrey Garen]

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.

[Sergio Villar Senin]

Maybe you should take a look at https://bugs.webkit.org/show_bug.cgi?id=118067

[Michael Catanzaro]

(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. ;)

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

(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.

[Zan Dobersek]

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.

[Carlos Garcia Campos]

(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.

[Zan Dobersek]

(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.

[Zan Dobersek]

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.

[Carlos Garcia Campos]

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!

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 266086 [details]
Updated patch

Fixed the seconds/microseconds usage in IncrementalSweeper

[Zan Dobersek]

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.

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 266175 [details]
Updated patch

Addressed review comments

[Zan Dobersek]

Comment on attachment 266175 [details]
Updated patch

r=me

[Carlos Garcia Campos]

Committed r192773: <http://trac.webkit.org/changeset/192773>

[Gyuyoung Kim]

(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.

[Michael Catanzaro]

*** Bug 118067 has been marked as a duplicate of this bug. ***