Bug 86382 – Enh: Add the Ability to Disable / Enable JavaScript GC Timer

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

[Michael Saboff]

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.

[Michael Saboff]

Created attachment 141755 [details]
Proposed Patch

[Michael Saboff]

Created attachment 141757 [details]
Patch with ChangeLog

Forgot ChangeLogs in the original patch posting.

[Build Bot]

Comment on attachment 141757 [details]
Patch with ChangeLog

Attachment 141757 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12678934

[Darin Adler]

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.

[Michael Saboff]

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.

[Michael Saboff]

Committed r117015: <http://trac.webkit.org/changeset/117015>