Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
764403002:
Expand UMA coverage for compositor-handled events and fling FPS (Closed)
|
Created:
6 years ago by jdduke (slow) Modified:
6 years ago CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, jdduke+watch_chromium.org, jam, mlamouri+watch-content_chromium.org, Rick Byers, tdresser Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionExpand UMA coverage for compositor-handled events and fling FPS
Include explicit compositor-thread UMA latency coverage for scroll
begin, as well as pinch and fling-related events. Also add an FPS metric
for both main and compositor-thread driven flings.
BUG=437310, 436965
Committed: https://crrev.com/0ead6d08a9974cfdabfe40af7501441154685076
Cr-Commit-Position: refs/heads/master@{#307096}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Update histograms.xml #
Total comments: 3
Patch Set 3 : Naming fix for histograms.xml #Patch Set 4 : Remove GestureScroll #
Total comments: 6
Patch Set 5 : Better description #
Total comments: 4
Patch Set 6 : Tweak phrasing #
Total comments: 5
Patch Set 7 : Fix build #
Total comments: 3
Messages
Total messages: 28 (7 generated)
jdduke@chromium.org changed reviewers: + miletus@chromium.org
miletus@: PTAL, thanks. rbyers@: Turns out we can get fling FPS UMA coverage for the main thread without too much trouble. https://codereview.chromium.org/764403002/diff/1/content/child/web_gesture_cu... File content/child/web_gesture_curve_impl.cc (right): https://codereview.chromium.org/764403002/diff/1/content/child/web_gesture_cu... content/child/web_gesture_curve_impl.cc:88: 1, 200, 100); Any thoughts on bucket extents/count? https://codereview.chromium.org/764403002/diff/1/content/child/web_gesture_cu... content/child/web_gesture_curve_impl.cc:92: "Event.FPS.RendererImpl.Fling", Also not sure about the preferred naming scheme...
Note you also need to add description for the new UMA data like https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist... https://codereview.chromium.org/764403002/diff/1/content/child/web_gesture_cu... File content/child/web_gesture_curve_impl.cc (right): https://codereview.chromium.org/764403002/diff/1/content/child/web_gesture_cu... content/child/web_gesture_curve_impl.cc:88: 1, 200, 100); On 2014/12/01 18:20:59, jdduke wrote: > Any thoughts on bucket extents/count? You can try a few test flings on your device and check its values at about://histograms to see what the value range looks like. https://codereview.chromium.org/764403002/diff/1/content/child/web_gesture_cu... content/child/web_gesture_curve_impl.cc:113: // Suppress recording of such redundant animate calls, avoiding artificially If this is true, do you want to also guard against the case there comes repeated apply() at the same first_animate_time ? e.g. the apply happens at 10, 10, 26 ?
rbyers@chromium.org changed reviewers: + rbyers@chromium.org
Great, glad to see this expanding. /cc tdresser since he's spent the most time staring at our input UMA data. https://codereview.chromium.org/764403002/diff/1/content/child/web_gesture_cu... File content/child/web_gesture_curve_impl.cc (right): https://codereview.chromium.org/764403002/diff/1/content/child/web_gesture_cu... content/child/web_gesture_curve_impl.cc:92: "Event.FPS.RendererImpl.Fling", On 2014/12/01 18:20:59, jdduke wrote: > Also not sure about the preferred naming scheme... We have Event.Latency.* for all latency measures. Maybe we should have Event.Frequency.* for anything where we want to monitor how frequently events are being serviced / produced. "FPS" is almost that except "frames" isn't necessarily the right concept. Eg. imagine we want to monitor stylus performance to see how often we're hitting 120 or 200hz event handling rate.
jdduke@chromium.org changed reviewers: + mpearson@chromium.org, sievers@chromium.org
Additional owners: sievers@: content/child mpearson@: tools/metrics/histograms https://codereview.chromium.org/764403002/diff/1/content/child/web_gesture_cu... File content/child/web_gesture_curve_impl.cc (right): https://codereview.chromium.org/764403002/diff/1/content/child/web_gesture_cu... content/child/web_gesture_curve_impl.cc:88: 1, 200, 100); On 2014/12/01 18:57:07, Yufeng Shen wrote: > On 2014/12/01 18:20:59, jdduke wrote: > > Any thoughts on bucket extents/count? > > You can try a few test flings on your device and check its values at > about://histograms to see what the value range looks like. Well, it *should* be capped by the refresh rate, so on most Android devices a max of 60. We definitely want to include 120hz displays, hence the 200 cap, but I wonder if we should consider 240hz displays? https://codereview.chromium.org/764403002/diff/1/content/child/web_gesture_cu... content/child/web_gesture_curve_impl.cc:92: "Event.FPS.RendererImpl.Fling", On 2014/12/01 19:47:10, Rick Byers wrote: > On 2014/12/01 18:20:59, jdduke wrote: > > Also not sure about the preferred naming scheme... > > We have Event.Latency.* for all latency measures. Maybe we should have > Event.Frequency.* for anything where we want to monitor how frequently events > are being serviced / produced. "FPS" is almost that except "frames" isn't > necessarily the right concept. Eg. imagine we want to monitor stylus > performance to see how often we're hitting 120 or 200hz event handling rate. Yeah, frequency is a nice name for this family. https://codereview.chromium.org/764403002/diff/1/content/child/web_gesture_cu... content/child/web_gesture_curve_impl.cc:113: // Suppress recording of such redundant animate calls, avoiding artificially On 2014/12/01 18:57:07, Yufeng Shen wrote: > If this is true, do you want to also guard against the case there comes repeated > apply() at the same first_animate_time ? e.g. the apply happens at 10, 10, 26 ? Good catch, done. https://codereview.chromium.org/764403002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/764403002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7064: +<histogram name="Event.Latency.RendererImpl" units="microseconds"> Does this conflict with the Event.Latency.RendererImpl.GestureScroll declaration below?
https://codereview.chromium.org/764403002/diff/1/content/child/web_gesture_cu... File content/child/web_gesture_curve_impl.cc (right): https://codereview.chromium.org/764403002/diff/1/content/child/web_gesture_cu... content/child/web_gesture_curve_impl.cc:88: 1, 200, 100); On 2014/12/01 20:20:28, jdduke wrote: > On 2014/12/01 18:57:07, Yufeng Shen wrote: > > On 2014/12/01 18:20:59, jdduke wrote: > > > Any thoughts on bucket extents/count? > > > > You can try a few test flings on your device and check its values at > > about://histograms to see what the value range looks like. > > Well, it *should* be capped by the refresh rate, so on most Android devices a > max of 60. We definitely want to include 120hz displays, hence the 200 cap, but > I wonder if we should consider 240hz displays? Do we push out frames as whatever the display refresh rate is ? or do we have an internal cap ? https://codereview.chromium.org/764403002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/764403002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7064: +<histogram name="Event.Latency.RendererImpl" units="microseconds"> On 2014/12/01 20:20:29, jdduke wrote: > Does this conflict with the Event.Latency.RendererImpl.GestureScroll declaration > below? I think we should just drop Event.Latency.RendererImpl.GestureScroll since we have stopped reporting it for quite a long time. Also you might want to add the description for individual metric, e.g. Event.Latency.RendererImpl.GestureScrollBegin ...
https://codereview.chromium.org/764403002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/764403002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7064: +<histogram name="Event.Latency.RendererImpl" units="microseconds"> On 2014/12/01 20:40:14, Yufeng Shen wrote: > On 2014/12/01 20:20:29, jdduke wrote: > > Does this conflict with the Event.Latency.RendererImpl.GestureScroll > declaration > > below? > > I think we should just drop Event.Latency.RendererImpl.GestureScroll since we > have stopped reporting it for quite a long time. > Done. > Also you might want to add the description for individual metric, e.g. > Event.Latency.RendererImpl.GestureScrollBegin ... Hmm, we don't do that for Event.Latency.Renderer, I wonder if I could reuse the RendererEventLatency suffix? Hopefully that plays nice with the GestureScroll2 suffix...
https://codereview.chromium.org/764403002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/764403002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7048: -<histogram name="Event.Latency.RendererImpl.GestureScroll" units="microseconds"> Please keep this old histogram name + description + obsolete message, even though you're adding a new histogram that is a .Something extension of it. https://codereview.chromium.org/764403002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/764403002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:6707: + Average animation frequency of a main-thread fling instance over its A histogram description should say when an entry is emitted. If indeed this histogram emits something that's an average of something else, the description needs to clearly state how it decides how many items to average / over what period. In this case, I think you need to describe how a lifetime is determined. It would also be nice if the description mentioned renderer, rather than merely finding it in the histogram name. Finally, I vaguely recall that sometimes renderers are killed (in order to be shut down faster) rather than going through the normal close process. Make sure your answer to the "when is this emitted" either implicitly or explicitly says whether this will be emitted under those conditions. ditto below https://codereview.chromium.org/764403002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7069: + when the event reaches Chrome, whereas on other platforms we use the to be clear: other platforms includes (iOS, Android, Mac, Linux, and ChromeOS) i.e., all those platforms include events with a timestamp you get from the kernel?
https://codereview.chromium.org/764403002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/764403002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7048: -<histogram name="Event.Latency.RendererImpl.GestureScroll" units="microseconds"> On 2014/12/01 21:27:36, Mark P wrote: > Please keep this old histogram name + description + obsolete message, even > though you're adding a new histogram that is a .Something extension of it. Done. https://codereview.chromium.org/764403002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/764403002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:6707: + Average animation frequency of a main-thread fling instance over its On 2014/12/01 21:27:36, Mark P wrote: > A histogram description should say when an entry is emitted. > Done. > If indeed this histogram emits something that's an average of something else, > the description needs to clearly state how it decides how many items to average > / over what period. In this case, I think you need to describe how a lifetime > is determined. > Done. > It would also be nice if the description mentioned renderer, rather than merely > finding it in the histogram name. > > Finally, I vaguely recall that sometimes renderers are killed (in order to be > shut down faster) rather than going through the normal close process. Make sure > your answer to the "when is this emitted" either implicitly or explicitly says > whether this will be emitted under those conditions. > Done. > ditto below https://codereview.chromium.org/764403002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7069: + when the event reaches Chrome, whereas on other platforms we use the On 2014/12/01 21:27:36, Mark P wrote: > to be clear: > other platforms includes (iOS, Android, Mac, Linux, and ChromeOS) > i.e., all those platforms include events with a timestamp you get from the > kernel? Yes.
https://codereview.chromium.org/764403002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/764403002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:6707: + Emitted after a renderer process, main-thread fling terminates, for any nit: omit first comma ditto other histogram below https://codereview.chromium.org/764403002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:6710: + animation ticks divided by the fling animation duration. If I start a fling, keep my finger on the object I'm flinging, out to ten, then finish my fling, this number will be very low, right? (We don't need to animate the object if I'm not moving it.) If so, I think you should explain mention this, otherwise a low value might be initially interpreted as bad animation / jank (too few animations per second). If not, I think you should explain why this pause doesn't affect the animation total (and hence the result is a good measure of jank). ditto other histogram below
Patchset #6 (id:100001) has been deleted
https://codereview.chromium.org/764403002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/764403002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:6707: + Emitted after a renderer process, main-thread fling terminates, for any On 2014/12/01 23:13:01, Mark P wrote: > nit: omit first comma > > ditto other histogram below Done. https://codereview.chromium.org/764403002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:6710: + animation ticks divided by the fling animation duration. On 2014/12/01 23:13:01, Mark P wrote: > If I start a fling, keep my finger on the object I'm flinging, out to ten, then > finish my fling, this number will be very low, right? (We don't need to animate > the object if I'm not moving it.) > A fling animation by definition occurs only after you've lifted your finger. I guess the terminology may not be self-evident, but scrolling is the motion induced by physical contact, and the fling is the virtual motion induced by the velocity at the time of contact release. When the fling animation starts, it will tick at the rate of the animation subsystem (independent of its velocity). > If so, I think you should explain mention this, otherwise a low value might be > initially interpreted as bad animation / jank (too few animations per second). > > If not, I think you should explain why this pause doesn't affect the animation > total (and hence the result is a good measure of jank). > > ditto other histogram below I tweaked the phrasing to indicating that we're animating a fling curve.
tdresser@chromium.org changed reviewers: + tdresser@chromium.org
High level approach LGTM. https://codereview.chromium.org/764403002/diff/120001/content/child/web_gestu... File content/child/web_gesture_curve_impl.cc (right): https://codereview.chromium.org/764403002/diff/120001/content/child/web_gestu... content/child/web_gesture_curve_impl.cc:86: "Event.Frequency.Renderer.FlingAnimate", It might be worth factoring out the histogram name, so you can reuse the rest of the UMA_HISTOGRAM_CUSTOM_COUNTS call. https://codereview.chromium.org/764403002/diff/120001/content/renderer/input/... File content/renderer/input/input_handler_proxy.cc (right): https://codereview.chromium.org/764403002/diff/120001/content/renderer/input/... content/renderer/input/input_handler_proxy.cc:140: "Event.Latency.RendererImpl.GestureScrollBegin", I think factoring out the histogram name here would improve readability. https://codereview.chromium.org/764403002/diff/120001/content/renderer/input/... content/renderer/input/input_handler_proxy.cc:190: base::TimeTicks::IsHighResNowFastAndReliable()) { Do you know if other metrics report low res data, or if they suppress reporting if high res data isn't available? Not reporting anything is probably more correct. We should try to unify this at some point.
https://codereview.chromium.org/764403002/diff/120001/content/renderer/input/... File content/renderer/input/input_handler_proxy.cc (right): https://codereview.chromium.org/764403002/diff/120001/content/renderer/input/... content/renderer/input/input_handler_proxy.cc:190: base::TimeTicks::IsHighResNowFastAndReliable()) { On 2014/12/02 14:42:27, tdresser wrote: > Do you know if other metrics report low res data, or if they suppress reporting > if high res data isn't available? > > Not reporting anything is probably more correct. We should try to unify this at > some point. Good question, I'd have to defer to Yufeng. What is curious is that the histogram description for GestureScroll2 mentions |IsHighResNowFastAndReliable()|, yet we weren't actually making that query before when logging the metric. As long as we're consistent between main/impl threads I don't have a strong preference.
miletus@google.com changed reviewers: + miletus@google.com
https://codereview.chromium.org/764403002/diff/120001/content/renderer/input/... File content/renderer/input/input_handler_proxy.cc (right): https://codereview.chromium.org/764403002/diff/120001/content/renderer/input/... content/renderer/input/input_handler_proxy.cc:190: base::TimeTicks::IsHighResNowFastAndReliable()) { On 2014/12/02 20:04:56, jdduke wrote: > On 2014/12/02 14:42:27, tdresser wrote: > > Do you know if other metrics report low res data, or if they suppress > reporting > > if high res data isn't available? > > > > Not reporting anything is probably more correct. We should try to unify this > at > > some point. > > Good question, I'd have to defer to Yufeng. What is curious is that the > histogram description for GestureScroll2 mentions > |IsHighResNowFastAndReliable()|, yet we weren't actually making that query > before when logging the metric. As long as we're consistent between main/impl > threads I don't have a strong preference. I don't remember GestureScroll2 is conditioned on |IsHighResNowFastAndReliable()|. I think it might just a copy over from Event.Latency.Renderer2 's description incorrectly.
histograms.xml lgtm
sievers@: Anything else you'd like to see from content/child?
lgtm https://codereview.chromium.org/764403002/diff/140001/content/renderer/input/... File content/renderer/input/input_handler_proxy.cc (right): https://codereview.chromium.org/764403002/diff/140001/content/renderer/input/... content/renderer/input/input_handler_proxy.cc:119: const ui::LatencyInfo& latency_info) { DCHECK(uma_latency_reporting_enabled_);
https://codereview.chromium.org/764403002/diff/140001/content/renderer/input/... File content/renderer/input/input_handler_proxy.cc (right): https://codereview.chromium.org/764403002/diff/140001/content/renderer/input/... content/renderer/input/input_handler_proxy.cc:119: const ui::LatencyInfo& latency_info) { On 2014/12/05 21:27:00, sievers wrote: > DCHECK(uma_latency_reporting_enabled_); Hmm, this isn't a member function, though I could make it one if you prefer?
https://codereview.chromium.org/764403002/diff/140001/content/renderer/input/... File content/renderer/input/input_handler_proxy.cc (right): https://codereview.chromium.org/764403002/diff/140001/content/renderer/input/... content/renderer/input/input_handler_proxy.cc:119: const ui::LatencyInfo& latency_info) { On 2014/12/05 21:31:53, jdduke wrote: > On 2014/12/05 21:27:00, sievers wrote: > > DCHECK(uma_latency_reporting_enabled_); > > Hmm, this isn't a member function, though I could make it one if you prefer? Oops nevermind, no big deal.
The CQ bit was checked by jdduke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/764403002/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0ead6d08a9974cfdabfe40af7501441154685076 Cr-Commit-Position: refs/heads/master@{#307096} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
