1224374 - Expand BHR pseudo-stack coverage

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Vladan Djeric (:vladan)]

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.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

So if I understand correctly, I need to sprinkle magic PROFILER_LABEL* powder around the top n chromehang stacks and BHR pseudo stacks, right?

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: 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/

Comment 3

[Benoit Girard (:BenWa)]

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!

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Requesting additional feedback from marcoz for `NotificationController::WillRefresh`. Essentially, if it's a hot function, we might need to move the label.

Comment 5

[Marco Zehe (:MarcoZ)]

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

Comment 6

[Marco Zehe (:MarcoZ)]

(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.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Actually, I'm basically sure that it's not hot.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: 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/

Comment 9

[Benoit Girard (:BenWa)]

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

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Boris, what's your word on that? Are `NPObjWrapper_Resolve` and `NPObjectMember_GetProperty` too hot to add them to the label?

Comment 11

[Boris Zbarsky [: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.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

(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.

Comment 13

[Benoit Girard (:BenWa)]

Sounds good then

Comment 14

[David Teller [:Yoric] - still alive but not very active]

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

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0dc02cb0b604
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f458e6e4997

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 17

[David Teller [:Yoric] - still alive but not very active]

I'm on it, thanks.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

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]:

Comment 20

[David Teller [:Yoric] - still alive but not very active]

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

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e245e8d31839
https://hg.mozilla.org/integration/mozilla-inbound/rev/e74405918e7a

Comment 22

[Ryan VanderMeulen [:RyanVM]]

FYI, this is going to need rebased patches for Aurora.

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Attachment: 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]:

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e245e8d31839
https://hg.mozilla.org/mozilla-central/rev/e74405918e7a

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8710278 [details] [diff] [review]
Aurora version of the patch

Improve the stack traces, taking it.

Comment 26

[David Teller [:Yoric] - still alive but not very active]

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