472702 - TM: using watchdog thread in js shell to trigger operation callback

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

[Igor Bukanov]

JS_THREADSAFE builds of js shell should use a separated thread to trigger periodically operation callbacks instead of relying on the operation counting. This would allow to test the infrastructure necessary to implement the watchdog thread in the browser (see bug 453157).

Comment 1

[Igor Bukanov]

Attachment: work in progess

Comment 2

[Igor Bukanov]

Attachment: v1

The patch adds API to set the operation callback without changing the operation limit and to trigger the callback call later. The jstracer.cpp changes ensures that if the application never sets the operation limit, then only code to check for the operationCount, not to update it, is generated.

This should restore the benchmark performance loses due to the bug 465030 for JS_THREADSAFE build of JS shell as with the watchdog thread the shell does not set any operation counter limits.

The watchdog code for js.cpp is based on the patch from the bug 453157 but it is updated to use the watchdog both to terminate the long-running scripts and to schedule JS_MaybeGC() and JS_SuspendRequest() calls. The patch also removes the extra time-related typedefs that I added in the bug 419086. I was wrong that it would be possible to use the same code for the operation callback both with JS_THREADSAFE and !JS_THREADSAFE cases. The logic is way to different with these 2 cases and explicit ifdefs leads to simpler code.

Another difference from the bug 453157 is that to terminate the watchdog thread the code uses explicit wait/notify instead of PR_JoinThread(). I suspect that the use of the latter in the original watchdog patch is not entirely correct. If this hypothesis is true, then it would explain the hangs with the unit tests. Overall with the patch the watchdog code in the js shell should be closer to the code that would be used for the browser allowing better test coverage.

Comment 3

[Igor Bukanov]

This bug blocks the bug 453157, which is 1.9.1 blocker. Nominating.

Comment 4

[Igor Bukanov]

Attachment: v2

The new version takes the lock only once in the body of the WatchdogMain instead of taking it on each iteration of the main loop there.

Comment 5

[Igor Bukanov]

Attachment: v3

The new version passes PR_UNJOINABLE_THREAD, not 0 (which will be converted to PR_JOINABLE_THREAD), to PR_CreateThread and fixes some comments.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 356514 [details] [diff] [review]
v3

Reviewing today, sorry for the delay.

/be

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 356514 [details] [diff] [review]
v3

>+ * expensive and is not suitable for calls when a GC lock is held. This forces
>+ * 

"us" here, before "to use":

>+   to use PRIntervalTime as a time type and deal with potential time-wraps
>+ * over uint32 limit. In particular, we must use time relative to some recent
>+ * timestamp when checking for expiration, not absolute time values, as in the
>+ * !JS_THREADSAFE case, when time is int64 and no time-wraps are feasible.

s/when/where/

>+struct JSShellContextData {
>+#ifdef JS_THREADSAFE
>+    PRIntervalTime  timeout;
>+    volatile PRIntervalTime startTime;      /* startTime + timeout is time when
>+                                               script must be stopped */
>+    PRIntervalTime  maybeGCPeriod;
>+    volatile PRIntervalTime lastMaybeGCTime;/* lastMaybeGCTime + maybeGCPeriod
>+                                               is the time to call MaybeGC */
>+    PRIntervalTime  yieldPeriod;
>+    volatile PRIntervalTime lastYieldTime;  /* lastYieldTime + yieldPeriod is
>+                                               the time to call
>+                                               JS_YieldRequest() */
>+#else
>+    int64           stopTime;               /* time when script must be
>+                                               stopped */
>+    int64           nextMaybeGCTime;        /* time to call JS_MaybeGC */
>+#endif

Nit: prettier to start all member declarators in teh same column?

>+/*
>+ * The function assumes that the GC lock is taken on entry.

s/taken/already held/

as "taken" is ambiguous here (does the function take the lock? want past perfect tense).

r=me with nits.

/be

Comment 8

[Igor Bukanov]

Attachment: v3b

The new version of the patch addresses the nits from the previous comment.

Comment 9

[Igor Bukanov]

landed to tm - http://hg.mozilla.org/tracemonkey/rev/255f6cd5ae6f

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/255f6cd5ae6f

Comment 11

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/07dd42747d83

Comment 12

[Gordon P. Hemsley [:GPHemsley]]

The variable isRunning, of type PRBool, in the function WatchdogMain() became superfluous as of v2 of the patch (it was used in v1). However, its original definition was carried through to v3b and checked in. As it is unnecessary, the compiler marks it as an unused variable.

Example location:
http://hg.mozilla.org/mozilla-central/file/6dbc129ccd19/js/src/shell/js.cpp#l3170

Comment 13

[Gordon P. Hemsley [:GPHemsley]]

Attachment: Remove unused variable declaration

I've created this patch to clarify what I'm referring to.

Comment 14

[Andreas Gal :gal]

Comment on attachment 359705 [details] [diff] [review]
Remove unused variable declaration

Please push to the TraceMonkey tree if you have access to hg.

Comment 15

[Jesse Ruderman]

Warning fixed pushed:
http://hg.mozilla.org/tracemonkey/rev/271e5fa192fd