Issue metadata
Sign in to add a comment
|
4.9%-949.6% regression in page_cycler_v2_site_isolation.basic_oopif at 482621:482822 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jun 29 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8975446948281942320
,
Jun 29 2017
=== Auto-CCing suspected CL author kylechar@chromium.org === Hi kylechar@chromium.org, the bisect results pointed to your CL, please take a look at the results. === BISECT JOB RESULTS === Perf regression found with culprit Suspected Commit Author : kylechar Commit : 390e0b850ab633846734aee825f1ce927c863d46 Date : Tue Jun 27 20:16:05 2017 Subject: Move FrameSink hierarchy registration to FrameSinkManagerHost. Bisect Details Configuration: mac_retina_perf_bisect Benchmark : page_cycler_v2.basic_oopif Metric : timeToFirstContentfulPaint_avg/pcv1-cold/http___booking.com Change : 161.29% | 167.012499988 -> 436.388333321 Revision Result N chromium@482648 167.012 +- 33.9647 6 good chromium@482692 161.876 +- 12.9365 6 good chromium@482714 163.108 +- 18.9751 6 good chromium@482720 164.691 +- 15.1698 5 good chromium@482723 161.881 +- 17.5265 6 good chromium@482724 442.029 +- 16.568 5 bad <-- chromium@482725 437.506 +- 24.0536 6 bad chromium@482735 438.707 +- 29.0773 6 bad chromium@482822 436.388 +- 30.1501 6 bad To Run This Test src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=http...booking.com page_cycler_v2.basic_oopif Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8975446948281942320 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=5813130925441024 | O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq | X | for more information addressing perf regression bugs. For feedback, | / \ | file a bug with component Speed>Bisection. Thank you!
,
Jun 29 2017
Oh that looks bad. The RegisterFrameSinkHierarchy() call will take longer over Mojo IPC compared to a direct function call. However, it's a single IPC that happens once (or once per embedded OOPIF) when opening a new tab/window and the overhead of the IPC should be well <1ms. I wouldn't have been surprised at a small slow down in time to first paint. The slow down of something like +2000ms on page_cycler_v2.basic_oopif/timeToFirstContentfulPaint_avg/pcv1-cold/http___booking.com wasn't expected. The same metric on other websites were not impacted by the change, which would support the IPC adding minimal overhead. My theory is that it is scheduling delay related. If the browser UI thread is very busy and many tasks have been posted to it before RegisterFrameSinkHierarchy() is called it will I think have to finish the other tasks first before Mojo gets to the IPC? I think at least, I'm not positive on how Mojo scheduling works for an in process Mojo call. I'll investigate further today and possibly revert 390e0b850ab633846734aee825f1ce927c863d46 for now.
,
Jun 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f9edf70a74e3f5027b2c38d980789a30726745dc commit f9edf70a74e3f5027b2c38d980789a30726745dc Author: kylechar <kylechar@chromium.org> Date: Thu Jun 29 18:02:36 2017 Revert "Move FrameSink hierarchy registration to FrameSinkManagerHost." This reverts commit 390e0b850ab633846734aee825f1ce927c863d46. Reason for revert: This caused some large regressions in some time to first paint metrics. See https://chromeperf.appspot.com/group_report?bug_id=737974 and http://crbug.com/737974 . Original change's description: > Move FrameSink hierarchy registration to FrameSinkManagerHost. > > Change all calls to SurfaceManager::(Unr|R)registerFrameSinkHiearchy() > to be on FrameSinkManagerHost instead. This will have the calls happen > via Mojo asynchronously. > > FrameSink hierarchy changes are infrequent so this isn't expected to > have performance implications. This will cause a change in ordering with > regards to FrameSink invalidation compared to hierarchy changes which is > already handled in cc::FrameSinkManager correctly. > > Bug: 657959 > Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation > Change-Id: I475e7952796cb93e1a89c1f168efee4a312d0e76 > Reviewed-on: https://chromium-review.googlesource.com/524003 > Commit-Queue: kylechar <kylechar@chromium.org> > Reviewed-by: Scott Violet <sky@chromium.org> > Reviewed-by: Nasko Oskov <nasko@chromium.org> > Reviewed-by: Ken Rockot <rockot@chromium.org> > Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> > Reviewed-by: danakj <danakj@chromium.org> > Reviewed-by: Bo Liu <boliu@chromium.org> > Cr-Commit-Position: refs/heads/master@{#482724} TBR=sadrul@chromium.org,sky@chromium.org,danakj@chromium.org,nasko@chromium.org,boliu@chromium.org,rockot@chromium.org,kylechar@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 737974 ,657959 Change-Id: Ieaa6e49e61c99c219acbcf63a5bdfe9da00fba17 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Reviewed-on: https://chromium-review.googlesource.com/555713 Commit-Queue: kylechar <kylechar@chromium.org> Reviewed-by: kylechar <kylechar@chromium.org> Cr-Commit-Position: refs/heads/master@{#483420} [modify] https://crrev.com/f9edf70a74e3f5027b2c38d980789a30726745dc/content/browser/compositor/surface_utils.cc [modify] https://crrev.com/f9edf70a74e3f5027b2c38d980789a30726745dc/content/browser/frame_host/render_widget_host_view_child_frame.cc [modify] https://crrev.com/f9edf70a74e3f5027b2c38d980789a30726745dc/content/browser/renderer_host/compositor_impl_android.cc [modify] https://crrev.com/f9edf70a74e3f5027b2c38d980789a30726745dc/ui/compositor/compositor.cc
,
Jul 5 2017
|
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by tdres...@chromium.org
, Jun 29 2017