Closed Bug 715953 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

(Whiteboard: [Snappy:P3])

Attachments

(1 file, 4 obsolete files)

Splitting off from bug 715380.

When were you expecting this computation to happen?  At each setTimeout, at timeout expiry, or some other time?
Would be cool to do this in a mouse event handler, but bz might have a better suggestion.
CC'ing bz to see if he has a better suggestion on when to measure this.
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)?
(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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #588423 - Flags: review?(bzbarsky)
Comment on attachment 588423 [details] [diff] [review]
patch

This leaks the timer through shutdown, no?
Attachment #588423 - Flags: review?(bzbarsky) → review-
Attached patch patch (obsolete) — Splinter Review
Good point.  Now with NS_IF_RELEASE.
Attachment #588423 - Attachment is obsolete: true
Attachment #588483 - Flags: review?(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?
Attachment #588483 - Flags: review?(bzbarsky) → review+
(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.
Followup is ok, but please do file a followup.  From what I understand, on mobile even once-every-30-seconds wakeups are not good.
Blocks: 718212
(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.
(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?
bz, what do you think about this patch?  It implements Taras's approach and doesn't use a separate timer.
Attachment #589545 - Flags: review?(bzbarsky)
This is what i wanted, thanks Nathan
Is there a more available person to review this the alternate patch than bz?
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
Attachment #589545 - Flags: review?(bzbarsky) → review+
Attached patch patch (obsolete) — Splinter Review
Adding parens and tweaking one assignment.  Carrying over r+.
Assignee: nobody → nfroyd
Attachment #588483 - Attachment is obsolete: true
Attachment #589545 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #591465 - Flags: review+
Keywords: checkin-needed
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
Attached patch patchSplinter Review
Added check for null timers; carrying over r+.  mochitests succeeded locally.
Attachment #591465 - Attachment is obsolete: true
Attachment #591860 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/532d055cabd6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [Snappy:P3]
Can you confirm that this made 12 as indicated or is it in 13?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.