1251667 - [e10s] Add new telemetry probe to count pages with one or more slow script notes (SLOW_SCRIPT_PAGE_COUNT?)

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Chris Peterson [:cpeterson]]

SLOW_SCRIPT_NOTICE_COUNT is not comparable between e10s and non-e10s. The slow script notice is modal in non-e10s, but asynchronous in e10s which can lead to double-counting of e10s slow script notices. This problem was originally reported in bug 1249978.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Jeff, do we care about this?

Comment 2

[Chris Peterson [:cpeterson]]

Yes. This bug is an action item from a meeting with Jeff, Benjamin, Anthony, and Bill. This new metric will replace SLOW_SCRIPT_NOTICE_COUNT.

Comment 3

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #1)
> Jeff, do we care about this?

Yes.

Comment 4

[Anthony Zhang [:azhang] (last day at Mozilla: 2016-04-29)]

Attachment: slow-script-page-measure.patch

Feel free to redirect the r? if needed.

Basically, we keep a flag keeping track of whether the page has been navigated since the last slow script notice for that page.

I've been verifying this for a while in e10s and non-e10s, and have not found any issues. Note that in e10s, the histogram will show up in child payloads rather than the parent payload, which should be taken into account when doing the analysis later on.

Besides the new measure, this patch doesn't touch anything else.

Comment 5

[Chris Peterson [:cpeterson]]

Comment on attachment 8724928 [details] [diff] [review]
slow-script-page-measure.patch

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

LGTM but I'm not the right person to review. Sending review to Bill.

::: dom/base/nsGlobalWindow.cpp
@@ +2483,5 @@
>    // having to *always* reach into the inner window to find the
>    // document.
>    mDoc = aDocument;
>  
> +  // Reset the slow script flag for since we're setting a new document.

Should the word "for" be removed?

::: toolkit/components/telemetry/Histograms.json
@@ +9067,5 @@
>      "description": "Count slow script notices"
>    },
> +  "SLOW_SCRIPT_PAGE_COUNT": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "never",

btw, the telemetry module owners prefer that new telemetry probes have an expiration version, but since SLOW_SCRIPT_NOTICE_COUNT has none, this is probably OK for SLOW_SCRIPT_PAGE_COUNT.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8724928 [details] [diff] [review]
slow-script-page-measure.patch

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

::: dom/base/nsGlobalWindow.cpp
@@ +2486,5 @@
>  
> +  // Reset the slow script flag for since we're setting a new document.
> +  // The flag is meant to represent whether scripts have been slow for
> +  // the currently loaded document.
> +  mHasHadSlowScript = false;

This line isn't really doing anything. There are two kinds of nsGlobalWindows: outer and inner. An outer window roughly corresponds to a tab: if you navigate, you stay withing the same outer window. An inner window is roughly 1:1 with a document: when you navigate, you get a different inner window. An inner window has a pointer to its outer window, and an outer window has a pointer to its current inner window. SetNewDocument is called on an outer window, and it changes the current inner window. ShowSlowScriptDialog is called on an inner window.

Since ShowSlowScriptDialog uses the mHasHadSlowScript on the inner window, you never even need to set the flag to false. When we navigate, we'll get a new inner window with a new flag.

The mHasHadSlowScript flag on outer windows is unused and you're just repeatedly setting it to false here. So we can remove this statement.

::: dom/base/nsGlobalWindow.h
@@ +1691,5 @@
>    // Window offline status. Checked to see if we need to fire offline event
>    bool                          mWasOffline : 1;
>  
> +  // Whether the page being shown by the window has had a slow script.
> +  // This is used by Telemetry measures such as SLOW_SCRIPT_PAGE_COUNT.

Please make it clear that this flag is only used on inner windows.

Comment 7

[Anthony Zhang [:azhang] (last day at Mozilla: 2016-04-29)]

Attachment: slow-script-page-measure.patch

This new version should have all of the above changes; let me know if there's anything else.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8725268 [details] [diff] [review]
slow-script-page-measure.patch

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

Looks great. Thanks.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15631f483457

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/15631f483457

Comment 11

[Anthony Zhang [:azhang] (last day at Mozilla: 2016-04-29)]

Comment on attachment 8725268 [details] [diff] [review]
slow-script-page-measure.patch

Approval Request Comment
[Feature/regressing bug #]:
bug 8725268, bug 1249978

[User impact if declined]:
We would not have a comparable metric for slow scripts between e10s and non-e10s, which is needed for release criteria evaluation.

See [1] for more details.

[Describe test coverage new/current, TreeHerder]:
No changes.

[Risks and why]: 
Low risk; just adds a single Telemetry measure.

[String/UUID change made/needed]:
No changes.

[1]: https://wiki.mozilla.org/Electrolysis/Release_Criteria/Slow_Script

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8725268 [details] [diff] [review]
slow-script-page-measure.patch

Too late for 45

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e13c24050f02