Expand BHR pseudo-stack coverage

RESOLVED FIXED in Firefox 45

Status

()

Toolkit
Telemetry
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: vladan, Assigned: Yoric)

Tracking

(Blocks: 1 bug)

Trunk
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed, firefox46 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
We'll be relying heavily on BHR pseudo-stacks to narrow down the causes of the apparent responsiveness regression in e10s (bug 1182637), so we should expand coverage of the BHR pseudostack.

The labels are mostly in gfx according to BenWa. We noticed asyncPluginInit doesn't have labels, etc.

We can generate the list of missing labels by triggering chromehang-style stackwalks from BHR. The chromehangs dash is another good starting point.
(Reporter)

Updated

2 years ago
Blocks: 1182637
(Reporter)

Updated

2 years ago
Blocks: 1222972
(Reporter)

Updated

2 years ago
Blocks: 1229104

Updated

2 years ago
Depends on: 1231968

Updated

a year ago
Depends on: 1071883
Assignee: vladan.bugzilla → dteller
So if I understand correctly, I need to sprinkle magic PROFILER_LABEL* powder around the top n chromehang stacks and BHR pseudo stacks, right?
Created attachment 8708086 [details]
MozReview Request: Bug 1224374 - Profiler labels for the 25 top chrome hangs;r?benwa

Experience shows that we do not have enough profiler labels to make
BHR hang reports meaningful. This patch adds enough labels to let us
exploit hang reports matching the 25 topmost chrome hangs.

Review commit: https://reviewboard.mozilla.org/r/30965/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30965/
Attachment #8708086 - Flags: review?(bgirard)
Comment on attachment 8708086 [details]
MozReview Request: Bug 1224374 - Profiler labels for the 25 top chrome hangs;r?benwa

https://reviewboard.mozilla.org/r/30965/#review27813

Looks alright. None of these look like they are hot functions but if you're unsure please ask the module owner for feedback. Otherwise feel free to land!
Attachment #8708086 - Flags: review?(bgirard) → review+
Attachment #8708086 - Flags: review?(n.nethercote)
Attachment #8708086 - Flags: review?(n.nethercote)
Attachment #8708086 - Flags: review?(mzehe)
Requesting additional feedback from marcoz for `NotificationController::WillRefresh`. Essentially, if it's a hot function, we might need to move the label.
Comment on attachment 8708086 [details]
MozReview Request: Bug 1224374 - Profiler labels for the 25 top chrome hangs;r?benwa

https://reviewboard.mozilla.org/r/30965/#review27983
Attachment #8708086 - Flags: review?(mzehe) → review+
(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #4)
> Requesting additional feedback from marcoz for
> `NotificationController::WillRefresh`. Essentially, if it's a hot function,
> we might need to move the label.

I'm OK with the label as is, but for a more technical review, especially what the "hot function" part is concerned, :surkov or :tbsaunde are better candidates to give this a look-over and decide whether the label should be moved or not.
Flags: needinfo?(dteller)
Actually, I'm basically sure that it's not hot.
Flags: needinfo?(dteller)
Created attachment 8708942 [details]
MozReview Request: Bug 1224374 - Profiler labels for the top 26-100 chrome hangs;r?benwa

Review commit: https://reviewboard.mozilla.org/r/31021/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31021/
Attachment #8708942 - Flags: review?(bgirard)
Attachment #8708942 - Flags: review?(bgirard) → review+
Comment on attachment 8708942 [details]
MozReview Request: Bug 1224374 - Profiler labels for the top 26-100 chrome hangs;r?benwa

https://reviewboard.mozilla.org/r/31021/#review28065

Please check with devtools if your labels are going to start showing up in the DevTools profiler. Some of this we don't want to expose to developers. I believe they might show anything that isn't category::other. If that's the case we may want to put some of these as category::other.

::: dom/plugins/base/nsJSNPRuntime.cpp:1679
(Diff revision 1)
> +  PROFILER_LABEL_FUNC(js::ProfileEntry::Category::JS);

I'm not familiar with this code but these resolve/getter could very likely be hot. I'd suggest that you get a peer to review this.

::: dom/plugins/base/nsNPAPIPluginInstance.cpp:285
(Diff revision 1)
> -
> +  

You've added trailing whitespace here
Boris, what's your word on that? Are `NPObjWrapper_Resolve` and `NPObjectMember_GetProperty` too hot to add them to the label?
Flags: needinfo?(bzbarsky)
I wouldn't expect them to be particularly hot in the grand scheme of things.  They only get called when manipulating plugin-provided objects from JS, which should actually be fairly rare on a "number of pages" basis.

I have no idea how hot they would be on pages that _do_ perform such manipulation.
Flags: needinfo?(bzbarsky)
(In reply to Benoit Girard (:BenWa) from comment #9)
> Please check with devtools if your labels are going to start showing up in
> the DevTools profiler. Some of this we don't want to expose to developers. I
> believe they might show anything that isn't category::other. If that's the
> case we may want to put some of these as category::other.

Apparently, they are not going to show up.

(In reply to Boris Zbarsky [:bz] from comment #11)
> I wouldn't expect them to be particularly hot in the grand scheme of things.
> They only get called when manipulating plugin-provided objects from JS,
> which should actually be fairly rare on a "number of pages" basis.
> 
> I have no idea how hot they would be on pages that _do_ perform such
> manipulation.

I suspect that plug-in manipulation is not that fast regardless, so I'm for taking the chance.
Sounds good then
Comment on attachment 8708942 [details]
MozReview Request: Bug 1224374 - Profiler labels for the top 26-100 chrome hangs;r?benwa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31021/diff/1-2/

Comment 15

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dc02cb0b604
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f458e6e4997
Backed out for compile error on Windows:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f98e2cc0df1d

11:56:00     INFO -  nsFilePicker.cpp
11:56:00     INFO -  c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/src/widget/windows/nsFilePicker.cpp(871) : error C3083: 'ProfileEntry': the symbol to the left of a '::' must be a type
11:56:00     INFO -  c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/src/widget/windows/nsFilePicker.cpp(871) : error C3083: 'Category': the symbol to the left of a '::' must be a type
11:56:00     INFO -  c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/src/widget/windows/nsFilePicker.cpp(871) : error C2039: 'OTHER' : is not a member of 'js'
11:56:00     INFO -  c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/src/widget/windows/nsFilePicker.cpp(871) : error C2065: 'OTHER' : undeclared identifier
11:56:00     INFO -  c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/src/widget/windows/nsFilePicker.cpp(871) : error C3861: 'PROFILER_LABEL_FUNC': identifier not found
11:56:00     INFO -  c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/src/config/rules.mk:957: recipe for target 'nsFilePicker.obj' failed
11:56:00     INFO -  mozmake.EXE[5]: *** [nsFilePicker.obj] Error 2
Flags: needinfo?(dteller)
I'm on it, thanks.
Flags: needinfo?(dteller)
Comment on attachment 8708086 [details]
MozReview Request: Bug 1224374 - Profiler labels for the 25 top chrome hangs;r?benwa

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Our BHR stack traces will remain largely useless, which will render the results of our next e10s experiment much less useful.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Shouldn't be any. Just profiler labels.
[String/UUID change made/needed]:

This needs to be uplifted during the week.
Attachment #8708086 - Flags: approval-mozilla-aurora?
Comment on attachment 8708942 [details]
MozReview Request: Bug 1224374 - Profiler labels for the top 26-100 chrome hangs;r?benwa

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8708942 - Flags: approval-mozilla-aurora?
Comment on attachment 8708942 [details]
MozReview Request: Bug 1224374 - Profiler labels for the top 26-100 chrome hangs;r?benwa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31021/diff/2-3/

Comment 21

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e245e8d31839
https://hg.mozilla.org/integration/mozilla-inbound/rev/e74405918e7a
Whiteboard: [e10s-45-uplift]
FYI, this is going to need rebased patches for Aurora.
Flags: needinfo?(vladan.bugzilla)
Flags: needinfo?(vladan.bugzilla) → needinfo?(dteller)
(Reporter)

Updated

a year ago
Blocks: 1241336
Created attachment 8710278 [details] [diff] [review]
Aurora version of the patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Flags: needinfo?(dteller)
Attachment #8710278 - Flags: review+
Attachment #8710278 - Flags: approval-mozilla-aurora?
Attachment #8708086 - Flags: approval-mozilla-aurora?
Attachment #8708942 - Flags: approval-mozilla-aurora?

Comment 24

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e245e8d31839
https://hg.mozilla.org/mozilla-central/rev/e74405918e7a
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment on attachment 8710278 [details] [diff] [review]
Aurora version of the patch

Improve the stack traces, taking it.
Attachment #8710278 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/c7ca88c57386
status-firefox45: affected → fixed
Whiteboard: [e10s-45-uplift]
Duplicate of this bug: 1242717
Duplicate of this bug: 1242717
Duplicate of this bug: 1163666
You need to log in before you can comment on or make changes to this bug.