Issue metadata
Sign in to add a comment
|
2.7%-15.5% regression in thread_times.tough_compositor_cases at 537295:537506 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Feb 20 2018
π Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/1296a3d7840000
,
Feb 21 2018
π 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
,
Feb 21 2018
+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.
,
Feb 21 2018
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.
,
Feb 21 2018
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.
,
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.
,
Feb 21 2018
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
,
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)
,
Feb 21 2018
π Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/16fdf917840000
,
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
,
Feb 23 2018
,
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
,
Mar 5 2018
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 |
|||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Feb 20 2018