Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2636583002:
UMA metrics for fractions of wheel and touch scrolls blocked on the main thread. (Closed)
DescriptionUMA metrics for fractions of wheel and touch scrolls blocked on the main thread.
BUG=626662, 626663
TEST=InputHandlerProxyTest.GestureScrollingThreadStatusHistograms,
InputHandlerProxyTest.WheelScrollingThreadStatusHistograms
Review-Url: https://codereview.chromium.org/2636583002
Cr-Commit-Position: refs/heads/master@{#445551}
Committed: https://chromium.googlesource.com/chromium/src/+/24b30bfd5ac15c60a29477638c91ab0fb8159055
Patch Set 1 #
Total comments: 13
Patch Set 2 : RecordScrollingThreadStatus called in FlingScrollByMouseWheel. #
Total comments: 2
Patch Set 3 : Histogram names used directly instead of being stored in a const char* #
Messages
Total messages: 29 (15 generated)
sahel@chromium.org changed reviewers: + tdresser@chromium.org
Please review this WIP cl: I couldn't come up with better names for the enum and histograms please let me know if you have any suggestions. Here are some clarification questions, it would be great if you could help me to address them: 1- For touch scroll I record the touch start result and then in handeGestureScrollBegin I call the recording function and use the stored result. On the other hand for wheel scroll, there is no event equivalent to touch start and I store the result in HandleMouseWheel instead. Is this ok? 2- To record the metrics, I use touch_start_result_ which gets updated in HandleTouchStart. DID_NOT_HANDLE_NON_BLOCKING_DUE_TO_FLING is never stored in the touch_start_result_. When HandleTouchStart returns this value, touch_start_result_ will be DID_NOT_HANDLED. The latter value is used for the following touch move events and that's why I record this situation as blocked on main, is it correct? 3- When latching is disabled, to handle touchpad fling, SrollBegin is called for every synthetic wheel, without calling HandleMouseWheel for the synthetic event. Should I record the metrics for this case as well? Right now I don't because mouse_wheel_result_ will be stall. It will have the value for the last actual HandleMouseWheel call. I can either use this stall value, or call handleMouseWheel for the synthetic wheel events. For the latter solution I have to pass a synthetic flag to make sure that current fling won't get cancelled.
On 2017/01/13 21:07:28, sahel wrote: > Please review this WIP cl: > > I couldn't come up with better names for the enum and histograms please let me > know if you have any suggestions. > > Here are some clarification questions, it would be great if you could help me to > address them: > > 1- For touch scroll I record the touch start result and then in > handeGestureScrollBegin I call the recording function and use the stored result. > On the other hand for wheel scroll, there is no event equivalent to touch start > and I store the result in HandleMouseWheel instead. Is this ok? > > 2- To record the metrics, I use touch_start_result_ which gets updated in > HandleTouchStart. DID_NOT_HANDLE_NON_BLOCKING_DUE_TO_FLING is never stored in > the touch_start_result_. When HandleTouchStart returns this value, > touch_start_result_ will be DID_NOT_HANDLED. The latter value is used for the > following touch move events and that's why I record this situation as blocked on > main, is it correct? > > 3- When latching is disabled, to handle touchpad fling, SrollBegin is called for > every synthetic wheel, without calling HandleMouseWheel for the synthetic event. > Should I record the metrics for this case as well? Right now I don't because > mouse_wheel_result_ will be stall. It will have the value for the last actual > HandleMouseWheel call. > > I can either use this stall value, or call handleMouseWheel for the synthetic > wheel events. For the latter solution I have to pass a synthetic flag to make > sure that current fling won't get cancelled. 1) I think that is reasonable 2) I wonder if touch_start_result should be updated due to fling. Seems like an oddity to me. That if we returned it the touchstart due to fling probably should reject any touch moves as well. 3) I think the the Touchpad fling case all events are handled by the compositor because it is already checked otherwise the fling would have moved to the main thread. But we may want to record a compositor scroll reason just so that wherever we record main thread scroll reason we also record these histograms.
dtapuska@chromium.org changed reviewers: + dtapuska@chromium.org
https://codereview.chromium.org/2636583002/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2636583002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:599: static const char* kGestureHistogramName = why do we need static variables for these? https://codereview.chromium.org/2636583002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:600: "Renderer4.GestureScrollingThreadStatus"; I think the names are a little funky but I see that is taken from the main thread scrolling reason. https://codereview.chromium.org/2636583002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:635: uint32_t scrolling_thread_status_enum_max = This can be const... and the name like kScrolingThreadStatusEnunMax or something.
The CQ bit was checked by sahel@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.
On 2017/01/13 21:57:58, dtapuska wrote: > https://codereview.chromium.org/2636583002/diff/1/ui/events/blink/input_handl... > File ui/events/blink/input_handler_proxy.cc (right): > > https://codereview.chromium.org/2636583002/diff/1/ui/events/blink/input_handl... > ui/events/blink/input_handler_proxy.cc:599: static const char* > kGestureHistogramName = > why do we need static variables for these? > > https://codereview.chromium.org/2636583002/diff/1/ui/events/blink/input_handl... > ui/events/blink/input_handler_proxy.cc:600: > "Renderer4.GestureScrollingThreadStatus"; > I think the names are a little funky but I see that is taken from the main > thread scrolling reason. > > https://codereview.chromium.org/2636583002/diff/1/ui/events/blink/input_handl... > ui/events/blink/input_handler_proxy.cc:635: uint32_t > scrolling_thread_status_enum_max = > This can be const... and the name like kScrolingThreadStatusEnunMax or > something. 3-done
https://codereview.chromium.org/2636583002/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2636583002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:599: static const char* kGestureHistogramName = On 2017/01/13 21:57:58, dtapuska wrote: > why do we need static variables for these? So that we don't need to initialize the values for each function call. I think the initialization overhead is negligible and I can get rid of static if you prefer. However keeping it is more similar to RecordMainThreadScrollingReasons function. https://codereview.chromium.org/2636583002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:600: "Renderer4.GestureScrollingThreadStatus"; On 2017/01/13 21:57:58, dtapuska wrote: > I think the names are a little funky but I see that is taken from the main > thread scrolling reason. Yes, I tried to be consistent with RecordMainThreadScrollingReasons but I don't particularly like the histogram names, either. Specially because the suffices start with G and W and the two histograms won't be next to each other in histograms.xml file. https://codereview.chromium.org/2636583002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:635: uint32_t scrolling_thread_status_enum_max = On 2017/01/13 21:57:58, dtapuska wrote: > This can be const... and the name like kScrolingThreadStatusEnunMax or > something. Done.
LGTM with nits. https://codereview.chromium.org/2636583002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2636583002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:52657: + on the compositor thread, or on the copositor thread but blocked on the main copositor -> compositor https://codereview.chromium.org/2636583002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:52658: + thread. The last case will happen when the TouchStartOrMove event listener This is a bit overly detailed. Perhaps just "The last case will happen when there is a blocking event listener". https://codereview.chromium.org/2636583002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:53068: + listener property is blocking. Same as above. https://codereview.chromium.org/2636583002/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy.h (right): https://codereview.chromium.org/2636583002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.h:249: int32_t mouse_wheel_result_; Comment. https://codereview.chromium.org/2636583002/diff/20001/ui/events/blink/input_h... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2636583002/diff/20001/ui/events/blink/input_h... ui/events/blink/input_handler_proxy.cc:639: UMA_HISTOGRAM_ENUMERATION(kGestureHistogramName, scrolling_thread_status, I'd probably just put the literal string in here, instead of using a constant. This is fine - it's just a bit more difficult to grep for where the histogram is recorded.
The CQ bit was checked by sahel@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.
I will file a separate bug for updating touch_start_result_ with DID_NOT_HANDLED_NON_BLOCKING_DUE_TO_FLING since it's not related to this patch. https://codereview.chromium.org/2636583002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2636583002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:52657: + on the compositor thread, or on the copositor thread but blocked on the main On 2017/01/17 20:27:43, tdresser wrote: > copositor -> compositor Done. https://codereview.chromium.org/2636583002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:53068: + listener property is blocking. On 2017/01/17 20:27:43, tdresser wrote: > Same as above. Done. https://codereview.chromium.org/2636583002/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy.h (right): https://codereview.chromium.org/2636583002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.h:249: int32_t mouse_wheel_result_; On 2017/01/17 20:27:43, tdresser wrote: > Comment. Done. https://codereview.chromium.org/2636583002/diff/20001/ui/events/blink/input_h... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2636583002/diff/20001/ui/events/blink/input_h... ui/events/blink/input_handler_proxy.cc:639: UMA_HISTOGRAM_ENUMERATION(kGestureHistogramName, scrolling_thread_status, On 2017/01/17 20:27:43, tdresser wrote: > I'd probably just put the literal string in here, instead of using a constant. > > This is fine - it's just a bit more difficult to grep for where the histogram is > recorded. Done.
sahel@chromium.org changed reviewers: + holte@chromium.org
histograms lgtm
On 2017/01/20 21:12:45, Steven Holte wrote: > histograms lgtm lgtm
The CQ bit was checked by sahel@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/2636583002/#ps40001 (title: "Histogram names used directly instead of being stored in a const char*")
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": 40001, "attempt_start_ts": 1485211764833710,
"parent_rev": "062a42c6769d9d6f6a3ce699c0acc758636f924e", "commit_rev":
"24b30bfd5ac15c60a29477638c91ab0fb8159055"}
Message was sent while issue was closed.
Description was changed from
==========
UMA metrics for fractions of wheel and touch scrolls blocked on the main thread.
BUG=626662, 626663
TEST=InputHandlerProxyTest.GestureScrollingThreadStatusHistograms,
InputHandlerProxyTest.WheelScrollingThreadStatusHistograms
==========
to
==========
UMA metrics for fractions of wheel and touch scrolls blocked on the main thread.
BUG=626662, 626663
TEST=InputHandlerProxyTest.GestureScrollingThreadStatusHistograms,
InputHandlerProxyTest.WheelScrollingThreadStatusHistograms
Review-Url: https://codereview.chromium.org/2636583002
Cr-Commit-Position: refs/heads/master@{#445551}
Committed:
https://chromium.googlesource.com/chromium/src/+/24b30bfd5ac15c60a29477638c91...
==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/24b30bfd5ac15c60a29477638c91...
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 445551 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
On 2017/01/24 02:07:20, findit-for-me wrote: > FYI: Findit identified this CL at revision 445551 as the culprit for > failures in the build cycles as shown on: > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... I tried to revert this patch, but it is too large to automatically revert. Please fix the tests or revert ASAP. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
