Closed Bug 1251667 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
e10s m9+ ---
firefox45 --- wontfix
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: cpeterson, Assigned: azhang)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Jeff, do we care about this?
Flags: needinfo?(jgriffiths)
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.
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #1)
> Jeff, do we care about this?

Yes.
Flags: needinfo?(jgriffiths)
Attached patch slow-script-page-measure.patch (obsolete) — Splinter Review
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.
Attachment #8724928 - Flags: review?(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": ["perf-telemetry-alerts@mozilla.com"],
> +    "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.
Attachment #8724928 - Flags: review?(wmccloskey)
Attachment #8724928 - Flags: review?(cpeterson)
Attachment #8724928 - Flags: feedback+
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.
Attachment #8724928 - Flags: review?(wmccloskey) → review+
This new version should have all of the above changes; let me know if there's anything else.
Attachment #8724928 - Attachment is obsolete: true
Attachment #8725268 - Flags: review?(wmccloskey)
Comment on attachment 8725268 [details] [diff] [review]
slow-script-page-measure.patch

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

Looks great. Thanks.
Attachment #8725268 - Flags: review?(wmccloskey) → review+
Blocks: 1251547
https://hg.mozilla.org/mozilla-central/rev/15631f483457
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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
Attachment #8725268 - Flags: approval-mozilla-beta?
Attachment #8725268 - Flags: approval-mozilla-aurora?
Comment on attachment 8725268 [details] [diff] [review]
slow-script-page-measure.patch

Too late for 45
Attachment #8725268 - Flags: approval-mozilla-beta?
Attachment #8725268 - Flags: approval-mozilla-beta-
Attachment #8725268 - Flags: approval-mozilla-aurora?
Attachment #8725268 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.