JSC: Add LLINT and baseline JIT support for timing out scripts

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

This work currently adds polling for an "interrupt” request in the LLINT and baseline JIT.  When the feature is enabled, the DFG JIT will be disabled and previously compiled functions will be released to allows them be re-compiled with the baseline JIT if needed.  Timeouts is not supported in the DFG JIT yet.

Patch and benchmark results coming shortly.

[Mark Lam]

Created attachment 197965 [details]
The patch.

[Mark Lam]

Created attachment 197966 [details]
benchmark result: LLINT only, run 1

[Mark Lam]

Created attachment 197967 [details]
benchmark result: LLINT only, run 2

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSContextRef.cpp', u'Source/JavaScriptCore/API/JSContextRef.h', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcproj', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JIT.h', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/jit/JITStubs.h', u'Source/JavaScriptCore/llint/LLIntExceptions.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.h', u'Source/JavaScriptCore/llint/LowLevelInterpreter.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter64.asm', u'Source/JavaScriptCore/runtime/ExceptionHelpers.cpp', u'Source/JavaScriptCore/runtime/ExceptionHelpers.h', u'Source/JavaScriptCore/runtime/JSGlobalData.h', u'Source/JavaScriptCore/runtime/JSInterrupts.cpp', u'Source/JavaScriptCore/runtime/JSInterrupts.h', u'Source/JavaScriptCore/runtime/JSInterruptsMac.cpp', u'Source/JavaScriptCore/runtime/JSInterruptsNone.cpp', u'Source/JavaScriptCore/runtime/Terminator.h', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/CurrentTime.cpp', u'Source/WTF/wtf/CurrentTime.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/JSEventListener.cpp', u'Source/WebCore/bindings/js/WorkerScriptController.cpp']" exit_code: 1
Source/JavaScriptCore/runtime/JSInterruptsMac.cpp:41:  Missing space before {  [whitespace/braces] [5]
Source/JavaScriptCore/runtime/JSInterrupts.cpp:35:  NEVER_TIMEOUT is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/API/JSContextRef.h:95:  JSSCRIPT_NEVER_TIMEOUT is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 33 files


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

[Mark Lam]

Created attachment 197968 [details]
benchmark result: LLINT + baseline JIT only, run 1

[Mark Lam]

Created attachment 197969 [details]
benchmark result: LLINT + baseline JIT only, run 2

[Mark Lam]

Created attachment 197970 [details]
benchmark result: LLINT + baseline JIT + DFG, run 1

[Mark Lam]

Created attachment 197971 [details]
benchmark result: LLINT + baseline JIT + DFG, run 2

[Early Warning System Bot]

Comment on attachment 197965 [details]
The patch.

Attachment 197965 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/72285

[kov's GTK+ EWS bot]

Comment on attachment 197965 [details]
The patch.

Attachment 197965 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/127079

[Build Bot]

Comment on attachment 197965 [details]
The patch.

Attachment 197965 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/72283

[Build Bot]

Comment on attachment 197965 [details]
The patch.

Attachment 197965 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/93107

[Mark Lam]

Created attachment 197974 [details]
the patch + build fixes.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSContextRef.cpp', u'Source/JavaScriptCore/API/JSContextRef.h', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcproj', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JIT.h', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/jit/JITStubs.h', u'Source/JavaScriptCore/llint/LLIntExceptions.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.h', u'Source/JavaScriptCore/llint/LowLevelInterpreter.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter64.asm', u'Source/JavaScriptCore/runtime/ExceptionHelpers.cpp', u'Source/JavaScriptCore/runtime/ExceptionHelpers.h', u'Source/JavaScriptCore/runtime/JSGlobalData.h', u'Source/JavaScriptCore/runtime/JSInterrupts.cpp', u'Source/JavaScriptCore/runtime/JSInterrupts.h', u'Source/JavaScriptCore/runtime/JSInterruptsMac.cpp', u'Source/JavaScriptCore/runtime/JSInterruptsNone.cpp', u'Source/JavaScriptCore/runtime/Terminator.h', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/CurrentTime.cpp', u'Source/WTF/wtf/CurrentTime.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/JSEventListener.cpp', u'Source/WebCore/bindings/js/WorkerScriptController.cpp']" exit_code: 1
Source/JavaScriptCore/runtime/JSInterruptsMac.cpp:41:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 33 files


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

[Build Bot]

Comment on attachment 197974 [details]
the patch + build fixes.

Attachment 197974 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/52263

New failing tests:
http/tests/ssl/ping-with-unsafe-redirect.html
fast/workers/termination-with-port-messages.html
http/tests/workers/terminate-during-sync-operation.html

[Build Bot]

Created attachment 197992 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Mark Lam]

(In reply to comment #15)
> (From update of attachment 197974 [details])
> Attachment 197974 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/52263
> 
> New failing tests:
> http/tests/ssl/ping-with-unsafe-redirect.html
> fast/workers/termination-with-port-messages.html
> http/tests/workers/terminate-during-sync-operation.html

http/tests/ssl/ping-with-unsafe-redirect.html is due to a missing expected file.  This was already the case in the baseline before this patch.

fast/workers/termination-with-port-messages.html appears to pass when run by itself.  The failure must have been due to something else run in its proximity.  I’ll investigate with a full run.

http/tests/workers/terminate-during-sync-operation.html appears to be due to this script termination now kicking in where it shouldn’t have before.  I will investigate the test and how it is triggering the script termination.

[Build Bot]

Comment on attachment 197974 [details]
the patch + build fixes.

Attachment 197974 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/127178

New failing tests:
http/tests/ssl/ping-with-unsafe-redirect.html
fast/workers/termination-with-port-messages.html

[Build Bot]

Created attachment 198000 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Mark Lam]

Created attachment 198011 [details]
patch 3: bug fix in throwTerminatedExecutionException(), and added missing entry in vcxproj.filter file.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSContextRef.cpp', u'Source/JavaScriptCore/API/JSContextRef.h', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcproj', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JIT.h', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/jit/JITStubs.h', u'Source/JavaScriptCore/llint/LLIntExceptions.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.h', u'Source/JavaScriptCore/llint/LowLevelInterpreter.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter64.asm', u'Source/JavaScriptCore/runtime/ExceptionHelpers.cpp', u'Source/JavaScriptCore/runtime/ExceptionHelpers.h', u'Source/JavaScriptCore/runtime/JSGlobalData.h', u'Source/JavaScriptCore/runtime/JSInterrupts.cpp', u'Source/JavaScriptCore/runtime/JSInterrupts.h', u'Source/JavaScriptCore/runtime/JSInterruptsMac.cpp', u'Source/JavaScriptCore/runtime/JSInterruptsNone.cpp', u'Source/JavaScriptCore/runtime/Terminator.h', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/CurrentTime.cpp', u'Source/WTF/wtf/CurrentTime.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/JSEventListener.cpp', u'Source/WebCore/bindings/js/WorkerScriptController.cpp']" exit_code: 1
Source/JavaScriptCore/runtime/JSInterruptsMac.cpp:41:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 34 files


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

[Mark Lam]

Comment on attachment 198011 [details]
patch 3: bug fix in throwTerminatedExecutionException(), and added missing entry in vcxproj.filter file.

Ready for a review.

[Geoffrey Garen]

Since enabling this feature turns off the DFG JIT, what does "benchmark result: LLINT + baseline JIT + DFG" mean?

[Mark Lam]

(In reply to comment #23)
> Since enabling this feature turns off the DFG JIT, what does "benchmark result: LLINT + baseline JIT + DFG" mean?

Sorry.  I should have been more clear.  All benchmark results are with the feature built in but not in use.  That means it will incur the polling cost for the LLINT and baseline JIT.  The benchmark numbers are for:

1. Running with LLINT alone (baseline JIT and DFG disabled via Options flags).
2. Running with LLINT and baseline JIT alone (DFG disabled via Options flags).
3. Running with LLINT, baseline JIT, and DFG allowed like normal.

In all 3 cases, the baseline build used for comparison is similar restricted to the 3 different configurations.  Basically, I’m measuring the 3 cases of interpreter performance, baseline JIT performance, and DFG performance when the feature is built in but not in use.

Usage of the feature should matter much because it normally still takes the same code paths except when the script times outs.

[Geoffrey Garen]

Let's not generate termination checking code in the baseline JIT unless the feature has been enabled.

[Mark Lam]

Created attachment 198428 [details]
benchmark result: LLINT + baseline JIT (DFG disabled) + timeout off, run 1

[Mark Lam]

Created attachment 198429 [details]
benchmark result: LLINT + baseline JIT (DFG disabled) + timeout off, run 2

[Mark Lam]

Created attachment 198430 [details]
benchmark result: LLINT + baseline JIT + DFG + timeout off, run 1

[Mark Lam]

Created attachment 198431 [details]
benchmark result: LLINT + baseline JIT + DFG + timeout off, run 2

[Mark Lam]

Created attachment 198432 [details]
patch 4: no poll checks emitted in baseline JIT when timeout is not enabled.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSContextRef.cpp', u'Source/JavaScriptCore/API/JSContextRef.h', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcproj', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JIT.h', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/jit/JITStubs.h', u'Source/JavaScriptCore/llint/LLIntExceptions.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.h', u'Source/JavaScriptCore/llint/LowLevelInterpreter.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter64.asm', u'Source/JavaScriptCore/runtime/ExceptionHelpers.cpp', u'Source/JavaScriptCore/runtime/ExceptionHelpers.h', u'Source/JavaScriptCore/runtime/JSGlobalData.h', u'Source/JavaScriptCore/runtime/JSInterrupts.cpp', u'Source/JavaScriptCore/runtime/JSInterrupts.h', u'Source/JavaScriptCore/runtime/JSInterruptsMac.cpp', u'Source/JavaScriptCore/runtime/JSInterruptsNone.cpp', u'Source/JavaScriptCore/runtime/Terminator.h', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/CurrentTime.cpp', u'Source/WTF/wtf/CurrentTime.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/JSEventListener.cpp', u'Source/WebCore/bindings/js/WorkerScriptController.cpp']" exit_code: 1
Source/JavaScriptCore/runtime/JSInterruptsMac.cpp:41:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 34 files


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

[Geoffrey Garen]

Comment on attachment 198432 [details]
patch 4: no poll checks emitted in baseline JIT when timeout is not enabled.

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

> Source/JavaScriptCore/ChangeLog:11
> +

Please add testapi code to test this API.

> Source/JavaScriptCore/API/JSContextRef.h:72
> +@typedef JSScriptTimeoutCallback

Let's call this JSShouldTerminateCallback.

"Timeout" is not so good because our other function calls it a "time limit".

"Script" is not so good because it doesn't specify what question we're asking.

> Source/JavaScriptCore/API/JSContextRef.h:76
> +@param callbackData User specified data previously passed to

Let's call this "context". That's what other callback APIs call it.

> Source/JavaScriptCore/API/JSContextRef.h:82
> + If you return true, the timed out script will be terminated.

Let's say "will terminate" to avoid the passive voice.

> Source/JavaScriptCore/API/JSContextRef.h:83
> + If you return false, the script will allowed to run for another period of the

Let's say "will run" to avoid the passive voice.

> Source/JavaScriptCore/API/JSContextRef.h:84
> + allowed time limit previously specified via JSContextGroupSetExecutionTimeLimit.

Let's not say "previously" because it implies that you're not allowed to change the time limit inside this callback.

> Source/JavaScriptCore/API/JSContextRef.h:89
> + set a new time limit. The new time limit will take effect as soon as it
> + makes sense to do so. If you set the time limit to JSSCRIPT_NEVER_TIMEOUT,
> + then the script timeout will be cancelled.

Let's not comment on when JSContextGroupSetExecutionTimeout takes effect. Like all functions, it takes effect immediately. Saying "as soon as it makes sense to do so" is vague, and implies some kind of delay.

No need to mention the JSSCRIPT_NEVER_TIMEOUT option here -- that's described below.

> Source/JavaScriptCore/API/JSContextRef.h:98
> +#ifdef __cplusplus
> +#define JSSCRIPT_NEVER_TIMEOUT std::numeric_limits<double>::infinity()
> +#else
> +#define JSSCRIPT_NEVER_TIMEOUT INFINITY
> +#endif

Now that I think about it, using infinity as in-band signaling for "cancel", and passing a NULL callback that will never be called, seems obtuse. And this preprocessor API makes me sad. Let's just add a JSContextGroupClearExecutionTimeLimit() instead, and remove this infinity stuff.

> Source/JavaScriptCore/API/JSContextRef.h:114
> +JS_EXPORT void JSContextGroupSetExecutionTimeLimit(JSContextGroupRef, double limit, JSScriptTimeoutCallback, void* callbackData) AVAILABLE_IN_WEBKIT_VERSION_4_0;

Please put this stuff in JSContextRefPrivate.h. Public API changes require formal review.

You should comment that this function needs to be called before code starts running in order to be guaranteed to take effect.

> Source/JavaScriptCore/runtime/JSInterrupts.cpp:91
> +JSInterrupts::Action JSInterrupts::checkForRequiredAction(ExecState* exec)

This should have an early return if m_didFire is already true.

> Source/JavaScriptCore/runtime/JSInterrupts.cpp:138
> +void JSInterrupts::requestInterrupt()

Let's call this timerDidFire(). "Request" is a misleading word here -- the timer just fires, it doesn't request anything from us.

> Source/JavaScriptCore/runtime/JSInterrupts.cpp:190
> +    m_hasTerminationRequest = true;
> +    requestInterrupt();

This can just jump ahead and set m_didFire to true.

> Source/JavaScriptCore/runtime/JSInterrupts.cpp:208
> +void JSInterrupts::fireWatchdog()

This function can go away.

> Source/JavaScriptCore/runtime/JSInterrupts.h:38
> +class JSInterrupts {

Let's call this class "Watchdog".

"Interrupts" is vague grammar, and a bit awkward to say. The plural implies some sort of collection, which isn't the case here.

Because we're in the JSC namespace, we don't need the "JS" prefix. We use the "JS" prefix to distinguish between classes with similar names, like Node vs JSNode. We don't have that problem here.

> Source/JavaScriptCore/runtime/JSInterrupts.h:43
> +    enum Action {
> +        None,
> +        Terminate,
> +    };

I don't think an enumeration is appropriate here, since there's only one condition. See below.

> Source/JavaScriptCore/runtime/JSInterrupts.h:45
> +    class Timer;

Let's call this class "Scope" to avoid confusion with our internal timer mechanism.

> Source/JavaScriptCore/runtime/JSInterrupts.h:50
> +    typedef bool (*TimeoutCallback)(ExecState*, void* data1, void* data2);

Let's call this ShouldTerminateCallback to match the name in API.

> Source/JavaScriptCore/runtime/JSInterrupts.h:51
> +    void setScriptTimeout(JSGlobalData&, double seconds, TimeoutCallback = 0, void* data1 = 0, void* data2 = 0);

Let's call this setTimeLimit to match the name in API.

> Source/JavaScriptCore/runtime/JSInterrupts.h:59
> +    Action checkForRequiredAction(ExecState*);

Let's call this didFire. It can return a bool, now that it's clear what we're asking.

> Source/JavaScriptCore/runtime/JSInterrupts.h:61
> +    bool hasScriptTimeoutEnabled();

Let's call this isEnabled.

> Source/JavaScriptCore/runtime/JSInterrupts.h:64
> +    void pauseWatchdog();
> +    void resumeWatchdog();

Nobody calls these. Can we remove them?

We can remove "Watchdog" from the names here, since this class is named Watchdog.

> Source/JavaScriptCore/runtime/JSInterrupts.h:66
> +    JS_EXPORT_PRIVATE bool isTerminating();

Let's call this didFire too, and add a comment explaining that this version is more efficient, but can only tell you if the watchdog fired in the past, and not whether it should fire right now. Since the point of this function is to be fast, it should be inlined.

> Source/JavaScriptCore/runtime/JSInterrupts.h:67
> +    JS_EXPORT_PRIVATE void requestTermination();

Let's call this fire.

> Source/JavaScriptCore/runtime/JSInterrupts.h:69
> +    bool hasRequests;

This shouldn't be public. See JSCell::structureOffset() for an example of how we expose data members to the JIT.

Let's call this m_timerDidFire. "hasRequests" is vague about what the request was.

> Source/JavaScriptCore/runtime/JSInterrupts.h:75
> +    void requestInterrupt();

Let's call this maybeFire.

> Source/JavaScriptCore/runtime/JSInterrupts.h:90
> +    bool m_hasTerminationRequest;
> +    bool m_isTerminating;

There's so much state here -- m_hasTerminationRequest, m_isTerminating, hasRequests -- just for knowing whether the watchdog fired. Let's get down to two pieces of state: m_timerDidFire and m_didFire. m_timerDidFire indicates that the timer fired, and we need to check CPU time as soon as possible, and m_didFire means we checked CPU time, and decided to terminate execution. An explicit termination in C++ code can just jump to the end and set m_didFire to true.

[Geoffrey Garen]

> Let's call this maybeFire.

Sorry, this was a mis-edit. My comment, above, was "let's call this timerDidFire".

[Mark Lam]

Created attachment 198499 [details]
patch 5: addressed all of Geoff's feedback, fixed a few bugs, and did some additional clean up.

Since there was a lot of changes, let's test this out on the EWS bots first.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSContextRef.cpp', u'Source/JavaScriptCore/API/JSContextRefPrivate.h', u'Source/JavaScriptCore/API/tests/testapi.c', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcproj', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JIT.h', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/jit/JITStubs.h', u'Source/JavaScriptCore/llint/LLIntExceptions.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.h', u'Source/JavaScriptCore/llint/LowLevelInterpreter.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter64.asm', u'Source/JavaScriptCore/runtime/ExceptionHelpers.cpp', u'Source/JavaScriptCore/runtime/ExceptionHelpers.h', u'Source/JavaScriptCore/runtime/JSGlobalData.cpp', u'Source/JavaScriptCore/runtime/JSGlobalData.h', u'Source/JavaScriptCore/runtime/Terminator.h', u'Source/JavaScriptCore/runtime/Watchdog.cpp', u'Source/JavaScriptCore/runtime/Watchdog.h', u'Source/JavaScriptCore/runtime/WatchdogMac.cpp', u'Source/JavaScriptCore/runtime/WatchdogNone.cpp', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/CurrentTime.cpp', u'Source/WTF/wtf/CurrentTime.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/JSEventListener.cpp', u'Source/WebCore/bindings/js/SerializedScriptValue.cpp', u'Source/WebCore/bindings/js/WorkerScriptController.cpp']" exit_code: 1
Source/JavaScriptCore/runtime/WatchdogMac.cpp:41:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 35 files


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

[Geoffrey Garen]

Comment on attachment 198499 [details]
patch 5: addressed all of Geoff's feedback, fixed a few bugs, and did some additional clean up.

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

Much improved, but I believe I've spotted a data race.

> Source/JavaScriptCore/API/JSContextRefPrivate.h:91
> +@param callbackData User data that you can provide to be passed back to you
> + in your callback.

This is "context" now.

> Source/JavaScriptCore/runtime/Watchdog.cpp:35
> +#define NEVER_TIMEOUT std::numeric_limits<double>::infinity()

WebKit style prefers something like "static const double noLimit = ..." vs #define NO_LIMIT.

> Source/JavaScriptCore/runtime/Watchdog.h:82
> +    bool m_timerDidFire;

This data member should include a comment that it is set by a secondary thread. You have a comment above next to a function, but it's clearer to think about thread-safety in terms of the data being accessed.

> Source/JavaScriptCore/runtime/WatchdogMac.cpp:47
> +        dispatch_source_cancel(m_timer);

Why do we need to cancel inside the fire handler? Once the timer fires, we wouldn't expect it to fire again.

> Source/JavaScriptCore/runtime/WatchdogMac.cpp:48
> +        timerDidFire();

I think this would be clearer if it just assigned directly to m_timerDidFire instead of calling an abstract function. There's no need for abstraction here, since we're inside the class's internal implementation functions, and this is the only line of code that will assign to the data member.

> Source/JavaScriptCore/runtime/WatchdogMac.cpp:56
> +    dispatch_source_cancel(m_timer);

dispatch_source_cancel is asynchronous, so this line of code does not synchronize with any code that might fire m_timer. Therefore, it's possible to execute this code, then return and delete the Watchdog, then observe the timer firing, writing to the deleted Watchdog's data member.

[Geoffrey Garen]

Comment on attachment 198499 [details]
patch 5: addressed all of Geoff's feedback, fixed a few bugs, and did some additional clean up.

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

> Source/JavaScriptCore/runtime/WatchdogMac.cpp:32
> +{

Let's ASSERT(!m_timer) here to catch leaks caused by repeat starts.

> Source/JavaScriptCore/runtime/WatchdogMac.cpp:57
> +    dispatch_release(m_timer);

Let's set m_timer to 0 here to help our ASSERT and to avoid a dangling pointer.

[Mark Lam]

Created attachment 198617 [details]
patch 6: addressed Geoff's feedback again.

[Geoffrey Garen]

Comment on attachment 198617 [details]
patch 6: addressed Geoff's feedback again.

r=me

[Mark Lam]

Thanks for the review.  Added 1 small change to defer allocating Watchdog::m_queue until its first use in startTimer().  Landed in r148639: <http://trac.webkit.org/changeset/148639>.

[Mark Lam]

ref: <rdar://problem/4115952>.

[Mark Lam]

Bots were bleeding.  Fixed in r148641: <http://trac.webkit.org/changeset/148641>.