Annotate BHR hangs occuring while there is a pending input event

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Nika, Assigned: Nika)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

We could annotate the hangs which occur while an input event is pending (has been received on the I/O thread, but has not been processed on the main thread yet).

This would be helpful for using BHR to improve our input responsiveness metrics.
From smaug: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/layout/base/PresShell.cpp#7977,8261,8271 is where we collect the input event telemetry - we should clear any flags we set at that point.
And somehow the UI should let one to filter "there is pending input event or we're handling input" hangs.
(I could imagine similar flag for vsync etc. but that isn't about this bug)
(In reply to Olli Pettay [:smaug] from comment #2)
> And somehow the UI should let one to filter "there is pending input event or
> we're handling input" hangs.
> (I could imagine similar flag for vsync etc. but that isn't about this bug)

There's already a checkbox for this idea in the http://squarewave.github.io UI , but it's just using the "UserInteracting" annotation which I believe is just based on a flag that we poll for every 5 seconds to check if we've received any mouse or keyboard events in that time. Do we want to just change the checkbox's meaning, or add another checkbox? I could see it being confusing to have two filters that mean the same thing just measured in two different ways.
Changing the meaning should be fine.
I'm wondering if it might be useful for Chrome hangs to annotate the hang with the ID of the event target. So long as we ensure that we're not including IDs of content elements, this should be fine from a privacy perspective, no?
This doesn't handle messages recieved when PContentBridge is the manager, as that goes through a different code path. I figured I'd get this patch reviewed to make sure I'm on the right path then I'll add that code in.

This will record at the 100ms mark whether or not there are any input events in the queue which are being blocked by the hanging runnable.

MozReview-Commit-ID: HRPMw2IfEKB
Attachment #8895540 - Flags: review?(bugs)
Comment on attachment 8895540 [details] [diff] [review]
Annotate BHR hangs which occur while there is a pending input event

>+  /**
>+   * Returns true if the passed-in mesasge is a pending InputEvent.
mesasge? message
and perhaps
is a pending InputEvent or otherwise equally important message (since the list contains also other stuff).

>+#ifdef NIGHTLY_BUILD
>+void
>+ContentChild::OnChannelReceivedMessage(const Message& aMsg)
>+{
>+  if (nsContentUtils::IsMessageInputEvent(aMsg)) {
>+    mPendingInputEvents++;
>+  }
>+}
>+
>+PContentChild::Result
>+ContentChild::OnMessageReceived(const Message& aMsg)
>+{
>+  if (nsContentUtils::IsMessageInputEvent(aMsg)) {
>+    DebugOnly<uint32_t> prevEvts = mPendingInputEvents--;
>+    MOZ_ASSERT(prevEvts > 0);
>+  }
>+
>+  return PContentChild::OnMessageReceived(aMsg);
>+}
>+
>+PContentChild::Result
>+ContentChild::OnMessageReceived(const Message& aMsg, Message*& aReply)
>+{
>+  return PContentChild::OnMessageReceived(aMsg, aReply);
>+}
I'm not familiar with this part of IPC, so I don't know who calls which of these and when.
Attachment #8895540 - Flags: review?(bugs) → review?(kchen)
Comment on attachment 8895540 [details] [diff] [review]
Annotate BHR hangs which occur while there is a pending input event

Ok, based on mystor's explanation and after looking some code, this looks reasonable.

(I'm surprised PContentChild::OnMessageReceived implementation seems to require a hashtable lookup for each message.)
Attachment #8895540 - Flags: review+
Attachment #8895540 - Flags: review?(kchen) → review+
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebd9e87fae8d
Annotate BHR hangs which occur while there is a pending input event, r=smaug
https://hg.mozilla.org/mozilla-central/rev/ebd9e87fae8d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Has hangs.html been updated to use this annotation now?
Assignee: nobody → michael
Flags: needinfo?(dothayer)
Hmm, it has been updated to use it, but I'm not seeing anything come through.

Ran this as a quick test:

pings.flatMap(lambda p: p['payload/hangs']).flatMap(lambda h: h['annotations'].keys()).countByValue()

Output:
> defaultdict(int,
>             {u'UserInteracting': 1347326,
>              u'pluginName': 1328,
>              u'pluginVersion': 1328})

Any thoughts? Am I missing something - I should be seeing "PendingInput" in there, correct?
Flags: needinfo?(dothayer) → needinfo?(michael)
Blocks: 1393912
I filed bug 1393912 to fix that - oops.
Flags: needinfo?(michael)
You need to log in before you can comment on or make changes to this bug.