Last Comment Bug 715953 - add telemetry for how many background timeouts are scheduled within 30seconds
: add telemetry for how many background timeouts are scheduled within 30seconds
Status: RESOLVED FIXED
[Snappy:P3]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Nathan Froyd [:froydnj]
:
Mentors:
Depends on:
Blocks: 715380 718212
  Show dependency treegraph
 
Reported: 2012-01-06 10:20 PST by Nathan Froyd [:froydnj]
Modified: 2012-02-16 19:40 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.29 KB, patch)
2012-01-13 08:11 PST, Nathan Froyd [:froydnj]
bzbarsky: review-
Details | Diff | Review
patch (3.47 KB, patch)
2012-01-13 11:44 PST, Nathan Froyd [:froydnj]
bzbarsky: review+
Details | Diff | Review
alternate patch with variable frequency (4.26 KB, patch)
2012-01-18 09:40 PST, Nathan Froyd [:froydnj]
bzbarsky: review+
Details | Diff | Review
patch (4.26 KB, patch)
2012-01-25 07:39 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Review
patch (4.30 KB, patch)
2012-01-26 11:00 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Review

Description Nathan Froyd [:froydnj] 2012-01-06 10:20:47 PST
Splitting off from bug 715380.

When were you expecting this computation to happen?  At each setTimeout, at timeout expiry, or some other time?
Comment 1 (dormant account) 2012-01-06 15:21:22 PST
Would be cool to do this in a mouse event handler, but bz might have a better suggestion.
Comment 2 Nathan Froyd [:froydnj] 2012-01-09 17:27:43 PST
CC'ing bz to see if he has a better suggestion on when to measure this.
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-09 19:11:08 PST
So for this one I'd like to understand what we're trying to measure.  Are we trying to measure how many timeouts got scheduled "recently"?  Or are we trying to measure how many are scheduled right now?  Do we want to sum over all background tabs (possibly all several hundred of them)?
Comment 4 (dormant account) 2012-01-10 11:40:13 PST
(In reply to Boris Zbarsky (:bz) from comment #3)
> So for this one I'd like to understand what we're trying to measure.  Are we
> trying to measure how many timeouts got scheduled "recently"?  Or are we
> trying to measure how many are scheduled right now?  Do we want to sum over
> all background tabs (possibly all several hundred of them)?

I'd like to measure how many timeouts are scheduled to run in the near future. But now that I think about it, I framed the problem wrong. It's easier to measure stuff like this from log since timeouts could reschedule self a good number of times within 30seconds.
I think what we really need is to measure how many setTimeouts fired in the last X interval, where x is a number like 30seconds.
Comment 5 Nathan Froyd [:froydnj] 2012-01-13 08:11:39 PST
Created attachment 588423 [details] [diff] [review]
patch

OK, so here's an attempt.  We start a separating, repeating timer that accumulates the number of timeouts set in its handler.  I didn't see any convenient place to hang this off Javascript, but that might be due to ignorance on my part.

This handler would be a good place to stick the telemetry for bug 715950, IMHO.

Testing with ~8 Alexa 100 sites open in tabs indicates that we're getting a little more than one timeout set every second.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-13 11:10:00 PST
Comment on attachment 588423 [details] [diff] [review]
patch

This leaks the timer through shutdown, no?
Comment 7 Nathan Froyd [:froydnj] 2012-01-13 11:44:45 PST
Created attachment 588483 [details] [diff] [review]
patch

Good point.  Now with NS_IF_RELEASE.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-13 13:04:00 PST
Comment on attachment 588483 [details] [diff] [review]
patch

r=me, but would it make sense to only turn on this timer when telemetry is enabled?
Comment 9 Nathan Froyd [:froydnj] 2012-01-14 07:34:38 PST
(In reply to Boris Zbarsky (:bz) from comment #8)
> r=me, but would it make sense to only turn on this timer when telemetry is
> enabled?

Probably.  That would require quite some fiddling, because I'd also like to have the timer turned on when telemetry turns on (e.g. coming out of private browsing).  I understand the desire to have telemetry have minimal impact (especially when people have telemetry disabled!), but I don't think it's worth doing here.  Maybe in a followup?

I'd be OK with not computing the stats for bug 715950 if telemetry is disabled; those are liable to be quite expensive.
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-14 08:45:35 PST
Followup is ok, but please do file a followup.  From what I understand, on mobile even once-every-30-seconds wakeups are not good.
Comment 11 (dormant account) 2012-01-17 13:40:37 PST
(In reply to Boris Zbarsky (:bz) from comment #10)
> Followup is ok, but please do file a followup.  From what I understand, on
> mobile even once-every-30-seconds wakeups are not good.

Nathan, I think adding a new timer specifically for telemetry is wrong. I think we should instead piggyback on an existing timer handler, but only record telemetry if 30s elapsed since last recording.
Comment 12 Nathan Froyd [:froydnj] 2012-01-18 06:54:16 PST
(In reply to Taras Glek (:taras) from comment #11)
> Nathan, I think adding a new timer specifically for telemetry is wrong. I
> think we should instead piggyback on an existing timer handler, but only
> record telemetry if 30s elapsed since last recording.

So instead of metronome-like data collection, we can do recently-but-unspecified-frequency data collection on existing timer expirations?
Comment 13 Nathan Froyd [:froydnj] 2012-01-18 09:40:20 PST
Created attachment 589545 [details] [diff] [review]
alternate patch with variable frequency

bz, what do you think about this patch?  It implements Taras's approach and doesn't use a separate timer.
Comment 14 (dormant account) 2012-01-19 10:08:36 PST
This is what i wanted, thanks Nathan
Comment 15 Nathan Froyd [:froydnj] 2012-01-24 11:02:57 PST
Is there a more available person to review this the alternate patch than bz?
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-24 14:23:39 PST
Comment on attachment 589545 [details] [diff] [review]
alternate patch with variable frequency

This looks fine if you add parens around the value of STATISTICS_INTERVAL
Comment 17 Nathan Froyd [:froydnj] 2012-01-25 07:39:48 PST
Created attachment 591465 [details] [diff] [review]
patch

Adding parens and tweaking one assignment.  Carrying over r+.
Comment 19 Jonathan Kew (:jfkthame) 2012-01-26 05:17:15 PST
Assertion failure: !aOther.IsNull() (Cannot compute with aOther null value), at ../../dist/include/mozilla/TimeStamp.h:221
TEST-UNEXPECTED-FAIL | automation.py | Exited with code 1 during test run

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/9b70467dfec0
Comment 20 Nathan Froyd [:froydnj] 2012-01-26 11:00:38 PST
Created attachment 591860 [details] [diff] [review]
patch

Added check for null timers; carrying over r+.  mochitests succeeded locally.
Comment 21 Daniel Holbert [:dholbert] 2012-01-30 17:18:05 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/532d055cabd6
Comment 22 Ed Morley [:emorley] 2012-01-31 06:57:02 PST
https://hg.mozilla.org/mozilla-central/rev/532d055cabd6
Comment 23 Lawrence Mandel [:lmandel] (use needinfo) 2012-02-16 13:36:07 PST
Can you confirm that this made 12 as indicated or is it in 13?

Note You need to log in before you can comment on or make changes to this bug.