Closed
Bug 1251667
Opened 9 years ago
Closed 9 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)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla47
People
(Reporter: cpeterson, Assigned: azhang)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
billm
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 2•9 years ago
|
||
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•9 years ago
|
||
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #1)
> Jeff, do we care about this?
Yes.
Flags: needinfo?(jgriffiths)
Assignee | ||
Comment 4•9 years ago
|
||
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)
Reporter | ||
Comment 5•9 years ago
|
||
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+
Assignee | ||
Comment 7•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Keywords: checkin-needed
Comment 10•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Assignee | ||
Comment 11•9 years ago
|
||
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 12•9 years ago
|
||
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+
Comment 13•9 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•