Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
577273003:
Add new latency_info for tracking browser composite time. (Closed)
|
Created:
6 years, 3 months ago by orglofch Modified:
6 years, 2 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, tdresser+watch_chromium.org, jam, penghuang+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, cc-bugs_chromium.org, James Su, Sami Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd new latency_info for tracking browser composite time.
RenderWidgetHostImpl maintains a rolling estimate of composite times
to be passed to RenderWidgetHostViewAndroid::SendBeginFrame to
improve deadline estimate.
Latency info is still currently tied to input events. Future patch
should make the latency_info update on a per frame basis.
BUG=414365
Committed: https://crrev.com/545a72d3f37fa15844cb08d83db7cca672b79cd0
Cr-Commit-Position: refs/heads/master@{#297950}
Patch Set 1 #Patch Set 2 : rm useless comment #
Total comments: 12
Patch Set 3 : Rename and add id's to components #
Total comments: 4
Patch Set 4 : add estimate slack #Patch Set 5 : made slack constant #
Total comments: 9
Patch Set 6 : Tuned kTypicalMaxComponentsPerLatencyInfo #Patch Set 7 : Sievers Review #
Messages
Total messages: 32 (11 generated)
Patchset #2 (id:20001) has been deleted
orglofch@chromium.org changed reviewers: + brianderson@chromium.org, jbauman@chromium.org, miletus@google.com, sievers@chromium.org
orglofch@chromium.org changed reviewers: + yufeng@chromium.org - miletus@google.com
PTAL
https://codereview.chromium.org/577273003/diff/40001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/577273003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.cc:1460: ui::INPUT_EVENT_BROWSER_COMPOSITE_COMPONENT, 0, 0); If the Browser is swap or commit throttled, there may be a delay from when the Renderer's swap is received to when it is actually consumed by the Browser. For deadline estimation purposes, we care about the timing from when the swap is consumed, not from when it is received. Not sure where the best place to add an INPUT_EVENT_RENDERER_SWAP_CONSUMED_COMPONENT is though... The INPUT_EVENT_BROWSER_COMPOSITE_COMPONENT component would still be useful though for test/debug purposes, so it might be nice to keep it here. Maybe call it INPUT_EVENT_RENDERER_SWAP_RECEIVED_COMPONENT though? https://codereview.chromium.org/577273003/diff/40001/ui/events/latency_info.h File ui/events/latency_info.h (right): https://codereview.chromium.org/577273003/diff/40001/ui/events/latency_info.h... ui/events/latency_info.h:59: // Timestamp of when an unparented output surface began swap buffers "an uparented output surface" -> "the Browser process" https://codereview.chromium.org/577273003/diff/40001/ui/events/latency_info.h... ui/events/latency_info.h:60: INPUT_EVENT_OUTPUT_SURFACE_SWAP_BUFFER_COMPONENT, INPUT_EVENT_BROWSER_SWAP_BUFFER_COMPONENT?
https://codereview.chromium.org/577273003/diff/40001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/577273003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.cc:1460: ui::INPUT_EVENT_BROWSER_COMPOSITE_COMPONENT, 0, 0); This should specify GetLatencyComponentId() as the component ID. https://codereview.chromium.org/577273003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.cc:2162: if (latency_info.FindLatency(ui::INPUT_EVENT_BROWSER_COMPOSITE_COMPONENT, This should instead find the component with GetLatencyComponentId() as the component id. You'll also need to modify RenderWidgetHostImpl::CompositorFrameDrawn to route the LatencyInfo to the correct RWHI based on that component id for this event. https://codereview.chromium.org/577273003/diff/40001/ui/events/latency_info.h File ui/events/latency_info.h (right): https://codereview.chromium.org/577273003/diff/40001/ui/events/latency_info.h... ui/events/latency_info.h:58: INPUT_EVENT_BROWSER_COMPOSITE_COMPONENT, These don't really have anything to do with input events, so I don't think we should have INPUT_EVENT in the name.
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/577273003/diff/40001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/577273003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.cc:1460: ui::INPUT_EVENT_BROWSER_COMPOSITE_COMPONENT, 0, 0); On 2014/09/19 20:40:29, jbauman wrote: > This should specify GetLatencyComponentId() as the component ID. Done. https://codereview.chromium.org/577273003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.cc:1460: ui::INPUT_EVENT_BROWSER_COMPOSITE_COMPONENT, 0, 0); On 2014/09/18 23:26:15, brianderson wrote: > If the Browser is swap or commit throttled, there may be a delay from when the > Renderer's swap is received to when it is actually consumed by the Browser. > > For deadline estimation purposes, we care about the timing from when the swap is > consumed, not from when it is received. > > Not sure where the best place to add an > INPUT_EVENT_RENDERER_SWAP_CONSUMED_COMPONENT is though... > > The INPUT_EVENT_BROWSER_COMPOSITE_COMPONENT component would still be useful > though for test/debug purposes, so it might be nice to keep it here. Maybe call > it INPUT_EVENT_RENDERER_SWAP_RECEIVED_COMPONENT t: @brian I've renamed the event. https://codereview.chromium.org/577273003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.cc:2162: if (latency_info.FindLatency(ui::INPUT_EVENT_BROWSER_COMPOSITE_COMPONENT, On 2014/09/19 20:40:29, jbauman wrote: > This should instead find the component with GetLatencyComponentId() as the > component id. > > You'll also need to modify RenderWidgetHostImpl::CompositorFrameDrawn to route > the LatencyInfo to the correct RWHI based on that component id for this event. Done. https://codereview.chromium.org/577273003/diff/40001/ui/events/latency_info.h File ui/events/latency_info.h (right): https://codereview.chromium.org/577273003/diff/40001/ui/events/latency_info.h... ui/events/latency_info.h:58: INPUT_EVENT_BROWSER_COMPOSITE_COMPONENT, On 2014/09/19 20:40:29, jbauman wrote: > These don't really have anything to do with input events, so I don't think we > should have INPUT_EVENT in the name. Technically the events are still tied to input in that latency_info only has input related begin components. I was planning on renaming these in a later patch once I've added per frame latency_info. https://codereview.chromium.org/577273003/diff/40001/ui/events/latency_info.h... ui/events/latency_info.h:59: // Timestamp of when an unparented output surface began swap buffers On 2014/09/18 23:26:15, brianderson wrote: > "an uparented output surface" -> "the Browser process" Done. https://codereview.chromium.org/577273003/diff/40001/ui/events/latency_info.h... ui/events/latency_info.h:60: INPUT_EVENT_OUTPUT_SURFACE_SWAP_BUFFER_COMPONENT, On 2014/09/18 23:26:15, brianderson wrote: > INPUT_EVENT_BROWSER_SWAP_BUFFER_COMPONENT? Done.
lgtm w/ nits, but you'll need OWNERS approval for dirs other than cc. This gives us good estimates for the most common single-producer use cases. We can worry about multiple producer and throttled use cases later, which will be more important once the unified BeginFrame stuff lands. https://codereview.chromium.org/577273003/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/577273003/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.cc:2347: return browser_composite_latency_history_.Percentile( Let's add some slack to the estimate by multiplying the estimate by 110%. https://codereview.chromium.org/577273003/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (left): https://codereview.chromium.org/577273003/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1343: base::TimeDelta estimated_browser_composite_time = Can get rid of this local variable now.
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
https://codereview.chromium.org/577273003/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/577273003/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.cc:2347: return browser_composite_latency_history_.Percentile( On 2014/09/22 22:59:35, brianderson wrote: > Let's add some slack to the estimate by multiplying the estimate by 110%. Done. https://codereview.chromium.org/577273003/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (left): https://codereview.chromium.org/577273003/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1343: base::TimeDelta estimated_browser_composite_time = On 2014/09/22 22:59:35, brianderson wrote: > Can get rid of this local variable now. Done.
mithro@mithis.com changed reviewers: + mithro@mithis.com
Does this make sense to support for all platforms, not just Android?
Yes, the plan is to move this to other platforms once unified begin frame has been added.
brianderson@chromium.org changed reviewers: + miletus@chromium.org - yufeng@chromium.org
@miletus: Can you review the LatencyInfo bits of this patch? @sievers, @jbauman: Do the other parts of this patch look good?
lgtm
https://codereview.chromium.org/577273003/diff/180001/ui/events/latency_info.h File ui/events/latency_info.h (right): https://codereview.chromium.org/577273003/diff/180001/ui/events/latency_info.... ui/events/latency_info.h:105: // Empirically determined constant based on a typical scroll sequence. Can you tune this number ?
https://codereview.chromium.org/577273003/diff/180001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/577273003/diff/180001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2165: if (latency_info.FindLatency(ui::INPUT_EVENT_RENDERER_SWAP_RECEIVED_COMPONENT, Hmm, so does that ignore the time it took the renderer to composite and for the SwapCompositorFrame msg to make it from renderer to UI thread? https://codereview.chromium.org/577273003/diff/180001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2351: kBrowserCompositeLatencyEstimationSlack; Can we also use the stddev or variance when coming up with the slack?
Patchset #6 (id:200001) has been deleted
https://codereview.chromium.org/577273003/diff/180001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/577273003/diff/180001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2165: if (latency_info.FindLatency(ui::INPUT_EVENT_RENDERER_SWAP_RECEIVED_COMPONENT, On 2014/10/01 22:15:08, sievers wrote: > Hmm, so does that ignore the time it took the renderer to composite and for the > SwapCompositorFrame msg to make it from renderer to UI thread? Yea this is just for the browser composite time. We're using it to inform the renderer of how much time it has until the next swap, so we don't need to take into account renderer composite time or SwapCompositorFrame propagation time since the deadline refers to the swap on the gpu service. https://codereview.chromium.org/577273003/diff/180001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2351: kBrowserCompositeLatencyEstimationSlack; On 2014/10/01 22:15:08, sievers wrote: > Can we also use the stddev or variance when coming up with the slack? We could, however, we run the risk of a one-off really bad frame messing with our slack estimates causing sort-of cascading bad estimates (which is also why we use the 90th percentile when selecting our estimate instead of the worst case). Let me know if this is overly cautious. https://codereview.chromium.org/577273003/diff/180001/ui/events/latency_info.h File ui/events/latency_info.h (right): https://codereview.chromium.org/577273003/diff/180001/ui/events/latency_info.... ui/events/latency_info.h:105: // Empirically determined constant based on a typical scroll sequence. On 2014/10/01 21:46:19, Yufeng Shen wrote: > Can you tune this number ? Yep, I've made it 9 since all 3 of the new components will be added when we don't abort a commit.
On 2014/10/02 00:46:04, orglofch wrote: > https://codereview.chromium.org/577273003/diff/180001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_impl.cc (right): > > https://codereview.chromium.org/577273003/diff/180001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_impl.cc:2165: if > (latency_info.FindLatency(ui::INPUT_EVENT_RENDERER_SWAP_RECEIVED_COMPONENT, > On 2014/10/01 22:15:08, sievers wrote: > > Hmm, so does that ignore the time it took the renderer to composite and for > the > > SwapCompositorFrame msg to make it from renderer to UI thread? > Yea this is just for the browser composite time. We're using it to inform the > renderer of how much time it has until the next swap, so we don't need to take > into account renderer composite time or SwapCompositorFrame propagation time > since the deadline refers to the swap on the gpu service. > > https://codereview.chromium.org/577273003/diff/180001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_impl.cc:2351: > kBrowserCompositeLatencyEstimationSlack; > On 2014/10/01 22:15:08, sievers wrote: > > Can we also use the stddev or variance when coming up with the slack? > We could, however, we run the risk of a one-off really bad frame messing with > our slack estimates causing sort-of cascading bad estimates (which is also why > we use the 90th percentile when selecting our estimate instead of the worst > case). Let me know if this is overly cautious. > > https://codereview.chromium.org/577273003/diff/180001/ui/events/latency_info.h > File ui/events/latency_info.h (right): > > https://codereview.chromium.org/577273003/diff/180001/ui/events/latency_info.... > ui/events/latency_info.h:105: // Empirically determined constant based on a > typical scroll sequence. > On 2014/10/01 21:46:19, Yufeng Shen wrote: > > Can you tune this number ? > > Yep, I've made it 9 since all 3 of the new components will be added when we > don't abort a commit. LatencyInfo lgtm.
https://codereview.chromium.org/577273003/diff/180001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/577273003/diff/180001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2165: if (latency_info.FindLatency(ui::INPUT_EVENT_RENDERER_SWAP_RECEIVED_COMPONENT, On 2014/10/02 00:46:04, orglofch wrote: > On 2014/10/01 22:15:08, sievers wrote: > > Hmm, so does that ignore the time it took the renderer to composite and for > the > > SwapCompositorFrame msg to make it from renderer to UI thread? > Yea this is just for the browser composite time. We're using it to inform the > renderer of how much time it has until the next swap, so we don't need to take > into account renderer composite time or SwapCompositorFrame propagation time > since the deadline refers to the swap on the gpu service. Ah ok, it looks like the renderer adjusts the deadline by how long it takes to child-composite: begin_impl_frame_args_.deadline -= draw_duration_estimate; Maybe the slack needs to be a bit bigger to also account for the latency between sending the frame, running OnSwapCompositorFrame, and actually running Composite(). On Android we often have high latency from these IPC thread hops. https://codereview.chromium.org/577273003/diff/180001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2351: kBrowserCompositeLatencyEstimationSlack; On 2014/10/02 00:46:04, orglofch wrote: > On 2014/10/01 22:15:08, sievers wrote: > > Can we also use the stddev or variance when coming up with the slack? > We could, however, we run the risk of a one-off really bad frame messing with > our slack estimates causing sort-of cascading bad estimates (which is also why > we use the 90th percentile when selecting our estimate instead of the worst > case). Let me know if this is overly cautious. I was actually more worried that we end up using something that is too close and then end up missing vsync every 10th frame or so. Wouldn't that cause jank? Is there another way we can come up with a number min_composite_time here and use the smaller of min_composite_time and the percentile here? Even if that value is just the hardcoded value we currently use (1/3 of vsync_period or whatever it is). I thought we are mostly trying to fix the problem of devices where our current constant is too small (those devices take longer to do all the work across the threads). What I don't understand is what the gain is from moving the deadline closer to the next vsync for devices that are actually faster. Or also: what is the problem with using an immediate deadline for devices that are slower and having main thread commits run in higher latency?
On 2014/10/02 20:17:42, sievers wrote: > I was actually more worried that we end up using something that is too close and > then end up missing vsync every 10th frame or so. Wouldn't that cause jank? > > Is there another way we can come up with a number min_composite_time here and > use the smaller of min_composite_time and the percentile here? > Even if that value is just the hardcoded value we currently use (1/3 of > vsync_period or whatever it is). > oops... "use the *greater* of min_..." !
https://codereview.chromium.org/577273003/diff/180001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/577273003/diff/180001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2351: kBrowserCompositeLatencyEstimationSlack; On 2014/10/02 20:17:42, sievers wrote: > On 2014/10/02 00:46:04, orglofch wrote: > > On 2014/10/01 22:15:08, sievers wrote: > > > Can we also use the stddev or variance when coming up with the slack? > > We could, however, we run the risk of a one-off really bad frame messing with > > our slack estimates causing sort-of cascading bad estimates (which is also why > > we use the 90th percentile when selecting our estimate instead of the worst > > case). Let me know if this is overly cautious. > > I was actually more worried that we end up using something that is too close and > then end up missing vsync every 10th frame or so. Wouldn't that cause jank? > > Is there another way we can come up with a number min_composite_time here and > use the smaller of min_composite_time and the percentile here? > Even if that value is just the hardcoded value we currently use (1/3 of > vsync_period or whatever it is). > > I thought we are mostly trying to fix the problem of devices where our current > constant is too small (those devices take longer to do all the work across the > threads). > > What I don't understand is what the gain is from moving the deadline closer to > the next vsync for devices that are actually faster. > > Or also: what is the problem with using an immediate deadline for devices that > are slower and having main thread commits run in higher latency? > Per our offline discussion, I've used the original estimate as the lower bound to prevent potential regressions in the short time on slow devices. Will followup on cases where the min_estimate is too large and we want to give the impl thread more time to activate etc. separately.
lgtm, thanks!
The CQ bit was checked by orglofch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/577273003/240001
Message was sent while issue was closed.
Committed patchset #7 (id:240001) as ca0f047e42f574e6fe043bd58f89e76c1cb3425a
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/545a72d3f37fa15844cb08d83db7cca672b79cd0 Cr-Commit-Position: refs/heads/master@{#297950} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
