New issue
Advanced search Search tips

Issue 849729 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

LatencyInfo: component-id no longer needed for AddLatency/FindLatency

Project Member Reported by sadrul@chromium.org, Jun 5 2018

Issue description

After the removal of TAB_SHOW_COMPONENT (in crrev.com/c/1083862), the |id| if the various AddLatency/FindLatency methods are no longer needed in LatencyInfo.
 
Summary: LatencyInfo: component-id no longer needed for AddLatency/FindLatency (was: LatencyInfo: id no longer needed for AddLatency/FindLatency)
The only places where a non-zero id is used currently are in RenderWidgetHostLatencyTracker for BEGIN_RWH_COMPONENT [1] and SCROLL_UPDATE_ORIGINAL_COMPONENT/FIRST_SCROLL_UPDATE_ORIGINAL_COMPONENT [2]. Each RenderWidgetHostImpl owns a RWHLatencyTracker, and therefore the LatencyInfo objects should ever be handled only by the same RWHLatencyTracker instance, and no other code adds these components to the LatencyInfo. So I don't believe using the non-zero component-id serves any purpose. So it should be safe to remove the component-id.

[1] https://cs.chromium.org/chromium/src/content/browser/renderer_host/input/render_widget_host_latency_tracker.cc?type=cs&sq=package:chromium&g=0&l=179
[2] https://cs.chromium.org/chromium/src/content/browser/renderer_host/input/render_widget_host_latency_tracker.cc?type=cs&sq=package:chromium&g=0&l=195

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/128d1c67f0594fbc62b8a5083345f2a113e4ffa7

commit 128d1c67f0594fbc62b8a5083345f2a113e4ffa7
Author: Sadrul Habib Chowdhury <sadrul@chromium.org>
Date: Fri Jun 08 00:37:56 2018

latency: Stop using a component-id for BEGIN_RWH component.

The component-id for BEGIN_RWH component is currently generated from the
process-id and routing-id of the associated RenderWidgetHostImpl, so
that it is possible to find the RenderWidgetHostImpl from the
LatencyInfo after gpu-swap happens. However, the BEGIN_RWH component is
unused at that time (only LatencyInfos that have snapshot requests are
processed after gpu-swap, and the snapshot requests already carry the
identifier for the RenderWidgetHostImpl separately), except for in a
test. So, the changes in this CL:
 . Use zero as the component-id for BEGIN_RWH component.
 . Introduce a SetLatencyInfoProcessorForTesting() method to install a
   callback that receives all the LatencyInfo after a gpu-swap happens,
   and use that in the tests that needed this.

BUG=849729

Change-Id: I19beb790931cfe8e708d0eb4223a3ba23d51e4c1
Reviewed-on: https://chromium-review.googlesource.com/1089583
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565488}
[modify] https://crrev.com/128d1c67f0594fbc62b8a5083345f2a113e4ffa7/content/browser/renderer_host/input/mouse_latency_browsertest.cc
[modify] https://crrev.com/128d1c67f0594fbc62b8a5083345f2a113e4ffa7/content/browser/renderer_host/input/render_widget_host_latency_tracker.cc
[modify] https://crrev.com/128d1c67f0594fbc62b8a5083345f2a113e4ffa7/content/browser/renderer_host/input/render_widget_host_latency_tracker.h
[modify] https://crrev.com/128d1c67f0594fbc62b8a5083345f2a113e4ffa7/content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc
[modify] https://crrev.com/128d1c67f0594fbc62b8a5083345f2a113e4ffa7/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/128d1c67f0594fbc62b8a5083345f2a113e4ffa7/content/browser/renderer_host/render_widget_host_impl.h
[modify] https://crrev.com/128d1c67f0594fbc62b8a5083345f2a113e4ffa7/content/browser/renderer_host/render_widget_host_unittest.cc

Comment 3 Deleted

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/73387efb66b0cb6324309d8b27a992f499240ba8

commit 73387efb66b0cb6324309d8b27a992f499240ba8
Author: Sadrul Habib Chowdhury <sadrul@chromium.org>
Date: Fri Jun 08 05:43:18 2018

latency: Remove the last non-zero component id.

Only SCROLL_UPDATE and FIRST_SCROLL_UPDATE components currently have
non-zero component ids. However, these component-ids are never used for
any purpose. So use zero as the component-id for these components. This
allows for the following cleanups:
 . RenderWidgetHostLatencyTracker no longer needs latency_component_id_.
 . Rename RenderWidgetHostImpl::GetLatencyComponentId() to a clearer
   name: GetFrameSinkIdForSnapshot().

BUG=849729

Change-Id: I7fea87d5cf1b06c580b6738816ffeb893063b1fb
Reviewed-on: https://chromium-review.googlesource.com/1089715
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565559}
[modify] https://crrev.com/73387efb66b0cb6324309d8b27a992f499240ba8/content/browser/renderer_host/input/render_widget_host_latency_tracker.cc
[modify] https://crrev.com/73387efb66b0cb6324309d8b27a992f499240ba8/content/browser/renderer_host/input/render_widget_host_latency_tracker.h
[modify] https://crrev.com/73387efb66b0cb6324309d8b27a992f499240ba8/content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc
[modify] https://crrev.com/73387efb66b0cb6324309d8b27a992f499240ba8/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/73387efb66b0cb6324309d8b27a992f499240ba8/content/browser/renderer_host/render_widget_host_impl.h
[modify] https://crrev.com/73387efb66b0cb6324309d8b27a992f499240ba8/content/browser/renderer_host/render_widget_host_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 9 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/41154dc00fcddb75421f57b033c28af29a66eb80

commit 41154dc00fcddb75421f57b033c28af29a66eb80
Author: Sadrul Habib Chowdhury <sadrul@chromium.org>
Date: Sat Jun 09 04:25:01 2018

latency: Remove component-id from LatencyInfo.

All the current uses of component-id uses zero as the component id,
and the component-id is no longer used for any purpose. So remove
this from LatencyInfo.

BUG=849729
TBR=danakj@ for trivial cc/ changes.

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I43725092fecbcd85d208205382b2cafe490e845e
Reviewed-on: https://chromium-review.googlesource.com/1088194
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565854}
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/cc/trees/latency_info_swap_promise_monitor.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/cc/trees/proxy_main.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/cc/trees/single_thread_proxy.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/components/viz/service/display/display.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/components/viz/service/display/output_surface.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/components/viz/service/display_embedder/software_output_surface.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/components/viz/service/frame_sinks/compositor_frame_sink_support.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/components/viz/service/frame_sinks/surface_synchronization_unittest.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/content/browser/compositor/software_browser_compositor_output_surface.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/content/browser/renderer_host/input/render_widget_host_latency_tracker.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/content/browser/renderer_host/input/render_widget_host_latency_tracker.h
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/content/browser/renderer_host/input/synthetic_gesture_target_base.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/content/browser/renderer_host/render_widget_host_view_cocoa.mm
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/content/browser/renderer_host/render_widget_host_view_mac.mm
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/content/common/input/event_with_latency_info_unittest.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/content/renderer/input/render_widget_input_handler.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/services/viz/public/cpp/compositing/struct_traits_unittest.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/ui/events/event.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/ui/events/event_unittest.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/ui/latency/ipc/latency_info_param_traits_unittest.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/ui/latency/latency_info.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/ui/latency/latency_info.h
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/ui/latency/latency_info_unittest.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/ui/latency/latency_tracker.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/ui/latency/mojo/latency_info.mojom
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/ui/latency/mojo/latency_info.typemap
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/ui/latency/mojo/latency_info_struct_traits.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/ui/latency/mojo/latency_info_struct_traits.h
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/ui/latency/mojo/struct_traits_unittest.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/ui/latency/mojo/traits_test_service.mojom
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/ui/views/win/hwnd_message_handler.cc
[modify] https://crrev.com/41154dc00fcddb75421f57b033c28af29a66eb80/ui/views/win/pen_event_processor.cc

Can this be marked as fixed now?
Sadrul, what's the status of this? Is it fixed?
Cc: -briander...@chromium.org
Components: -Speed>Metrics

Sign in to add a comment