JSGlobalContextRelease should not trigger a synchronous garbage collection
Created attachment 93818 [details] Patch
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?
Committed r86712: <http://trac.webkit.org/changeset/86712>
What's the motivation for this? Neither this bug nor the ChangeLog say!
(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.
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?
> 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().
(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.
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.
(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!
> 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.
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.
(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.
> 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?
> 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.
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.