Bug 168363 – [GLib] GCActivityCallback::scheduleTimer() keeps pushing dispatch into the future

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Zan Dobersek]

GCActivityCallback::scheduleTimer() accepts a delay argument and, in case it's at most half the amount of the current delay, uses this new value to reschedule the internal timer.
https://trac.webkit.org/browser/trunk/Source/JavaScriptCore/heap/GCActivityCallback.cpp#L119

Say the GCActivityCallback is scheduled for the first time in 8 seconds. The timer is then set to dispatch 8 seconds from the current time (marked as CT#0). After the JS heap continues to grow, additional scheduleTimer() calls are done, each time decreasing the delay proportional to the amount of memory that's been allocated so far. This is calculated in GCActivityCallback::didAllocate().
https://trac.webkit.org/browser/trunk/Source/JavaScriptCore/heap/GCActivityCallback.cpp#L150

It's possible that the allocations progress in such way that, for example, after CT#0 + 7 seconds the computed delay drops to 4 seconds. At that point the call to scheduleTimer() reschedules the timer, but using the current time as the base, CT#1, which equals to CT#0 + 7, and uses 4 seconds as the delay amount, which means the next dispatch will be at CT#1 + 4 = CT#0 + 11 seconds, three seconds after the time when the first scheduled delay was supposed to dispatch.

This isn't how the USE(CF) implementation works. When the delay is set there for the first time, it's set as the latest time when the callback should be fired. Repeating the previous scenario, if the timer is first scheduled to be dispatched after 8 seconds from the current time (CT#0 + 8), and after 7 seconds the delay is dropped to 4 seconds, the new dispatch time will be CT#0 + 4, which is 3 seconds in the past, meaning the timer will dispatch ASAP.

The current GLib implementation should probably match that. Note that the current delay computation doesn't mean that GC doesn't get triggered at all, as far as I've tested it just means that the dispatch gets delayed long enough for the delay values to become really small and close enough to the present that the timer gets dispatched in the near-future main loop iteration.

The delays here are used to showcase the problem. The initial delays are usually quite large, but then become smaller as the allocations grow.

The USE(CF) implementation still sets m_nextFireTime to currentTime() + delay, just like we do, but note that there appears to be no use of the m_nextFireTime member variable or the GCActivityCallback::nextFireTime() getter.

[Zan Dobersek]

Handling this similarly to USE(CF) is a bit hard because we also support disabling the timer completely (via a -1 delay), instead of just scheduling it a decade into the future. This is a bit of a problem because we then treat an infinite delay as a valid one, which results in setting the GSource's ready time to G_MAXINT64.

We should probably just simplify this by doing the decade thing.

[Zan Dobersek]

Created attachment 301736 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 301736 [details]
Patch

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

Thanks!

> Source/JavaScriptCore/heap/HeapTimer.cpp:190
> +        g_source_set_ready_time(heapTimer.m_timer.get(), g_get_monotonic_time() + HeapTimer::s_decade * G_USEC_PER_SEC);

Why don't we do this before calling the callback as we did before? There you have the source already as parameter

[Zan Dobersek]

Comment on attachment 301736 [details]
Patch

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

>> Source/JavaScriptCore/heap/HeapTimer.cpp:190
>> +        g_source_set_ready_time(heapTimer.m_timer.get(), g_get_monotonic_time() + HeapTimer::s_decade * G_USEC_PER_SEC);
> 
> Why don't we do this before calling the callback as we did before? There you have the source already as parameter

Because HeapTimer::s_decade isn't accessible from there. heapTimerSourceFunctions could be moved into the HeapTimer class, but that would require bringing in all the GLib headers in HeapTimer.h. So there's different trade-offs.

[Zan Dobersek]

Comment on attachment 301736 [details]
Patch

Clearing flags on attachment: 301736

Committed r212541: <http://trac.webkit.org/changeset/212541>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.