Bug 41318 – GC should reclaim garbage even when new objects are not being allocated rapidly

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Nathan Lawrence]

Created attachment 59959 [details]
Patch

It would be nice to have the GC run a few seconds after the last allocation.  We can approximate this by having a timer start running at the beginning of allocation, and resetting the timer whenever the heap is reset.  If there are a number of allocations happening, then the timer will always get reset before it times out and the GC is called.  If there are very few allocations happening then the timer fire and we will collect the garbage.

[WebKit Review Bot]

Attachment 59959 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/js/WebCoreJSClientData.h:87:  Missing space after ,  [whitespace/comma] [3]
JavaScriptCore/runtime/Collector.cpp:982:  Declaration has space between type name and * in Heap *heap  [whitespace/declaration] [3]
JavaScriptCore/runtime/Collector.cpp:1001:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
JavaScriptCore/runtime/Collector.h:85:  Missing space after ,  [whitespace/comma] [3]
JavaScriptCore/runtime/Collector.h:161:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 5 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 59959 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3287968

[Nathan Lawrence]

Created attachment 59962 [details]
Patch

Should fix build problems on non macintosh platforms.

[WebKit Review Bot]

Attachment 59959 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3311983

[Mark Rowe (bdash)]

Putting CF code directly in to Heap is pretty nasty.  I also do not think there is any  guarantee that the main thread is spinning a CF runloop from JavaScriptCore’s point of view.  I also don’t think OS(DARWIN) is the right condition to use since Darwin proper doesn’t include CoreFoundation.

[Early Warning System Bot]

Attachment 59962 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3323917

[WebKit Review Bot]

Attachment 59959 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3291940

[WebKit Review Bot]

Attachment 59962 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3304939

[Nathan Lawrence]

Created attachment 59978 [details]
Patch

This should fix compilation issues on platforms that do not have ENABLE(JSC_MULTIPLE_THREADS).  It also changes OS(DARWIN) to PLATFORM(CF) to check for core foundation.

[WebKit Review Bot]

Attachment 59962 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3301990

[WebKit Review Bot]

Attachment 59978 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3283931

[Nathan Lawrence]

Created attachment 60050 [details]
Patch (callback in separate file)

[WebKit Review Bot]

Attachment 60050 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/runtime/GCActivityCallback.h:29:  #ifndef header guard has wrong style, please use: GCActivityCallback_h  [build/header_guard] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

+    runtime/GCActivityCallback.cpp
+    runtime/GCActivityCallbackCF.cpp

Project files should only use one or the other. This also means that you can remove the #if(!CF) from the non-CF file.


+    : m_callback(defaultGCActivityCallback)
+    , m_callbackData(0)

Rename to m_activityCallback*


+        typedef void (*GCCallback)(Heap*, void*);

Move to CGActivityCallback.h, and rename to GCActivityCallback.


+        void setGCCallback(GCCallback, void*);

setActivityCallback


+void defaultGCActivityCallback(Heap* heap, void*)
+{
+    static CFRunLoopTimerRef timer;
+    static CFRunLoopTimerContext ctx;

Not thread safe.

Might invoke timer after heap has been destroyed.

Leaks one timer per heap.

Maybe change this callback to a callback object with a proper constructor and destructor and a virtual operator(), or add a createData and destroyData callback.

[Nathan Lawrence]

Created attachment 61335 [details]
Patch

[WebKit Review Bot]

Attachment 61335 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/runtime/Collector.h:137:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 61335 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3539002

[WebKit Review Bot]

Attachment 61335 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3486273

[Nathan Lawrence]

Created attachment 61373 [details]
Patch

[Early Warning System Bot]

Attachment 61373 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3484283

[WebKit Review Bot]

Attachment 61373 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3484289

[Nathan Lawrence]

Created attachment 61382 [details]
Patch

[Nathan Lawrence]

Created attachment 61411 [details]
Patch (with smart pointers)

[Geoffrey Garen]

Comment on attachment 61411 [details]
Patch (with smart pointers)

I think the callback invocation should be inside reset(), so that the timer is active even after events like collections due to extra cost.

It would be better not to rely on calling out to WebCore just to achieve a cross-platform timer implementation. I would nix the WebCore code, and, as you suggested, add a FIXME explaining how other platforms can get the right behavior by porting the WebCore timer code to WTF.

The canonical name we use for platform-speicifc data structure is Platform*. In this case, DefaultGCActivityCallbackPlatformData.

Please post SunSpider results for this change.

[Nathan Lawrence]

Created attachment 62343 [details]
Patch

moved where the callback gets called from, verified no regression on sunspider, + other changes suggested by Geoff.

[WebKit Commit Bot]

Comment on attachment 62343 [details]
Patch

Rejecting patch 62343 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Oliver Hunt', u'--force']" exit_code: 1
Last 500 characters of output:
ts to file JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj.rej
patching file JavaScriptCore/jit/JITPropertyAccess.cpp
patching file JavaScriptCore/runtime/Collector.cpp
patching file JavaScriptCore/runtime/Collector.h
patching file JavaScriptCore/runtime/GCActivityCallback.cpp
patching file JavaScriptCore/runtime/GCActivityCallback.h
patching file JavaScriptCore/runtime/GCActivityCallbackCF.cpp
patching file JavaScriptCore/runtime/JSLock.cpp
patching file JavaScriptCore/runtime/JSLock.h

Full output: http://queues.webkit.org/results/3618235

[Geoffrey Garen]

Committed revision 64585.