Record js slow script dialog delay in telemetry

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

(Blocks: 1 bug)

Trunk
mozilla49
Points:
---

Firefox Tracking Flags

(firefox47 fixed, firefox48 fixed, firefox49 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
Testing in bug 1260769 found that the slow script notification may be more responsive under e10s. Add a probe that measures the difference between slow script timeout prefs and the actual time it takes to trigger the notification. compare these values under e01s and non-e10s.

prefs:
dom.max_script_run_time
dom.max_chrome_script_run_time
(Assignee)

Comment 1

3 years ago
Created attachment 8751451 [details] [diff] [review]
patch

Adds a telemetry probe for content timeouts.
Assignee: nobody → jmathies
Attachment #8751451 - Flags: review?(wmccloskey)
Comment on attachment 8751451 [details] [diff] [review]
patch

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1364,5 @@
>      // care of that case.
>      if (self->mSlowScriptCheckpoint.IsNull()) {
>          self->mSlowScriptCheckpoint = TimeStamp::NowLoRes();
>          self->mSlowScriptSecondHalf = false;
> +        self->mSlowScriptActualWait = 0;

This would be cleaner as |mSlowScriptActualWait = mozilla::TimeDuration()|.

@@ +1436,5 @@
>      }
>  
> +    // Accumulate slow script invokation delay.
> +    if (!chrome && !self->mTimeoutAccumulated) {
> +      uint32_t delay = uint32_t(floor(self->mSlowScriptActualWait.ToMilliseconds() - (limit * 1000.0)));

I don't think you need floor here.

@@ +3712,5 @@
>  
>      // Start the slow script timer.
>      mSlowScriptCheckpoint = mozilla::TimeStamp::NowLoRes();
>      mSlowScriptSecondHalf = false;
> +    mSlowScriptActualWait = 0;

Same as above.
Attachment #8751451 - Flags: review?(wmccloskey) → review+
Whiteboard: btpp-active

Comment 4

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1a022b6b949c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Comment 5

3 years ago
Comment on attachment 8751451 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]:
no  bug, e10s related diagnostic telemetry probe
[User impact if declined]:
none
[Describe test coverage new/current, TreeHerder]:
on mc for a day or so
[Risks and why]: 
low, well understood code changes
[String/UUID change made/needed]:
none
Attachment #8751451 - Flags: approval-mozilla-beta?
Attachment #8751451 - Flags: approval-mozilla-aurora?

Updated

3 years ago
status-firefox47: --- → affected
status-firefox48: --- → affected

Comment 6

3 years ago
Comment on attachment 8751451 [details] [diff] [review]
patch

Telemetry data on slow script dialog delay, Aurora48+, Beta47+
Attachment #8751451 - Flags: approval-mozilla-beta?
Attachment #8751451 - Flags: approval-mozilla-beta+
Attachment #8751451 - Flags: approval-mozilla-aurora?
Attachment #8751451 - Flags: approval-mozilla-aurora+

Comment 7

3 years ago
Hi Jim, for new telemetry probes that need uplift to Aurora/Beta, it would be nice if we can verify the expected data is coming on Nightly before uplifting to Aurora/Beta. This is to avoid unnecessary churn. Thanks!
Flags: needinfo?(jmathies)

Comment 8

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/b27e0175db55
status-firefox48: affected → fixed

Comment 9

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/a9e1d0bc8a8c
status-firefox47: affected → fixed
(Assignee)

Comment 10

3 years ago
(In reply to Ritu Kothari (:ritu) from comment #7)
> Hi Jim, for new telemetry probes that need uplift to Aurora/Beta, it would
> be nice if we can verify the expected data is coming on Nightly before
> uplifting to Aurora/Beta. This is to avoid unnecessary churn. Thanks!

Data is starting to trickle in from nightly - 

https://telemetry.mozilla.org/new-pipeline/dist.html#!compare=e10sEnabled&cumulative=0&end_date=2016-05-13&keys=__none__!__none__!__none__&max_channel_version=nightly%252F49&measure=SLOW_SCRIPT_NOTIFY_DELAY&min_channel_version=null&product=Firefox&sanitize=0&sort_keys=submissions&start_date=2016-05-13&table=0&trim=0&use_submission_date=0
Flags: needinfo?(jmathies)
(Assignee)

Comment 11

3 years ago
Created attachment 8752780 [details]
SLOW_SCRIPT_NOTIFY_DELAY pings.png

The distribution is interesting as well as the raw number of pings we receive back from the browser. Currently under e10s we are receiving 7x more telemetry pings containing this probe.
(Assignee)

Comment 12

3 years ago
(In reply to Jim Mathies [:jimm] from comment #11)
> Created attachment 8752780 [details]
> SLOW_SCRIPT_NOTIFY_DELAY pings.png
> 
> The distribution is interesting as well as the raw number of pings we
> receive back from the browser. Currently under e10s we are receiving 7x more
> telemetry pings containing this probe.

In thinking about this, the 7x number probably has to do with very few users running non-e10s on night/aurora. So this may not be significant.
(Assignee)

Comment 13

3 years ago
(In reply to Wes Kocher (:KWierso) from comment #9)
> https://hg.mozilla.org/releases/mozilla-beta/rev/a9e1d0bc8a8c

Hey Ritu, do you know what beta build this will land in?
Flags: needinfo?(rkothari)
(In reply to Jim Mathies [:jimm] from comment #13)
> (In reply to Wes Kocher (:KWierso) from comment #9)
> > https://hg.mozilla.org/releases/mozilla-beta/rev/a9e1d0bc8a8c
> 
> Hey Ritu, do you know what beta build this will land in?

Hi Jim, it should be in 47.0b6 (which we gtb today noon PST).
Flags: needinfo?(rkothari)
You need to log in before you can comment on or make changes to this bug.