568730 - Allow customizing the sleep duration in XPCJSRuntime::WatchdogMain

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alon Zakai (:azakai)]

XPCJSRuntime::WatchdogMain (in xpjsruntime.cpp) continually runs a loop of calling JS_TriggerOperationCallback for contexts and then sleeping for 1 second. The effect is one CPU wakeup/second, which is negligible on the desktop, but every wakeup on mobile devices will drain the battery faster (wakeups disturb the CPU from its power-saving sleep mode for a short while). So it would be useful to make this customizable - as a config setting perhaps? - so that mobile devices could have a longer sleep period for fewer wakeups.

(As a comparison, we have ~2 wakeups/second on average currently, so WatchdogMain accounts for about half of them.)

Would there be a problem if the things JS_TriggerOperationCallback is used for (periodic GC, detecting non-halting scripts, other stuff?) were called less frequently than the current 1/second?

---

Background stuff about power: http://www.lesswatts.org/documentation/silicon-power-mgmnt/ (not really important here, just if anyone's curious)

Comment 1

[Brendan Eich [:brendan]]

Can we disarm the 1 second timer until some JS runs again, and provided queues are empty and nothing seems bound to run in this event loop turn? Presumably nothing else is going on (or will be, once we fix all the other bogus timers). This way we run every second (or so) while there's activity.

/be

Comment 2

[David Mandelin [:dmandelin]]

(Comment composed while Brendan was also typing.) I don't think decreasing the frequency of wakeups would be a problem. But I think what you really want is to only activate the JS callback when JS is actually running, instead of all the time. A couple of possibilities:

- Turn on/off the timer when JS is entered/exited.

- Change the implementation to use a countdown, like it did before. That might cost a percent or two of perf in TM, though. I think we could make it pretty fast on JM x64, though.

Comment 3

[Igor Bukanov]

We could add a callback that would run when JS engine enters/leaves a request. That can start/suspend watchdog as necessary.

Comment 4

[Alon Zakai (:azakai)]

Definitely, turning off the timer when possible (so there are no wakeups at all when idle) would be optimal.

I suspect though that customizing the sleep duration might still be useful as well in some cases. For example if a page has a JavaScript timeout that does something very minor 1/second, then I think we wouldn't be idle but it would still be good for the watchdog to run less frequently than 1/second (at least on mobiles).

Comment 5

[Mike Shaver (:shaver emeritus)]

What do you mean by "very minor 1/second"?  The watchdog wouldn't get woken if something wasn't running, since AIUI the proposal we would disable the watchdog when we stopped running script, and re-enable it when we started.

Comment 6

[Alon Zakai (:azakai)]

(In reply to comment #5)
> What do you mean by "very minor 1/second"?  The watchdog wouldn't get woken if
> something wasn't running, since AIUI the proposal we would disable the watchdog
> when we stopped running script, and re-enable it when we started.

Well, I meant something like a page that shows an updating clock with seconds, so it wakes up once per second for that, but the wakeup is extremely brief. If that is enough to keep the watchdog running (not sure I understood the above proposal entirely), then we would have a total of 2 wakeups/second in this example.

Comment 7

[Brendan Eich [:brendan]]

When no JS is running, there's no watchdog. The JS request model allows us to tell when any JS is running.

/be

Comment 8

[Alon Zakai (:azakai)]

Ok, I misunderstood before.

Sounds great.

Comment 9

[Alon Zakai (:azakai)]

Attachment: Patch

The patch makes the Watchdog hibernate (sleep until awoken) if there is no call context active over the last 2 seconds. I did this with call contexts because it seemed easier/cleaner than changing something more directly related to requests. (But, I am really not that familiar with the JS engine code, so please correct me if there is a better way to do this.)

Comment 10

[Andreas Gal :gal]

Comment on attachment 451403 [details] [diff] [review]
Patch

Deferring to bent. This more more a XPCOM thing.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 451403 [details] [diff] [review]
Patch

I'd much rather see this done based on a callback from the JS engine when we first enter a request and when we enter the last request. I fear that doing this in the XPCCallContext ctor/dtor will be too costly since we hit those a *lot*.

Comment 12

[Alon Zakai (:azakai)]

Attachment: JS_StartRequest / JSEndRequest version

Here is a version that adds callbacks, sent from JS_BeginRequest and JS_EndRequest.

There seems to be no way to avoid adding a lock for purposes of counting requests, but perhaps there is a simpler way somehow? In the current patch it is consequently also required that the callbacks not begin or end requests themselves (which is not a problem for the Watchdog).

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 454580 [details] [diff] [review]
JS_StartRequest / JSEndRequest version

There is absolutely no reason for another lock. Also, #ifdef JS_THREADSAFE around all locking/MT code.

Use JS_ATOMIC_INCREMENT and JS_ATOMIC_DECREMENT, and do not (using any lock, new or existing) lock across the callback.

/be

Comment 14

[Igor Bukanov]

> Created an attachment (id=454580) [details]
> 
> There seems to be no way to avoid adding a lock for purposes of counting
> requests, but perhaps there is a simpler way somehow?

What is wrong with using JSRuntime::requestCount?

Comment 15

[Alon Zakai (:azakai)]

(In reply to comment #13)
> (From update of attachment 454580 [details] [diff] [review])
> There is absolutely no reason for another lock. Also, #ifdef JS_THREADSAFE
> around all locking/MT code.
> 
> Use JS_ATOMIC_INCREMENT and JS_ATOMIC_DECREMENT, and do not (using any lock,
> new or existing) lock across the callback.
> 

Ok, I understand now how atomic increments and decrements are enough for this. I'll do it that way.

(In reply to comment #14)
> > Created an attachment (id=454580) [details] [details]
> > 
> > There seems to be no way to avoid adding a lock for purposes of counting
> > requests, but perhaps there is a simpler way somehow?
> 
> What is wrong with using JSRuntime::requestCount?

Well, JSRuntime::requestCount is only used if JS_THREADSAFE is set. But in theory we might want idle/active callbacks even from non-JS_THREADSAFE builds, I think? If not, then this could be done much simpler, with JSRuntime::requestCount, I'd be happy to do it that way.

Otherwise, come to think of it, if on the other hand we do want these new callbacks in non-JS_THREADSAFE builds, then we might as well just move the JSRuntime::requestCount code out of the JS_THREADSAFE checks (but keep the JS_THREADSAFE on the locks etc.). So no need for a new variable in that case.

So the question is at this point, do we want this to work in non-JS_THREADSAFE builds, or not?

Comment 16

[Brendan Eich [:brendan]]

There are no requests at all in non-JS_THREADSAFE builds!

Please just follow the grain of the code.

/be

Comment 17

[Alon Zakai (:azakai)]

Attachment: JS_StartRequest / JSEndRequest version v2

Fixed patch following the comments.

Sorry for my confusion and lack of clarity before, I thought it might make sense to have these callbacks even in non-multithreaded builds (since they are allowed to run JS_BeginRequest/JS_EndRequest, even though there are no actual requests occurring).

Regarding names, I wasn't sure what to call the callbacks. Maybe it should be RuntimeIdleCallback/RuntimeActiveCallback?

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 454954 [details] [diff] [review]
JS_StartRequest / JSEndRequest version v2

- In XPCJSRuntime::NoteActive():

+    if (mWatchdogHibernating)
+    {
+        AutoLockJSGC lock(mJSRuntime);
+        mWatchdogHibernating = PR_FALSE;
+        PR_NotifyCondVar(mWatchdogWakeup);
+    }

This can race with the watchdog thread setting mWatchdogHibernating, we should do the check while holding the mJSRuntime lock.

- In XPCJSRuntime::ContextIdleCallback():

+    return true;
+}

...

- and XPCJSRuntime::ContextActiveCallback()

+    return false;

Why return true in one of these and false in the other?

r=jst for the xpconnect parts here, someone else should review the JS engine changes (Andreas?).

Comment 19

[Andreas Gal :gal]

Comment on attachment 454954 [details] [diff] [review]
JS_StartRequest / JSEndRequest version v2

> #ifdef JS_THREADSAFE
>+    JSRuntime *rt;
>+    JSBool needCallback = JS_FALSE;
>+

This ain't 1967 any more. false! ;)

>     JS_ASSERT(CURRENT_THREAD_IS_ME(cx->thread));
>     if (!cx->requestDepth) {
>-        JSRuntime *rt = cx->runtime;
>+        rt = cx->runtime;
>         AutoLockGC lock(rt);
> 
>         /* Wait until the GC is finished. */
>@@ -775,6 +778,13 @@
>         cx->thread->contextsInRequests++;
>         cx->requestDepth = 1;
>         cx->outstandingRequests++;
>+        needCallback = rt->requestCount == 1 && rt->cxActiveCallback;
>+        if (!needCallback)
>+            return;
>+    }
>+    if (needCallback) {
>+        /* Do a callback in this separate block, without locking */
>+        rt->cxActiveCallback(rt->cxActiveCallbackArg);
>         return;
>     }

We have a JSAutoUnlockGC helper. Use that instead of the needCallback hack here.

>     cx->requestDepth++;
>@@ -787,6 +797,7 @@
> {
> #ifdef JS_THREADSAFE
>     JSRuntime *rt;
>+    JSBool needCallback = JS_FALSE;
> 
>     CHECK_REQUEST(cx);
>     JS_ASSERT(CURRENT_THREAD_IS_ME(cx->thread));
>@@ -811,6 +822,13 @@
>         cx->thread->contextsInRequests--;
>         if (rt->requestCount == 0)
>             JS_NOTIFY_REQUEST_DONE(rt);
>+        needCallback = rt->requestCount == 0 && rt->cxIdleCallback;
>+        if (!needCallback)
>+            return;
>+    }
>+    if (needCallback) {
>+        /* Do a callback in this separate block, without locking */
>+        rt->cxIdleCallback(rt->cxIdleCallbackArg);

Why cx? These callbacks are for the engine really, so how about rt->idleCallback and rt->activeCallback. Also, you might want to just make it one callback with an argument? Since likely you are interested in both at the same time. rt->idleCallback and then idle(void *private, bool active) or something like that. Up to you.

Comment 20

[Igor Bukanov]

The patch conflicts with recent changes on TM branch. So rebase with them and let me have a look at the request changes before landing.

Comment 21

[Alon Zakai (:azakai)]

Attachment: Patch v3

jst and gal, thanks for the comments. I made the requested changes.

gal: I changed the names as you suggested (to rt->idleCallback and rt->activeCallback). I kept the 2 separate callbacks though, since I suspect some applications might care about only one of the two.

Patch is rebased against the latest TM.

Comment 22

[Igor Bukanov]

Comment on attachment 460685 [details] [diff] [review]
Patch v3

Please attach a patch generated via hg diff -U8 -p.

Comment 23

[Igor Bukanov]

Comment on attachment 460685 [details] [diff] [review]
Patch v3

>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp
>+++ b/js/src/jsapi.cpp
>@@ -802,6 +802,12 @@
>         cx->outstandingRequests++;
>         cx->thread->requestContext = cx;
>         rt->requestCount++;
>+
>+        // Do not lock for the callback
>+        AutoUnlockGC unlock(rt);
>+        if (rt->requestCount == 1 && rt->activeCallback) {
>+            rt->activeCallback(rt->activeCallbackArg);
>+        }

rt->requestCount must be accessed inside the lock. Also what prevents another thread from setting the callback to NULL after the current one checks it?

Another problem is that using AutoUnlockGC here is rather suboptimal as the destructor would first take the lock before the destructor for the outer AutoLockGC would relase the lock.


>     }
> #endif
> }
>@@ -852,6 +858,12 @@
>         rt->requestCount--;
>         if (rt->requestCount == 0)
>             JS_NOTIFY_REQUEST_DONE(rt);
>+
>+        // Do not lock for the callback
>+        AutoUnlockGC unlock(rt);
>+        if (rt->requestCount == 0 && rt->idleCallback) {
>+            rt->idleCallback(rt->idleCallbackArg);
>+        }

Same here.

Comment 24

[Igor Bukanov]

I think it is wrong to call the callback when the request count drops to 0 for the whole runtime. The main thread and web worker threads behaves differently. For example, we do not need to stop long-running web-workers nor we need to call a GC on a worker thread. This means that we can suspend the watchdog when the main thread is outside the request. 

In turn this implied that FF needs a callback which can be called when a thread leaves/enters the request. And ideally this callback should be-thread specific so we set it only for the main thread.

Comment 25

[Alon Zakai (:azakai)]

Attachment: Patch v4

Ok, I fixed the locking issues in comment #23, and made a new diff with the requested parameters.

(In reply to comment #24)
> I think it is wrong to call the callback when the request count drops to 0 for
> the whole runtime. The main thread and web worker threads behaves differently.
> For example, we do not need to stop long-running web-workers nor we need to
> call a GC on a worker thread. This means that we can suspend the watchdog when
> the main thread is outside the request. 
> 
> In turn this implied that FF needs a callback which can be called when a thread
> leaves/enters the request. And ideally this callback should be-thread specific
> so we set it only for the main thread.

In general I think this can indeed be a further optimization, but it would not be relevant in all cases. For example, these callbacks can be used for various things aside from GC, like whether the JS engine is using CPU at all - which would not differentiate between the main thread and worker threads.

In the original motivation for this patch, the goal was to reduce the # of wakeups when the CPU is idle, and when the CPU is not idle, the wakeups don't matter anyhow (for purposes of battery life). So that is also actually a case where the specific thread doesn't matter and the current patch is fine.

So, I would prefer to keep the callbacks as in this patch and leave further optimizations to a later bug as the need arises.

Comment 26

[Igor Bukanov]

(In reply to comment #25)
> For example, these callbacks can be used for various
> things aside from GC, like whether the JS engine is using CPU at all - which
> would not differentiate between the main thread and worker threads.

The callbacks can only be used for such things if the embeddings know for sure that other threads would not enter the request at the moment of the callback invocation. Such knowledge implies that the embedding controls fully the scheduling of the threads. But in this case it does not need the callback.

On the other hand if the code base does not allow such control then the proposed callback would not allow to alter scheduling of JS threads. That is why I see a need for a callback that allows to monitor start/stop events for threads. If necessary, it could be used to record how much or which threads are in requests and decide on how JS-idle is multi-core CPU.

> In the original motivation for this patch, the goal was to reduce the # of
> wakeups when the CPU is idle, and when the CPU is not idle, the wakeups don't
> matter anyhow (for purposes of battery life).

Each useless context switch hurts performance. So why from the beginning do we need to add API that would be suboptimal for its first user, the FF code base? I see no problems with changing the semantics of the callback to run it when any thread enter/leaves the request and adding a check to its implementation to ignore non-main thread events.

Comment 27

[Alon Zakai (:azakai)]

(In reply to comment #26)
> (In reply to comment #25)
> > For example, these callbacks can be used for various
> > things aside from GC, like whether the JS engine is using CPU at all - which
> > would not differentiate between the main thread and worker threads.
> 
> The callbacks can only be used for such things if the embeddings know for sure
> that other threads would not enter the request at the moment of the callback
> invocation. Such knowledge implies that the embedding controls fully the
> scheduling of the threads. But in this case it does not need the callback.
> 
> On the other hand if the code base does not allow such control then the
> proposed callback would not allow to alter scheduling of JS threads. That is
> why I see a need for a callback that allows to monitor start/stop events for
> threads. If necessary, it could be used to record how much or which threads are
> in requests and decide on how JS-idle is multi-core CPU.
> 
> > In the original motivation for this patch, the goal was to reduce the # of
> > wakeups when the CPU is idle, and when the CPU is not idle, the wakeups don't
> > matter anyhow (for purposes of battery life).
> 
> Each useless context switch hurts performance. So why from the beginning do we
> need to add API that would be suboptimal for its first user, the FF code base?
>
> I see no problems with changing the semantics of the callback to run it when
> any thread enter/leaves the request and adding a check to its implementation to
> ignore non-main thread events.

Well, if it is very easy to do then I am all for it. I am just not sure what you mean for the API to be in this case - can you please clarify or maybe just write it out explicitly?

Comment 28

[Igor Bukanov]

(In reply to comment #27)
> Well, if it is very easy to do then I am all for it. I am just not sure what
> you mean for the API to be in this case - can you please clarify or maybe just
> write it out explicitly?

The API is the same callback you proposed but that takes JSContext * cx argument and which is executed when thread enter and exit request. Also lets not proliferate the number of callbacks and use the argument to describe the event as suggested in the comment 19. An alternative is to have a proper C++ interface with a couple of virtual methods like in:

struct JSRequestNotifier /* or whatever name */ {
    virtual void requestEnter(JSContext *cx) = 0;
    virtual void requestExit(JSContext *cx) = 0;
};

If C embedding would need to use this callback, we can provide a wrapper that would create such callback like in:

JSRequestCallback *JS_CreateRequestNotifier(enterCallback, exitCallback, arg)

with the provision that the caller must use free() to release the result.

Comment 29

[Alon Zakai (:azakai)]

Attachment: Patch v5

Patch following discussion on IRC with Igor:

* Moved the API stuff from jsapi.h to jscntxt.h
* Moved to a single callback function with an argument, active true/false

Comment 30

[Igor Bukanov]

Comment on attachment 461320 [details] [diff] [review]
Patch v5

Please attach patches for review generated with -U8 -p options to have more context to read!

>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp
>+++ b/js/src/jsapi.cpp
>@@ -802,6 +802,11 @@
>         cx->outstandingRequests++;
>         cx->thread->requestContext = cx;
>         rt->requestCount++;
>+
>+        if (rt->requestCount == 1 && rt->activityCallback) {
>+            AutoUnlockGC unlock(rt);
>+            rt->activityCallback(rt->activityCallbackArg, true);
>+        }

This still has 2 problems: rt->activityCallback can change outside the lock and AutoUnlockGC leads to uselss lock/unlock in the destructor. One possibility is to write it like in:

    } else {
        JSActivityCallback cb = NULL;
        {
            JSRuntime *rt = cx->runtime;
            AutoLockGC lock(rt);

            /* Wait until the GC is finished. */
            if (rt->gcThread != cx->thread) {
                while (rt->gcThread)
                    JS_AWAIT_GC_DONE(rt);
            }

            /* Indicate that a request is running. */
            cx->requestDepth = 1;
            cx->outstandingRequests++;
            cx->thread->requestContext = cx;
            rt->requestCount++;
            if (rt->requestCount)
                cb = rt->activityCallback;
        }
        if (cb) cb(...);

    }

But even better would be to call the callback under the lock since it is going to take the same lock.

>+        if (rt->requestCount == 0 && rt->activityCallback) {
>+            AutoUnlockGC unlock(rt);
>+            rt->activityCallback(rt->activityCallbackArg, false);

The same here.

>diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h
>--- a/js/src/jscntxt.h
>+++ b/js/src/jscntxt.h
>@@ -1247,6 +1247,12 @@
> extern JS_FRIEND_API(void)
> js_TriggerAllOperationCallbacks(JSRuntime *rt, JSBool gcLocked);
> 
>+typedef void
>+(* JSActivityCallback)(void *arg, JSBool active);
>+
>+extern JS_FRIEND_API(JSActivityCallback)
>+JS_SetActivityCallback(JSRuntime *rt, JSActivityCallback cxCallback, void *arg);

No need to have a method here - just add an inline method to JSRuntime to set
activityCallback right after the field declaration like in
void setActivationCallback(...) { activationCallback = cb;  Arg = arg;}

Comment 31

[Alon Zakai (:azakai)]

Attachment: Patch v6

Sorry about the diff options last time.

Ok, I made the requested fixes in this patch.

Comment 32

[Igor Bukanov]

Comment on attachment 461349 [details] [diff] [review]
Patch v6

>@@ -797,25 +800,35 @@ JS_BeginRequest(JSContext *cx)
>+    if (activityCallback) {
>+        activityCallback(activityCallbackArg, true);
>     }

Style nit: remove {} around single-line, single statement if body.

>@@ -845,18 +858,24 @@ StopRequest(JSContext *cx)
>+    if (activityCallback) {
>+        activityCallback(activityCallbackArg, false);
>     }

Style nit: remove {} around single-line, single statement if body.

>+//static
>+void
>+XPCJSRuntime::ActivityCallback(void *arg, PRBool active)
>+{
>+    XPCJSRuntime* self = static_cast<XPCJSRuntime*>(arg);
>+    if (active) {
>+        self->mLastActiveTime = -1;

mLastActiveTime is 64 bit and non-atomic on 32 bit CPU. Thus it must be updated inside the lock. With that the lock will have to be taken for both active and !active cases. This is the same lock that is taken in request. So just move the callback invocation inside the lock in BeginRequest/StopRequest with comments about that.

Comment 33

[Alon Zakai (:azakai)]

Attachment: Patch v7

Fixed patch, after latest comments.

Comment 34

[Igor Bukanov]

Comment on attachment 461712 [details] [diff] [review]
Patch v7

>diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h
>@@ -1276,16 +1279,30 @@ struct JSRuntime {
>     /*
>+     * Sets a callback that is run whenever the runtime goes idle - the
>+     * last active request ceases - and begins activity - when it was
>+     * idle and a request begins. Note: The callback is called under the
>+     * runtime's lock.
>+     */

Nit: s/runtime's/GC/

Comment 35

[Alon Zakai (:azakai)]

Attachment: Patch v7.01

Fixed comment text as requested, carried over r+.

Comment 36

[Alon Zakai (:azakai)]

http://hg.mozilla.org/tracemonkey/rev/442559dabdf5

Comment 37

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/68a9a3355a63 - followup to add missing null check.

Comment 38

[Jon Hemming]

Is this fixed now?

Comment 39

[Alon Zakai (:azakai)]

Yes, marking as resolved.