1423890 - Record markers for Background Hangs

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[Alexandre Poirot [:ochameau]]

To better understand how busy the event loop is and better debug BHR reports, it would be helpful to display markers for Bakground Hangs.
I'm wondering how much BHR reports relates to "event processing delay" we display in perf-html. May be this is redundant, but having the markers would help verifying.

I tried to craft the C++ patch to do that, but I can't see the marker appear in perf-html:
  https://perfht.ml/2nE7qhl
  STR to get background hangs is easy, just open the devtools.
  Here, I can see the user timings added from JS code,
  but I can't see the markers recorded from BackgroundHangMonitor.cpp.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1423890 - Add markers for background hangs

Review commit: https://reviewboard.mozilla.org/r/206258/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206258/

Comment 2

[Alexandre Poirot [:ochameau]]

Looks like I've been confused because this code is running in its own thread that is not recorded by default.

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8935362 [details]
Bug 1423890 - Add markers for background hangs

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

Comment 4

[Alexandre Poirot [:ochameau]]

I've not been really inspired to find a fancy name for this marker, but do not hesitate to suggest anything else.
Here is a sample profile:
  https://perfht.ml/2nEuNYk

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8935362 [details]
Bug 1423890 - Add markers for background hangs

Doug, would you mind reviewing the BHR part? froydnj is away until 2018 now.

Comment 6

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8935362 [details]
Bug 1423890 - Add markers for background hangs

https://reviewboard.mozilla.org/r/206258/#review211948

Looks good to me.

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8935362 [details]
Bug 1423890 - Add markers for background hangs

https://reviewboard.mozilla.org/r/206258/#review217532

Sorry for the extremely long delay here. I've tested it and it looks good, but the current name "Hang" is probably a bit confusing for users because it's not clear how it's different from the red "responsiveness" annotations. Let's rename it.

::: toolkit/components/backgroundhangmonitor/HangDetails.cpp:281
(Diff revision 2)
> +    if (profiler_is_active()) {
> +      TimeStamp endTime = hangDetails->mDetails.mEndTime;
> +      TimeStamp startTime = endTime -
> +                            TimeDuration::FromMilliseconds(hangDetails->mDetails.mDuration);
> +      profiler_add_marker(
> +        "Hang",

Let's change the name to "BHR-detected hang", either here or in the other place. I'm not sure which one of these strings determines what we show in the tooltip on perf.html.

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 8935362 [details]
Bug 1423890 - Add markers for background hangs

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

Comment 9

[Alexandre Poirot [:ochameau]]

(In reply to Markus Stange [:mstange] from comment #7)
> Let's change the name to "BHR-detected hang", either here or in the other
> place. I'm not sure which one of these strings determines what we show in
> the tooltip on perf.html.

I think that's the one in HangMarkerPayload::StreamPayload, but I renamed both for consistency.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88e1c3fbec11
Add markers for background hangs r=dthayer,mstange

Comment 11

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/88e1c3fbec11