1384238 - Annotate BHR hangs occuring while there is a pending input event

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 3

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

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

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Changing the meaning should be fine.

Comment 5

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

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?

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Annotate BHR hangs which occur while there is a pending input event

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

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebd9e87fae8d
Annotate BHR hangs which occur while there is a pending input event, r=smaug

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ebd9e87fae8d

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Has hangs.html been updated to use this annotation now?

Comment 12

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

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?

Comment 13

[Nika Layzell [:nika] (ni? for response)]

I filed bug 1393912 to fix that - oops.