Bug 60990 – JSGlobalContextRelease should not trigger a synchronous garbage collection

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

JSGlobalContextRelease should not trigger a synchronous garbage collection

[Sam Weinig]

Created attachment 93818 [details]
Patch

[Oliver Hunt]

Comment on attachment 93818 [details]
Patch

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

> Source/JavaScriptCore/API/JSContextRef.cpp:142
> +    //   garbarge collect soon.

garbarge?

[Sam Weinig]

Committed r86712: <http://trac.webkit.org/changeset/86712>

[Adam Roben (:aroben)]

What's the motivation for this? Neither this bug nor the ChangeLog say!

[Sam Weinig]

(In reply to comment #4)
> What's the motivation for this? Neither this bug nor the ChangeLog say!

The motivation is to coalesce garbage collections that are triggered as a result of releasing a global context.  Otherwise, repeated calls to JSGlobalContextRelease would each trigger a full collection, where one at the end would be much better.

As a general rule, synchronous full garbage collection should be avoided as it is very expensive, and should definitely not happen unexpectedly as was happening with JSGlobalContextRelease.

[Alexey Proskuryakov]

We discussed this in person, but I'm not sure how the landed version avoids leaking objects in a worker context. Worker threads don't even have CFRunLoops to schedule timers on.

Is this patch correct?

[Geoffrey Garen]

> We discussed this in person, but I'm not sure how the landed version avoids leaking objects in a worker context. Worker threads don't even have CFRunLoops to schedule timers on.

When a worker thread exits, it releases the last reference to its context group (JSGlobalData), which calls Heap::destroy().

[Sam Weinig]

(In reply to comment #6)
> We discussed this in person, but I'm not sure how the landed version avoids leaking objects in a worker context. Worker threads don't even have CFRunLoops to schedule timers on.
> 
> Is this patch correct?

What Geoff said.

[Alexey Proskuryakov]

Thanks. I verified that "run-webkit-tests fast/workers --leaks" reports no leaks.

What about arbitrary clients of JSC API? These still don't need to have a CFRunLoop, which DefaultGCActivityCallback seems to require.

[Adam Roben (:aroben)]

(In reply to comment #5)
> (In reply to comment #4)
> > What's the motivation for this? Neither this bug nor the ChangeLog say!
> 
> The motivation is to coalesce garbage collections that are triggered as a result of releasing a global context.  Otherwise, repeated calls to JSGlobalContextRelease would each trigger a full collection, where one at the end would be much better.
> 
> As a general rule, synchronous full garbage collection should be avoided as it is very expensive, and should definitely not happen unexpectedly as was happening with JSGlobalContextRelease.

Thanks, Sam!

[Geoffrey Garen]

> What about arbitrary clients of JSC API? These still don't need to have a CFRunLoop, which DefaultGCActivityCallback seems to require.

As we've discussed before, on CF platforms, DefaultGCActivityCallback is optimized for CFRunLoop, but doesn't require it.

[Alexey Proskuryakov]

For my education, could you please describe how it works without CFRunLoop?

The only call to heap->collectAllGarbage() in CGActivityCallbackCF.cpp is in DefaultGCActivityCallbackPlatformData::trigger(), and the only call to trigger() is through a CFRunLoopTimer. This is the only GCActivityCallback subclass in JSC.

[Geoffrey Garen]

(In reply to comment #12)
> For my education, could you please describe how it works without CFRunLoop?

If a CF-configured client doesn't run the CFRunLoop, DefaultGCActivityCallback's optimization to GC from the runloop just doesn't kick in.

[Alexey Proskuryakov]

> DefaultGCActivityCallback's optimization to GC from the runloop just doesn't kick in

This patch replaced code that works in such threads (globalData.heap.collectAllGarbage()) with code that's effectively no-op. Are you saying that this is OK?

[Geoffrey Garen]

> This patch replaced code that works in such threads (globalData.heap.collectAllGarbage()) with code that's effectively no-op. Are you saying that this is OK?

I don't think that's an accurate characterization. The old code didn't "work." It sometimes eagerly reclaimed memory, sometimes didn't eagerly reclaim memory, and sometimes caused O(N^2) hangs.

The new code does coalesced attempts at reclaiming memory on threads that run the CFRunLoop, and nothing on threads that don't. Whether that's OK or not depends on the thread and its requirements. On all the threads that I've observed in WebKit, the new code resolves the O(N^2) issue without a regression in memory use.

[Alexey Proskuryakov]

The code was added a long time ago (http://trac.webkit.org/changeset/35917), and unfortunately, the commit that added it didn't have a lot of detail in ChangeLog. All I remember is that non-WebKit JavaScriptCore API clients' needs dictated when JSGlobalContextRelease should perform GC.