Bug 41318 - GC should reclaim garbage even when new objects are not being allocated rapidly
Summary: GC should reclaim garbage even when new objects are not being allocated rapidly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nathan Lawrence
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-28 16:45 PDT by Nathan Lawrence
Modified: 2010-08-03 13:34 PDT (History)
8 users (show)

See Also:


Attachments
Patch (9.79 KB, patch)
2010-06-28 16:45 PDT, Nathan Lawrence
no flags Details | Formatted Diff | Diff
Patch (9.85 KB, patch)
2010-06-28 17:14 PDT, Nathan Lawrence
no flags Details | Formatted Diff | Diff
Patch (9.85 KB, patch)
2010-06-28 19:53 PDT, Nathan Lawrence
no flags Details | Formatted Diff | Diff
Patch (callback in separate file) (19.09 KB, patch)
2010-06-29 14:09 PDT, Nathan Lawrence
ggaren: review-
Details | Formatted Diff | Diff
Patch (24.87 KB, patch)
2010-07-13 00:13 PDT, Nathan Lawrence
no flags Details | Formatted Diff | Diff
Patch (24.87 KB, patch)
2010-07-13 07:28 PDT, Nathan Lawrence
no flags Details | Formatted Diff | Diff
Patch (24.67 KB, patch)
2010-07-13 08:50 PDT, Nathan Lawrence
no flags Details | Formatted Diff | Diff
Patch (with smart pointers) (25.16 KB, patch)
2010-07-13 13:36 PDT, Nathan Lawrence
ggaren: review-
Details | Formatted Diff | Diff
Patch (22.00 KB, patch)
2010-07-22 14:40 PDT, Nathan Lawrence
oliver: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nathan Lawrence 2010-06-28 16:45:27 PDT
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.
Comment 1 WebKit Review Bot 2010-06-28 16:48:00 PDT
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.
Comment 2 Early Warning System Bot 2010-06-28 16:59:09 PDT
Attachment 59959 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3287968
Comment 3 Nathan Lawrence 2010-06-28 17:14:32 PDT
Created attachment 59962 [details]
Patch

Should fix build problems on non macintosh platforms.
Comment 4 WebKit Review Bot 2010-06-28 17:37:38 PDT
Attachment 59959 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3311983
Comment 5 Mark Rowe (bdash) 2010-06-28 17:42:27 PDT
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.
Comment 6 Early Warning System Bot 2010-06-28 17:46:57 PDT
Attachment 59962 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3323917
Comment 7 WebKit Review Bot 2010-06-28 17:52:00 PDT
Attachment 59959 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3291940
Comment 8 WebKit Review Bot 2010-06-28 19:27:48 PDT
Attachment 59962 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3304939
Comment 9 Nathan Lawrence 2010-06-28 19:53:31 PDT
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.
Comment 10 WebKit Review Bot 2010-06-28 21:16:38 PDT
Attachment 59962 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3301990
Comment 11 WebKit Review Bot 2010-06-29 02:12:51 PDT
Attachment 59978 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3283931
Comment 12 Nathan Lawrence 2010-06-29 14:09:00 PDT
Created attachment 60050 [details]
Patch (callback in separate file)
Comment 13 WebKit Review Bot 2010-06-29 14:14:08 PDT
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.
Comment 14 Geoffrey Garen 2010-07-02 15:43:26 PDT
+    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.
Comment 15 Nathan Lawrence 2010-07-13 00:13:20 PDT
Created attachment 61335 [details]
Patch
Comment 16 WebKit Review Bot 2010-07-13 00:19:34 PDT
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.
Comment 17 Early Warning System Bot 2010-07-13 00:29:09 PDT
Attachment 61335 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3539002
Comment 18 WebKit Review Bot 2010-07-13 02:50:10 PDT
Attachment 61335 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3486273
Comment 19 Nathan Lawrence 2010-07-13 07:28:01 PDT
Created attachment 61373 [details]
Patch
Comment 20 Early Warning System Bot 2010-07-13 07:42:44 PDT
Attachment 61373 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3484283
Comment 21 WebKit Review Bot 2010-07-13 08:20:45 PDT
Attachment 61373 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3484289
Comment 22 Nathan Lawrence 2010-07-13 08:50:29 PDT
Created attachment 61382 [details]
Patch
Comment 23 Nathan Lawrence 2010-07-13 13:36:51 PDT
Created attachment 61411 [details]
Patch (with smart pointers)
Comment 24 Geoffrey Garen 2010-07-16 16:09:15 PDT
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.
Comment 25 Nathan Lawrence 2010-07-22 14:40:58 PDT
Created attachment 62343 [details]
Patch

moved where the callback gets called from, verified no regression on sunspider, + other changes suggested by Geoff.
Comment 26 WebKit Commit Bot 2010-07-28 07:23:02 PDT
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
Comment 27 Geoffrey Garen 2010-08-03 13:34:36 PDT
Committed revision 64585.