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.
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.
Created attachment 301736 [details] Patch
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
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.
Comment on attachment 301736 [details] Patch Clearing flags on attachment: 301736 Committed r212541: <http://trac.webkit.org/changeset/212541>
All reviewed patches have been landed. Closing bug.