Closed Bug 871079 Opened 8 years ago Closed 8 years ago

Don't trigger operation callback from watchdog unless JS has been running long enough

Categories

(Core :: XPConnect, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
Right now the watchdog triggers the operation callback once per second, unless JS has been inactive for a couple seconds (in which case it hibernates).  This firing is only used for showing a slow script dialog, however, so in almost all cases triggering the callback is pointless.  Bug 864220 makes triggering the callback from off thread more expensive, so this would be good to avoid when possible.  The attached patch extends the mechanism used for hibernating the watchdog thread so that the callback isn't triggered unless the active request has been running for more than a second, which should never happen in interactive apps / games etc. where the callback triggering cost is problematic.
Attachment #748342 - Flags: review?(bobbyholley+bmo)
There's been some recent discussion of the JS Watchdog thread over in bug 870043, in the context of mobile battery use when JS is waking up for short periods.  Maybe this will help with that, too?
(In reply to Andrew McCreight [:mccr8] from comment #1)
> There's been some recent discussion of the JS Watchdog thread over in bug
> 870043, in the context of mobile battery use when JS is waking up for short
> periods.  Maybe this will help with that, too?

By itself this won't help bug 870043, as this doesn't change when the watchdog runs, just when it triggers the callback.  It seems like 870043 would be helped if the watchdog was always hibernating (aka waiting on a condvar with no timeout) when JS is not running --- there's this odd behavior right now where it will keep waking itself up once a second for a couple seconds after JS stops running, which is I think the problem in 870043 and is presumably to avoid excessive stopping/starting of the thread when running lots of short lived scripts.  I wonder if the latter is really a problem though, either for desktop or b2g.
Comment on attachment 748342 [details] [diff] [review]
patch

Review of attachment 748342 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1100,5 @@
>          }
>          MOZ_ALWAYS_TRUE(PR_WaitCondVar(self->mWatchdogWakeup, sleepInterval) == PR_SUCCESS);
> +
> +        // Don't trigger the operation callback if activity started less than one second ago.
> +        // The callback is only used for detecting long running scripts.

Mention that triggering the callback from OMT is expensive?

::: js/xpconnect/src/xpcprivate.h
@@ +940,5 @@
>      PRThread *mWatchdogThread;
>      nsTArray<JSGCCallback> extraGCCallbacks;
>      bool mWatchdogHibernating;
>      PRTime mLastActiveTime; // -1 if active NOW
> +    PRTime mStartActiveTime; // -1 if inactive

These little bits of magic are extremely confusing, given that they're mutually exclusive and -1 means opposite things. So let's have something like:

* mRuntimeState, which can be either RUNTIME_ACTIVE or RUNTIME_INACTIVE
* mLastStateChange, which records the timestamp of the most recent state change.
* a helper IsRuntimeActive(), which returns (mRuntimeState == RUNTIME_ACTIVE).
* a helper TimeSinceLastStateChange(), which does the PR_NOW() subtraction dance.

That way, the existing code can do:

if (IsRuntimeActive || TimeSinceLastStateChange() <= PRTime(2*PR_USEC_PER_SEC))

and your code can do:

if (IsRuntimeActive() && TimeSinceLastStateChange() >= PRTime(PR_USEC_PER_SEC))
Attachment #748342 - Flags: review?(bobbyholley+bmo) → review-
Attached patch revisedSplinter Review
Revised per comments.
Assignee: nobody → bhackett1024
Attachment #748342 - Attachment is obsolete: true
Attachment #751097 - Flags: review?(bobbyholley+bmo)
Comment on attachment 751097 [details] [diff] [review]
revised

Review of attachment 751097 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great - thanks.
Attachment #751097 - Flags: review?(bobbyholley+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/bfb827bd48d0
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.