Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1907803002:
Add metric to determine the benefit of forcing an event listener to be passive. (Closed)
|
Created:
4 years, 8 months ago by dtapuska Modified:
4 years, 8 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, dtapuska+chromiumwatch_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master_touch_scroll_intervention_uma Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd metric to determine the benefit of forcing an event listener to be passive.
Record the duration of processing an event when its dispatchType
was set to be forced non-blocking.
BUG=604828
Committed: https://crrev.com/cb268f1f0043f3426ba9f35282f50c2f16e3c185
Cr-Commit-Position: refs/heads/master@{#389163}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Pull metric into check for high resolution #
Total comments: 3
Depends on Patchset: Messages
Total messages: 23 (8 generated)
Description was changed from ========== Add metric to determine the benefit of forcing an event listener to be passive. Record the duration of processing an event when its dispatchType was set to be forced non-blocking. BUG=604828 ========== to ========== Add metric to determine the benefit of forcing an event listener to be passive. Record the duration of processing an event when its dispatchType was set to be forced non-blocking. BUG=604828 ==========
dtapuska@chromium.org changed reviewers: + lanwei@chromium.org, tdresser@chromium.org
On 2016/04/21 01:31:38, dtapuska wrote: > mailto:dtapuska@chromium.org changed reviewers: > + mailto:lanwei@chromium.org, mailto:tdresser@chromium.org I'm not sure if this is what you want. But the histogram would be computed inside the content layer so the forced behaviour is intended to be set by the compositor.
https://codereview.chromium.org/1907803002/diff/1/content/renderer/input/rend... File content/renderer/input/render_widget_input_handler.cc (right): https://codereview.chromium.org/1907803002/diff/1/content/renderer/input/rend... content/renderer/input/render_widget_input_handler.cc:170: base::TimeTicks now = base::TimeTicks::Now(); Are you intentionally only recording the first one if it's high resolution, but always recording this metric? Also, we can't have both Event.PassiveListeners.Latency and Event.PassiveListeners.ForcedNonBlockingLatency, can we? Maybe move this to an else if, or add a DCHECK or something?
https://codereview.chromium.org/1907803002/diff/1/content/renderer/input/rend... File content/renderer/input/render_widget_input_handler.cc (right): https://codereview.chromium.org/1907803002/diff/1/content/renderer/input/rend... content/renderer/input/render_widget_input_handler.cc:170: base::TimeTicks now = base::TimeTicks::Now(); On 2016/04/21 13:36:42, tdresser wrote: > Are you intentionally only recording the first one if it's high resolution, but > always recording this metric? > > Also, we can't have both Event.PassiveListeners.Latency and > Event.PassiveListeners.ForcedNonBlockingLatency, can we? > > Maybe move this to an else if, or add a DCHECK or something? Err no; I meant to move this into a if(highRes) { ... }
https://codereview.chromium.org/1907803002/diff/1/content/renderer/input/rend... File content/renderer/input/render_widget_input_handler.cc (right): https://codereview.chromium.org/1907803002/diff/1/content/renderer/input/rend... content/renderer/input/render_widget_input_handler.cc:170: base::TimeTicks now = base::TimeTicks::Now(); On 2016/04/21 13:36:42, tdresser wrote: > Are you intentionally only recording the first one if it's high resolution, but > always recording this metric? > > Also, we can't have both Event.PassiveListeners.Latency and > Event.PassiveListeners.ForcedNonBlockingLatency, can we? > > Maybe move this to an else if, or add a DCHECK or something? Done.
LGTM https://codereview.chromium.org/1907803002/diff/20001/content/renderer/input/... File content/renderer/input/render_widget_input_handler.cc (right): https://codereview.chromium.org/1907803002/diff/20001/content/renderer/input/... content/renderer/input/render_widget_input_handler.cc:163: base::TimeTicks now = base::TimeTicks::Now(); Move computation of now outside of conditional.
https://codereview.chromium.org/1907803002/diff/20001/content/renderer/input/... File content/renderer/input/render_widget_input_handler.cc (right): https://codereview.chromium.org/1907803002/diff/20001/content/renderer/input/... content/renderer/input/render_widget_input_handler.cc:163: base::TimeTicks now = base::TimeTicks::Now(); On 2016/04/21 20:54:52, tdresser wrote: > Move computation of now outside of conditional. this is an if, else if.. since it avoids a costly call getting the current time; I prefer calling it in each block separately so we avoid it most of the time (the else case).
dtapuska@chromium.org changed reviewers: + isherman@chromium.org
https://codereview.chromium.org/1907803002/diff/20001/content/renderer/input/... File content/renderer/input/render_widget_input_handler.cc (right): https://codereview.chromium.org/1907803002/diff/20001/content/renderer/input/... content/renderer/input/render_widget_input_handler.cc:163: base::TimeTicks now = base::TimeTicks::Now(); On 2016/04/21 21:02:41, dtapuska wrote: > On 2016/04/21 20:54:52, tdresser wrote: > > Move computation of now outside of conditional. > > this is an if, else if.. since it avoids a costly call getting the current time; > I prefer calling it in each block separately so we avoid it most of the time > (the else case). Sorry, I was thinking this was an "if/else" not "if / else if". You're right.
LGTM, thanks!
metrics lgtm
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1907803002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1907803002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1907803002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1907803002/20001
Message was sent while issue was closed.
Description was changed from ========== Add metric to determine the benefit of forcing an event listener to be passive. Record the duration of processing an event when its dispatchType was set to be forced non-blocking. BUG=604828 ========== to ========== Add metric to determine the benefit of forcing an event listener to be passive. Record the duration of processing an event when its dispatchType was set to be forced non-blocking. BUG=604828 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add metric to determine the benefit of forcing an event listener to be passive. Record the duration of processing an event when its dispatchType was set to be forced non-blocking. BUG=604828 ========== to ========== Add metric to determine the benefit of forcing an event listener to be passive. Record the duration of processing an event when its dispatchType was set to be forced non-blocking. BUG=604828 Committed: https://crrev.com/cb268f1f0043f3426ba9f35282f50c2f16e3c185 Cr-Commit-Position: refs/heads/master@{#389163} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/cb268f1f0043f3426ba9f35282f50c2f16e3c185 Cr-Commit-Position: refs/heads/master@{#389163} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
