Record markers for Background Hangs

RESOLVED FIXED in Firefox 59

Status

()

Core
Gecko Profiler
RESOLVED FIXED
2 months ago
10 days ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

unspecified
mozilla59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

2 months 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

2 months 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

2 months 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

2 months 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

2 months 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

11 days 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

11 days ago
Assignee: nobody → poirot.alex
Comment hidden (mozreview-request)
(Assignee)

Comment 9

11 days 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

11 days 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 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/88e1c3fbec11
Status: NEW → RESOLVED
Last Resolved: 10 days 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.