Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2695603004:
[VSync Queue] Add tracing for event queuing time and coalesced count (Closed)
Description[VSync Queue] Add tracing for event queuing time and coalesced count
We need these tracing to understand when the events are push and
popped, and how many events are coalesced together.
The tracing works as follow:
1. For each new event, we first check if it could be coalesced with
existing events or have to be pushed to the end
2. If it could be coalesced, we simply coalesced it
3. Otherwise we push and begin a new tracing with
|id == new_event->first_original_event()|
4. Each time an queue was popped we end the tracing with
|id == new_event->first_original_event()|
Example flow of original events:
Case 1 - |ScrollUpdate| + |ScrollUpdate| => |ScrollUpdate|
Original Events - [o1, o2] + [o3, o4] => [o1, o2, o3, o4]
Case 2 - |PinchUpdate| + |ScrollUpdate| =>
|ScrollUpdate| + |PinchUpdate|
Original Events - [o1, o2] + [o3, o4] =>
[] + [o1, o2, o3, o4]
Please see bug for an example tracing picture.
BUG=636332
Review-Url: https://codereview.chromium.org/2695603004
Cr-Commit-Position: refs/heads/master@{#451094}
Committed: https://chromium.googlesource.com/chromium/src/+/b6e0dbdfb8e8aec9b3066ce3e835e21e4915be4a
Patch Set 1 #
Total comments: 1
Patch Set 2 : Trace original events and add unittest #Patch Set 3 : Fix unittest: Add MessageLoop attribuate #
Total comments: 4
Patch Set 4 : Add comments for nestable tracing and |first_original_event()| #
Messages
Total messages: 38 (26 generated)
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Add tracing for compositor event queue
BUG=636332
==========
to
==========
[VSync Queue] Add tracing for event life time and coalesced count
This CL adds tracing for:
1. Construction and destruction of |EventWithCallback|
2. |InputHandlerProxy::HandleInputEventWithLatencyInfo| and
|InputHandlerProxy::DispatchSingleInputEvent|
3. Event type and coalesced count for continues gesture events
BUG=636332
==========
chongz@chromium.org changed reviewers: + tdresser@chromium.org
tdresser@ PTAL, thanks!
tdresser@chromium.org changed reviewers: + oysteine@chromium.org
+oysteine, is there an easy way to write tests for what trace events are recorded? Chong, could you attach a trace taken with these included, and add some details to the description indicating what these trace points are needed for? https://codereview.chromium.org/2695603004/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2695603004/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:346: "input", "InputHandlerProxy::CoalescedContinusGestureEvent", Continuous
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/14 at 16:31:54, tdresser wrote: > +oysteine, is there an easy way to write tests for what trace events are recorded? > There's trace_event_analyzer.h which a few tests use, should be fairly straightforward. A good example is in frame_tree_node_blame_context_unittest.cc. It involves a little bit of boilerplate (could probably be even more simplified), but not too bad.
On 2017/02/14 17:56:57, oystein wrote: > On 2017/02/14 at 16:31:54, tdresser wrote: > > +oysteine, is there an easy way to write tests for what trace events are > recorded? > > > > There's trace_event_analyzer.h which a few tests use, should be fairly > straightforward. > > A good example is in frame_tree_node_blame_context_unittest.cc. It involves a > little bit of boilerplate (could probably be even more simplified), but not too > bad. Chong, this seems like a good opportunity to start adding some basic testing around tracing in this part of the codebase. Could you try adding some tests for this based on frame_tree_node_blame_context_unittest.cc?
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
[VSync Queue] Add tracing for event life time and coalesced count
This CL adds tracing for:
1. Construction and destruction of |EventWithCallback|
2. |InputHandlerProxy::HandleInputEventWithLatencyInfo| and
|InputHandlerProxy::DispatchSingleInputEvent|
3. Event type and coalesced count for continues gesture events
BUG=636332
==========
to
==========
[VSync Queue] Add tracing for event queuing time and coalesced count
The tracing works as follow:
1. For each new event, we first check if it could be coalesced with
existing events or have to be pushed to the end
2. If it could be coalesced, we simply coalesced it
3. Otherwise we push and begin a new tracing with |id ==
new_event->first_original_event()|
4. Each time an queue was popped we end the tracing with |id ==
new_event->first_original_event()|
Note: The follow of original events is
Case 1 - Coalesce
BUG=636332
==========
Description was changed from
==========
[VSync Queue] Add tracing for event queuing time and coalesced count
The tracing works as follow:
1. For each new event, we first check if it could be coalesced with
existing events or have to be pushed to the end
2. If it could be coalesced, we simply coalesced it
3. Otherwise we push and begin a new tracing with |id ==
new_event->first_original_event()|
4. Each time an queue was popped we end the tracing with |id ==
new_event->first_original_event()|
Note: The follow of original events is
Case 1 - Coalesce
BUG=636332
==========
to
==========
[VSync Queue] Add tracing for event queuing time and coalesced count
The tracing works as follow:
1. For each new event, we first check if it could be coalesced with
existing events or have to be pushed to the end
2. If it could be coalesced, we simply coalesced it
3. Otherwise we push and begin a new tracing with |id ==
new_event->first_original_event()|
4. Each time an queue was popped we end the tracing with |id ==
new_event->first_original_event()|
Note: The follow of original events is
Case 1 - |ScrollUpdate| + |ScrollUpdate| => |ScrollUpdate|
Original Events - [o1, o2] + [o3, o4] => [o1, o2, o3, o4]
Case 2 - |PinchUpdate| + |ScrollUpdate| =>
|ScrollUpdate| + |PinchUpdate|
Original Events - [o1, o2] + [o3, o4] =>
[] + [o1, o2, o3, o4]
BUG=636332
==========
Description was changed from
==========
[VSync Queue] Add tracing for event queuing time and coalesced count
The tracing works as follow:
1. For each new event, we first check if it could be coalesced with
existing events or have to be pushed to the end
2. If it could be coalesced, we simply coalesced it
3. Otherwise we push and begin a new tracing with |id ==
new_event->first_original_event()|
4. Each time an queue was popped we end the tracing with |id ==
new_event->first_original_event()|
Note: The follow of original events is
Case 1 - |ScrollUpdate| + |ScrollUpdate| => |ScrollUpdate|
Original Events - [o1, o2] + [o3, o4] => [o1, o2, o3, o4]
Case 2 - |PinchUpdate| + |ScrollUpdate| =>
|ScrollUpdate| + |PinchUpdate|
Original Events - [o1, o2] + [o3, o4] =>
[] + [o1, o2, o3, o4]
BUG=636332
==========
to
==========
[VSync Queue] Add tracing for event queuing time and coalesced count
The tracing works as follow:
1. For each new event, we first check if it could be coalesced with
existing events or have to be pushed to the end
2. If it could be coalesced, we simply coalesced it
3. Otherwise we push and begin a new tracing with
|id == new_event->first_original_event()|
4. Each time an queue was popped we end the tracing with
|id == new_event->first_original_event()|
Note: The follow of original events is
Case 1 - |ScrollUpdate| + |ScrollUpdate| => |ScrollUpdate|
Original Events - [o1, o2] + [o3, o4] => [o1, o2, o3, o4]
Case 2 - |PinchUpdate| + |ScrollUpdate| =>
|ScrollUpdate| + |PinchUpdate|
Original Events - [o1, o2] + [o3, o4] =>
[] + [o1, o2, o3, o4]
BUG=636332
==========
Description was changed from
==========
[VSync Queue] Add tracing for event queuing time and coalesced count
The tracing works as follow:
1. For each new event, we first check if it could be coalesced with
existing events or have to be pushed to the end
2. If it could be coalesced, we simply coalesced it
3. Otherwise we push and begin a new tracing with
|id == new_event->first_original_event()|
4. Each time an queue was popped we end the tracing with
|id == new_event->first_original_event()|
Note: The follow of original events is
Case 1 - |ScrollUpdate| + |ScrollUpdate| => |ScrollUpdate|
Original Events - [o1, o2] + [o3, o4] => [o1, o2, o3, o4]
Case 2 - |PinchUpdate| + |ScrollUpdate| =>
|ScrollUpdate| + |PinchUpdate|
Original Events - [o1, o2] + [o3, o4] =>
[] + [o1, o2, o3, o4]
BUG=636332
==========
to
==========
[VSync Queue] Add tracing for event queuing time and coalesced count
The tracing works as follow:
1. For each new event, we first check if it could be coalesced with
existing events or have to be pushed to the end
2. If it could be coalesced, we simply coalesced it
3. Otherwise we push and begin a new tracing with
|id == new_event->first_original_event()|
4. Each time an queue was popped we end the tracing with
|id == new_event->first_original_event()|
Example flow of original events:
Case 1 - |ScrollUpdate| + |ScrollUpdate| => |ScrollUpdate|
Original Events - [o1, o2] + [o3, o4] => [o1, o2, o3, o4]
Case 2 - |PinchUpdate| + |ScrollUpdate| =>
|ScrollUpdate| + |PinchUpdate|
Original Events - [o1, o2] + [o3, o4] =>
[] + [o1, o2, o3, o4]
Please see bug for an example tracing picture.
BUG=636332
==========
Description was changed from
==========
[VSync Queue] Add tracing for event queuing time and coalesced count
The tracing works as follow:
1. For each new event, we first check if it could be coalesced with
existing events or have to be pushed to the end
2. If it could be coalesced, we simply coalesced it
3. Otherwise we push and begin a new tracing with
|id == new_event->first_original_event()|
4. Each time an queue was popped we end the tracing with
|id == new_event->first_original_event()|
Example flow of original events:
Case 1 - |ScrollUpdate| + |ScrollUpdate| => |ScrollUpdate|
Original Events - [o1, o2] + [o3, o4] => [o1, o2, o3, o4]
Case 2 - |PinchUpdate| + |ScrollUpdate| =>
|ScrollUpdate| + |PinchUpdate|
Original Events - [o1, o2] + [o3, o4] =>
[] + [o1, o2, o3, o4]
Please see bug for an example tracing picture.
BUG=636332
==========
to
==========
[VSync Queue] Add tracing for event queuing time and coalesced count
We need these tracing to understand when the events are push and
popped, and how many events are coalesced together.
The tracing works as follow:
1. For each new event, we first check if it could be coalesced with
existing events or have to be pushed to the end
2. If it could be coalesced, we simply coalesced it
3. Otherwise we push and begin a new tracing with
|id == new_event->first_original_event()|
4. Each time an queue was popped we end the tracing with
|id == new_event->first_original_event()|
Example flow of original events:
Case 1 - |ScrollUpdate| + |ScrollUpdate| => |ScrollUpdate|
Original Events - [o1, o2] + [o3, o4] => [o1, o2, o3, o4]
Case 2 - |PinchUpdate| + |ScrollUpdate| =>
|ScrollUpdate| + |PinchUpdate|
Original Events - [o1, o2] + [o3, o4] =>
[] + [o1, o2, o3, o4]
Please see bug for an example tracing picture.
BUG=636332
==========
tdresser@ I've 1. Rewrote tracing; 2. Updated description; 3. Uploaded a sample tracing picture to crbug; 4. Added unittest. PTAL again, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Awesome, thanks so much for the test! https://codereview.chromium.org/2695603004/diff/40001/ui/events/blink/composi... File ui/events/blink/compositor_thread_event_queue.cc (right): https://codereview.chromium.org/2695603004/diff/40001/ui/events/blink/composi... ui/events/blink/compositor_thread_event_queue.cc:25: TRACE_EVENT_NESTABLE_ASYNC_BEGIN0("input", In what case would these nest? https://codereview.chromium.org/2695603004/diff/40001/ui/events/blink/event_w... File ui/events/blink/event_with_callback.h (right): https://codereview.chromium.org/2695603004/diff/40001/ui/events/blink/event_w... ui/events/blink/event_with_callback.h:58: blink::WebInputEvent* first_original_event() { Can't we just use coalesced_count(), instead of adding a new method?
LGTM, as we covered the comments offline. Can you add a comment about when these would end up nested?
https://codereview.chromium.org/2695603004/diff/40001/ui/events/blink/composi... File ui/events/blink/compositor_thread_event_queue.cc (right): https://codereview.chromium.org/2695603004/diff/40001/ui/events/blink/composi... ui/events/blink/compositor_thread_event_queue.cc:25: TRACE_EVENT_NESTABLE_ASYNC_BEGIN0("input", On 2017/02/16 13:44:18, tdresser wrote: > In what case would these nest? Added comments. // Trace could be nested as there might be multiple events in queue. // e.g. |ScrollUpdate|, |ScrollEnd|, and another scroll sequence. https://codereview.chromium.org/2695603004/diff/40001/ui/events/blink/event_w... File ui/events/blink/event_with_callback.h (right): https://codereview.chromium.org/2695603004/diff/40001/ui/events/blink/event_w... ui/events/blink/event_with_callback.h:58: blink::WebInputEvent* first_original_event() { On 2017/02/16 13:44:18, tdresser wrote: > Can't we just use coalesced_count(), instead of adding a new method? Added comments. // |first_original_event()| is used as ID for tracing.
The CQ bit was checked by chongz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2695603004/#ps60001 (title: "Add comments for nestable tracing and |first_original_event()|")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by chongz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487276914432610,
"parent_rev": "8cf7e79b5bb95b8108861d8d7b910afdbc1db94d", "commit_rev":
"b6e0dbdfb8e8aec9b3066ce3e835e21e4915be4a"}
Message was sent while issue was closed.
Description was changed from
==========
[VSync Queue] Add tracing for event queuing time and coalesced count
We need these tracing to understand when the events are push and
popped, and how many events are coalesced together.
The tracing works as follow:
1. For each new event, we first check if it could be coalesced with
existing events or have to be pushed to the end
2. If it could be coalesced, we simply coalesced it
3. Otherwise we push and begin a new tracing with
|id == new_event->first_original_event()|
4. Each time an queue was popped we end the tracing with
|id == new_event->first_original_event()|
Example flow of original events:
Case 1 - |ScrollUpdate| + |ScrollUpdate| => |ScrollUpdate|
Original Events - [o1, o2] + [o3, o4] => [o1, o2, o3, o4]
Case 2 - |PinchUpdate| + |ScrollUpdate| =>
|ScrollUpdate| + |PinchUpdate|
Original Events - [o1, o2] + [o3, o4] =>
[] + [o1, o2, o3, o4]
Please see bug for an example tracing picture.
BUG=636332
==========
to
==========
[VSync Queue] Add tracing for event queuing time and coalesced count
We need these tracing to understand when the events are push and
popped, and how many events are coalesced together.
The tracing works as follow:
1. For each new event, we first check if it could be coalesced with
existing events or have to be pushed to the end
2. If it could be coalesced, we simply coalesced it
3. Otherwise we push and begin a new tracing with
|id == new_event->first_original_event()|
4. Each time an queue was popped we end the tracing with
|id == new_event->first_original_event()|
Example flow of original events:
Case 1 - |ScrollUpdate| + |ScrollUpdate| => |ScrollUpdate|
Original Events - [o1, o2] + [o3, o4] => [o1, o2, o3, o4]
Case 2 - |PinchUpdate| + |ScrollUpdate| =>
|ScrollUpdate| + |PinchUpdate|
Original Events - [o1, o2] + [o3, o4] =>
[] + [o1, o2, o3, o4]
Please see bug for an example tracing picture.
BUG=636332
Review-Url: https://codereview.chromium.org/2695603004
Cr-Commit-Position: refs/heads/master@{#451094}
Committed:
https://chromium.googlesource.com/chromium/src/+/b6e0dbdfb8e8aec9b3066ce3e835...
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/b6e0dbdfb8e8aec9b3066ce3e835... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
