715953 - add telemetry for how many background timeouts are scheduled within 30seconds

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nathan Froyd [:froydnj]]

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)]

Would be cool to do this in a mouse event handler, but bz might have a better suggestion.

Comment 2

[Nathan Froyd [:froydnj]]

CC'ing bz to see if he has a better suggestion on when to measure this.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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)]

(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]]

Attachment: 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 [:bzbarsky]]

Comment on attachment 588423 [details] [diff] [review]
patch

This leaks the timer through shutdown, no?

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: patch

Good point.  Now with NS_IF_RELEASE.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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]]

(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 [:bzbarsky]]

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)]

(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]]

(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]]

Attachment: 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)]

This is what i wanted, thanks Nathan

Comment 15

[Nathan Froyd [:froydnj]]

Is there a more available person to review this the alternate patch than bz?

Comment 16

[Boris Zbarsky [:bzbarsky]]

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]]

Attachment: patch

Adding parens and tweaking one assignment.  Carrying over r+.

Comment 18

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/26509d4e545d

Comment 19

[Jonathan Kew [:jfkthame]]

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]]

Attachment: patch

Added check for null timers; carrying over r+.  mochitests succeeded locally.

Comment 21

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/532d055cabd6

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/532d055cabd6

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Can you confirm that this made 12 as indicated or is it in 13?