New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 813970 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

2.7%-15.5% regression in thread_times.tough_compositor_cases at 537295:537506

Project Member Reported by briander...@chromium.org, Feb 20 2018

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Feb 20 2018

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=813970

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=f4c8ce0ac002b5b16ccbaa2eaa7131888baa5df0279fac066cf462189eea7b4a


Bot(s) for this bug's original alert(s):

android-nexus5X
chromium-rel-win7-gpu-ati
chromium-rel-win7-gpu-intel
chromium-rel-win7-gpu-nvidia
linux-release
Project Member

Comment 2 by 42576172...@developer.gserviceaccount.com, Feb 20 2018

πŸ“ Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/1296a3d7840000
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Feb 21 2018

Cc: kenrb@chromium.org fsam...@chromium.org jonr...@chromium.org kylec...@chromium.org piman@chromium.org
Owner: jonr...@chromium.org
Status: Assigned (was: Untriaged)
πŸ“ Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/1296a3d7840000

Renderer observer of frame submission by jonross@chromium.org
https://chromium.googlesource.com/chromium/src/+/a2ff4f82109df55045dee9f54985a98054f86dc4

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions
Cc: sadrul@chromium.org
+sadrul@ it would be good to sync up offline about this.

I've reviewed the graphs and spoke with kylechar@. The regressions in the charts are what we would expect for the change listed in #3

We've added a new mojo api which is sending IPCs. So the change in tasks per frame is expected (3 - 2 for each side of IPC, 1 for compositor thread to IO thread)

There's also an increase in time/cpu for IO which is also expected.

Though as we begin pulling items out of CompositorFrameMetadata and only into RenderFrameMetadata we will reduce the amount of duplicate information being sent in IPCs, which should help bring some of this back down.
What expects this value? Given the regression, maybe we should reconsider only turning on this IPC when needed. We will have additional races to deal with but again I would argue they’re worth the effort to avoid the regression. 
Expectation was by looking at the change in number of tasks per frame. Which matches the number of tasks per frame which we've added.

We can certainly reconsider switching this IPC to only be on when needed. There are a few points to weigh
  - introducing new races in the code
  - the number of tasks per frame tracking back up as more parts of the browser use this
  - potential savings as we reduce what is in the other IPC (CompositorFrameMetadata)
  - complexity of this work when the regressions are mostly within the recent range of these metrics. What are our overall tolerances here?

Also while these regressions are in task counts, the frame throughput showed little changes: https://pinpoint-dot-chromeperf.appspot.com/results2/1296a3d7840000

mean_frame_time_renderer_compositor 16.960ms vs 16.880ms. A 0.5% variance in frame time seems like actual user facing impact is within noise.

Comment 7 by piman@chromium.org, Feb 21 2018

I suspect you'll find that the overhead of the extra IPC (posted tasks, locks, syscalls) is dwarfing the cost of the data being sent (a few bytes to copy), so moving fields between IPCs will probably have negligible impact.
It doesn't look like there is any regression in the CPU time for browser, IO or total. It's just the number of tasks has increased.

https://chromeperf.appspot.com/report?sid=cb37fbe8a10283963ea6a9253d13945b5b4534f1b377ecada05e7246314413bc&rev=537361

Comment 9 by sadrul@google.com, Feb 21 2018

https://chromeperf.appspot.com/group_report?bug_id=813970 shows some small regressions on IO thread-time (~0.17ms to ~0.18ms), and renderer-compositor thread-time (~0.90ms to ~0.95ms)
Project Member

Comment 10 by 42576172...@developer.gserviceaccount.com, Feb 21 2018

πŸ“ Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/16fdf917840000
Project Member

Comment 11 by 42576172...@developer.gserviceaccount.com, Feb 23 2018

πŸ“ Found a significant difference after 1 commit.
https://chromeperf.appspot.com/job/16fdf917840000

Renderer observer of frame submission by jonross@chromium.org
https://chromium.googlesource.com/chromium/src/+/a2ff4f82109df55045dee9f54985a98054f86dc4

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions
Status: Started (was: Assigned)
Project Member

Comment 13 by bugdroid1@chromium.org, Mar 2 2018

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

commit 13c8861d8d297b2dcb54b7a68eda0d09a6ea2c0e
Author: jonross <jonross@chromium.org>
Date: Fri Mar 02 17:12:02 2018

Rate Limit RenderFrameMetadataObserver

RenderFrameMetadataObserverImpl was sending a message from the renderer to the
browser whenever any metadata changes. One such piece of data is the scroll
offset. Due to this, when scrolling, there would be an IPC from the renderer
to the browser on each frame.

This was picked up as a regression in the number of tasks.

After analysing the plans for which data will exist in RenderFrameMetadata we
determined that there are some which are high frequency, and some which are low.
Additionally the majority of the usage of the high frequency data is in tests.

In this change RenderFrameMetadataObserverImpl is updated to check for the
difference in RenderFrameMetadata based on high frequency and low frequency
fields. It will always send updates for low frequency fields. However for high
frequency fields, such as root_scroll_offset, it will only send in testing mode.

The currently implemented tests continue to pass with this change.

TEST=NonBlockingEventBrowserTest

Bug:  813970 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I4c21030a703c207519a56a851e052d9087708533
Reviewed-on: https://chromium-review.googlesource.com/934626
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540539}
[modify] https://crrev.com/13c8861d8d297b2dcb54b7a68eda0d09a6ea2c0e/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/13c8861d8d297b2dcb54b7a68eda0d09a6ea2c0e/cc/trees/render_frame_metadata.cc
[modify] https://crrev.com/13c8861d8d297b2dcb54b7a68eda0d09a6ea2c0e/cc/trees/render_frame_metadata.h
[modify] https://crrev.com/13c8861d8d297b2dcb54b7a68eda0d09a6ea2c0e/cc/trees/render_frame_metadata_observer.h
[modify] https://crrev.com/13c8861d8d297b2dcb54b7a68eda0d09a6ea2c0e/content/browser/renderer_host/input/non_blocking_event_browsertest.cc
[modify] https://crrev.com/13c8861d8d297b2dcb54b7a68eda0d09a6ea2c0e/content/browser/renderer_host/input/touch_action_browsertest.cc
[modify] https://crrev.com/13c8861d8d297b2dcb54b7a68eda0d09a6ea2c0e/content/common/render_frame_metadata.mojom
[modify] https://crrev.com/13c8861d8d297b2dcb54b7a68eda0d09a6ea2c0e/content/common/render_frame_metadata_struct_traits.h
[modify] https://crrev.com/13c8861d8d297b2dcb54b7a68eda0d09a6ea2c0e/content/renderer/render_frame_metadata_observer_impl.cc
[modify] https://crrev.com/13c8861d8d297b2dcb54b7a68eda0d09a6ea2c0e/content/renderer/render_frame_metadata_observer_impl.h

Status: Fixed (was: Started)
All graphs in the bug are back to (or below) original levels. With spikes down associated for when the patch landed.

Sign in to add a comment