[webkit-reviews] review denied: [Bug 114577] JSC: Add LLINT and baseline JIT support for timing out scripts : [Attachment 198432] patch 4: no poll checks emitted in baseline JIT when timeout is not enabled.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 114577: JSC: Add LLINT and baseline JIT support for timing out scripts
https://bugs.webkit.org/show_bug.cgi?id=114577

Attachment 198432: patch 4: no poll checks emitted in baseline JIT when timeout
is not enabled.
https://bugs.webkit.org/attachment.cgi?id=198432&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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
> + at 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
> + at 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.