Record markers for Background Hangs

RESOLVED FIXED in Firefox 59

Status

()

RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

unspecified
mozilla59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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 hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
Looks like I've been confused because this code is running in its own thread that is not recorded by default.
Comment hidden (mozreview-request)
(Assignee)

Comment 4

a year ago
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
(Assignee)

Comment 5

a year ago
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.
Attachment #8935362 - Flags: review?(dothayer)

Comment 6

a year ago
mozreview-review
Comment on attachment 8935362 [details]
Bug 1423890 - Add markers for background hangs

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

Looks good to me.
Attachment #8935362 - Flags: review?(dothayer) → review+

Comment 7

10 months ago
mozreview-review
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.
Attachment #8935362 - Flags: review?(mstange) → review+
(Assignee)

Updated

10 months ago
Assignee: nobody → poirot.alex
Comment hidden (mozreview-request)
(Assignee)

Comment 9

10 months ago
(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

10 months ago
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/88e1c3fbec11
Add markers for background hangs r=dthayer,mstange

Comment 11

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/88e1c3fbec11
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.