Bug 86382 - Enh: Add the Ability to Disable / Enable JavaScript GC Timer
Summary: Enh: Add the Ability to Disable / Enable JavaScript GC Timer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-14 10:10 PDT by Michael Saboff
Modified: 2012-05-14 16:48 PDT (History)
0 users

See Also:


Attachments
Proposed Patch (9.35 KB, patch)
2012-05-14 10:40 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff
Patch with ChangeLog (13.10 KB, patch)
2012-05-14 10:47 PDT, Michael Saboff
darin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch with Updates from Review and Speculative Windows Build Fix (14.25 KB, patch)
2012-05-14 15:25 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2012-05-14 10:10:05 PDT
To help debug, the GCActivityCallback timer should be controllable via the users of WebKit.  We need to add the plumbing to set and clear an enable flag for the timer.
Comment 1 Michael Saboff 2012-05-14 10:40:29 PDT
Created attachment 141755 [details]
Proposed Patch
Comment 2 Michael Saboff 2012-05-14 10:47:21 PDT
Created attachment 141757 [details]
Patch with ChangeLog

Forgot ChangeLogs in the original patch posting.
Comment 3 Build Bot 2012-05-14 11:32:13 PDT
Comment on attachment 141757 [details]
Patch with ChangeLog

Attachment 141757 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12678934
Comment 4 Darin Adler 2012-05-14 12:07:05 PDT
Comment on attachment 141757 [details]
Patch with ChangeLog

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

Looks like the Windows build is broken, need this in some def file perhaps:

setGCTimerEnabled@Heap@JSC@@QAEX_N@Z

I’m going to say r=me but I think you should spend a little more time on the names of these new functions and methods.

> Source/JavaScriptCore/ChangeLog:13
> +        (JSC):

Please don’t leave lines like this in the change log. Better, consider writing per-file and per-function comments. Even better, consider fixing this problem in prepare-ChangeLog some day.

> Source/JavaScriptCore/ChangeLog:15
> +        (Heap):

Ditto.

> Source/JavaScriptCore/runtime/GCActivityCallback.h:50
> +    bool isEnabled() { return m_enabled; }

Should be const.

> Source/JavaScriptCore/runtime/GCActivityCallbackCF.cpp:67
> +    if (!heap->activityCallback()->isEnabled())
> +        return;

This seems OK for a debug-only feature. If this was a real end-user feature, then I’d suggest not even scheduling the timer when it’s disabled.

> Source/WebCore/bindings/js/GCController.h:47
> +        void setJavaScriptGCTimer(bool);

This seems like a strange function name. Given the name I’d expect to pass this a timer.

> Source/WebKit2/ChangeLog:15
> +        (WebKit):

More strange change log lines.

> Source/WebKit2/ChangeLog:17
> +        (WebContext):

More.

> Source/WebKit2/ChangeLog:20
> +        (WebKit):

More.

> Source/WebKit2/UIProcess/API/C/WKContext.h:159
> +WK_EXPORT void WKContextSetJavaScriptGCTimer(WKContextRef context, bool enable);

This name doesn’t seem great. A setter with this name should take a timer, not a boolean. Note also that the previous function spells out GC. So I’d suggest the name WKContextSetJavaScriptGarbageCollectionTimerEnabled.

> Source/WebKit/mac/Misc/WebCoreStatistics.h:50
> ++ (void)setJavaScriptGCTimer:(BOOL)enabled;

Same issue with naming here.
Comment 5 Michael Saboff 2012-05-14 15:25:28 PDT
Created attachment 141798 [details]
Patch with Updates from Review and Speculative Windows Build Fix

Posting this patch to check Windows build.

Complied with all the comments except:
> > Source/JavaScriptCore/runtime/GCActivityCallbackCF.cpp:67
> > +    if (!heap->activityCallback()->isEnabled())
> > +        return;
> 
> This seems OK for a debug-only feature. If this was a real end-user feature, then I’d suggest not even scheduling the timer when it’s disabled.

Besides being a debug-only feature, the other reason I did this was to reduce the elapsed time before we start GC'ing using the timer when the user enables the timer.
Comment 6 Michael Saboff 2012-05-14 16:48:04 PDT
Committed r117015: <http://trac.webkit.org/changeset/117015>