Closed Bug 1224374 Opened 4 years ago Closed 4 years ago

Expand BHR pseudo-stack coverage

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox45 --- fixed
firefox46 --- fixed

People

(Reporter: vladan, Assigned: Yoric)

References

Details

Attachments

(3 files)

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.
Blocks: 1182637
Blocks: 1222972
Blocks: 1229104
Depends on: 1231968
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?
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)
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/
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/
Whiteboard: [e10s-45-uplift]
FYI, this is going to need rebased patches for Aurora.
Flags: needinfo?(vladan.bugzilla)
Flags: needinfo?(vladan.bugzilla) → needinfo?(dteller)
Blocks: 1241336
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?
https://hg.mozilla.org/mozilla-central/rev/e245e8d31839
https://hg.mozilla.org/mozilla-central/rev/e74405918e7a
Status: NEW → RESOLVED
Closed: 4 years ago
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+
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.