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

RESOLVED FIXED in mozilla12

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:P3])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 years ago
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

6 years ago
Would be cool to do this in a mouse event handler, but bz might have a better suggestion.
(Assignee)

Comment 2

6 years ago
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)?

Comment 4

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
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.
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-
(Assignee)

Comment 7

6 years ago
Created attachment 588483 [details] [diff] [review]
patch

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+
(Assignee)

Comment 9

6 years ago
(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.
(Assignee)

Updated

6 years ago
Blocks: 718212

Comment 11

6 years ago
(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.
(Assignee)

Comment 12

6 years ago
(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?
(Assignee)

Comment 13

6 years ago
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.
Attachment #589545 - Flags: review?(bzbarsky)

Comment 14

6 years ago
This is what i wanted, thanks Nathan
(Assignee)

Comment 15

6 years ago
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+
(Assignee)

Comment 17

6 years ago
Created attachment 591465 [details] [diff] [review]
patch

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+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/26509d4e545d
Keywords: checkin-needed
Target Milestone: --- → mozilla12
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
(Assignee)

Comment 20

6 years ago
Created attachment 591860 [details] [diff] [review]
patch

Added check for null timers; carrying over r+.  mochitests succeeded locally.
Attachment #591465 - Attachment is obsolete: true
Attachment #591860 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/532d055cabd6
Keywords: checkin-needed
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/532d055cabd6
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Snappy:P3]
Can you confirm that this made 12 as indicated or is it in 13?
You need to log in before you can comment on or make changes to this bug.