1345540 - Measure input event handler to paint request latency

Status: RESOLVED FIXED

(Toolkit :: Performance Monitoring, enhancement)

Description

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

In bug 1313465, we want to measure event to dispatch latency, which can be divide to three parts:
1. from user input to event dispatcher (including IPC)
2. from event dispatcher to paint request (mostly in JavaScript engine)
3. from paint request to actual rendering (compositor, gfx ... etc.)

This bug measures the second part.

Comment 1

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: Bug 1345540 - Measure input event handling latency. , data-review?bsmedberg

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

Comment 2

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Comment on attachment 8859980 [details]
Bug 1345540 - Measure input event handling latency. , data-review?bsmedberg

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

Comment 3

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

Comment on attachment 8859980 [details]
Bug 1345540 - Measure input event handling latency. , data-review?bsmedberg

https://reviewboard.mozilla.org/r/132026/#review134816

::: toolkit/components/telemetry/Histograms.json:11407
(Diff revision 2)
> +    "expires_in_version": "60",
> +    "kind": "exponential",
> +    "high": 5000,
> +    "n_buckets": 100,
> +    "bug_numbers": [1345540],
> +    "description": "Time (ms) for the mouse click event spent in handlers."

This isn't quite right, since the events are mousedown/up.
recording mouseup would include click, but mousedown certainly isn't including click

Perhaps we need to tweak INPUT_EVENT_QUEUED_CLICK_MS too a bit.

Comment 4

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

And this isn't about part (2).



Perhaps _CLICK_ should be split to MOUSEDOWN and MOUSEUP

Comment 5

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Comment on attachment 8859980 [details]
Bug 1345540 - Measure input event handling latency. , data-review?bsmedberg

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

Comment 6

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

Comment on attachment 8859980 [details]
Bug 1345540 - Measure input event handling latency. , data-review?bsmedberg

https://reviewboard.mozilla.org/r/132026/#review135674

I mostly don't understand why this is so different to the previous version of the patch.
Keeping all the measurements in presshell wouldn't affect microbenchmarks at least.

::: dom/events/EventListenerManager.cpp:74
(Diff revision 3)
>      (mIsMainThreadELM && ls->mTypeAtom == userType) ||                  \
>      (!mIsMainThreadELM && ls->mTypeString.Equals(typeString)))) ||      \
>     (allEvents && ls->mAllEvents))
>  
> +
> +class AutoAccumulateHandlerTime {

{ goes to its own line with class definitions.

::: dom/events/EventListenerManager.cpp:1156
(Diff revision 3)
>      result = CompileEventHandlerInternal(aListener, nullptr, nullptr);
>      aListener = nullptr;
>    }
>  
>    if (NS_SUCCEEDED(result)) {
> +    AutoAccumulateHandlerTime timer(aDOMEvent);

We definitely don't want this here. This would most probably start to show up in microbenchmarks:
addref/release event for each listener call and also calling Now()

Comment 7

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Comment on attachment 8859980 [details]
Bug 1345540 - Measure input event handling latency. , data-review?bsmedberg

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

Comment 8

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

Comment on attachment 8859980 [details]
Bug 1345540 - Measure input event handling latency. , data-review?bsmedberg

https://reviewboard.mozilla.org/r/132026/#review136212

Comment 9

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Comment on attachment 8859980 [details]
Bug 1345540 - Measure input event handling latency. , data-review?bsmedberg

Request data review.

Comment 10

[Benjamin Smedberg]

Comment on attachment 8859980 [details]
Bug 1345540 - Measure input event handling latency. , data-review?bsmedberg

https://reviewboard.mozilla.org/r/132026/#review138062

data-r=me

Comment 11

[Carsten Book [:Tomcat]]

seems to have 3 open issues in mozreview that need to be fixed first before we can use autoland

Comment 12

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Ah, I forgot about that, fixed now.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e9a2a51f1c14
Measure input event handling latency. r=bsmedberg,smaug, data-review?bsmedberg

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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