Bug 151391 - [GLIB] Implement garbage collector timers
Summary: [GLIB] Implement garbage collector timers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
: 118067 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-11-18 10:06 PST by Carlos Garcia Campos
Modified: 2015-12-31 16:21 PST (History)
13 users (show)

See Also:


Attachments
Patch (10.32 KB, patch)
2015-11-18 10:08 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (10.58 KB, patch)
2015-11-23 05:30 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (10.63 KB, patch)
2015-11-26 03:34 PST, Carlos Garcia Campos
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2015-11-18 10:06:07 PST
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.
Comment 1 Carlos Garcia Campos 2015-11-18 10:08:13 PST
Created attachment 265749 [details]
Patch
Comment 2 Martin Robinson 2015-11-18 10:57:38 PST
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 3 Geoffrey Garen 2015-11-18 11:06:34 PST
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.
Comment 4 Sergio Villar Senin 2015-11-18 11:09:33 PST
Maybe you should take a look at https://bugs.webkit.org/show_bug.cgi?id=118067
Comment 5 Michael Catanzaro 2015-11-18 11:51:33 PST
(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. ;)
Comment 6 Carlos Garcia Campos 2015-11-19 00:14:57 PST
(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.
Comment 7 Carlos Garcia Campos 2015-11-19 00:16:25 PST
(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.
Comment 8 Carlos Garcia Campos 2015-11-19 00:19:11 PST
(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 9 Zan Dobersek 2015-11-19 00:51:09 PST
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.
Comment 10 Carlos Garcia Campos 2015-11-19 00:59:11 PST
(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.
Comment 11 Zan Dobersek 2015-11-19 01:34:15 PST
(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 12 Zan Dobersek 2015-11-23 01:25:18 PST
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 13 Carlos Garcia Campos 2015-11-23 01:38:56 PST
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!
Comment 14 Carlos Garcia Campos 2015-11-23 05:26:20 PST
(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.
Comment 15 Carlos Garcia Campos 2015-11-23 05:30:19 PST
Created attachment 266086 [details]
Updated patch

Fixed the seconds/microseconds usage in IncrementalSweeper
Comment 16 Zan Dobersek 2015-11-26 02:03:17 PST
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 17 Carlos Garcia Campos 2015-11-26 02:09:05 PST
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.
Comment 18 Carlos Garcia Campos 2015-11-26 03:34:42 PST
Created attachment 266175 [details]
Updated patch

Addressed review comments
Comment 19 Zan Dobersek 2015-11-26 05:42:22 PST
Comment on attachment 266175 [details]
Updated patch

r=me
Comment 20 Carlos Garcia Campos 2015-11-26 05:53:02 PST
Committed r192773: <http://trac.webkit.org/changeset/192773>
Comment 21 Gyuyoung Kim 2015-11-26 19:15:30 PST
(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.
Comment 22 Michael Catanzaro 2015-12-31 16:21:00 PST
*** Bug 118067 has been marked as a duplicate of this bug. ***