Closed Bug 472702 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: JavaScript Engine, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files, 4 obsolete files)

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).
Blocks: 453157
No longer depends on: 453157
Attached patch work in progess (obsolete) — Splinter Review
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #356026 - Attachment is obsolete: true
Attachment #356092 - Flags: review?(brendan)
This bug blocks the bug 453157, which is 1.9.1 blocker. Nominating.
Flags: blocking1.9.1?
Attached patch v2 (obsolete) — Splinter Review
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.
Attachment #356092 - Attachment is obsolete: true
Attachment #356215 - Flags: review?(brendan)
Attachment #356092 - Flags: review?(brendan)
Attached patch v3 (obsolete) — Splinter Review
The new version passes PR_UNJOINABLE_THREAD, not 0 (which will be converted to PR_JOINABLE_THREAD), to PR_CreateThread and fixes some comments.
Attachment #356514 - Flags: review?(brendan)
Attachment #356215 - Attachment is obsolete: true
Attachment #356215 - Flags: review?(brendan)
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P1
Comment on attachment 356514 [details] [diff] [review]
v3

Reviewing today, sorry for the delay.

/be
Attachment #356514 - Flags: review?(brendan) → review+
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
Attached patch v3bSplinter Review
The new version of the patch addresses the nits from the previous comment.
Attachment #356514 - Attachment is obsolete: true
Attachment #356956 - Flags: review+
landed to tm - http://hg.mozilla.org/tracemonkey/rev/255f6cd5ae6f
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/255f6cd5ae6f
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 473721
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
I've created this patch to clarify what I'm referring to.
Attachment #359705 - Attachment description: Remove unused variable declaration. → Remove unused variable declaration
Attachment #359705 - Flags: review?(igor)
Comment on attachment 359705 [details] [diff] [review]
Remove unused variable declaration

Please push to the TraceMonkey tree if you have access to hg.
Attachment #359705 - Flags: review?(igor) → review+
You need to log in before you can comment on or make changes to this bug.