Issue metadata
Sign in to add a comment
|
5.2% regression in media.android.tough_video_cases_tbmv2 at 489547:489568 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jul 31 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8972518853802617440
,
Jul 31 2017
=== Auto-CCing suspected CL author sunnyps@chromium.org === Hi sunnyps@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 : Sunny Sachanandani Commit : 70d14d2a335c81fb2e8d35db453d58de423c726d Date : Wed Jul 26 06:03:59 2017 Subject: Reland of gpu: Allow empty sync tokens in mailboxes. Bisect Details Configuration: android_nexus5X_perf_bisect Benchmark : media.android.tough_video_cases_tbmv2 Metric : cpu_time_percentage_avg/video.html?src_tulip2.mp4 Change : 4.00% | 0.550668161499 -> 0.572680378408 Revision Result N chromium@489546 0.550668 +- 0.0127825 6 good chromium@489557 0.553817 +- 0.00533871 6 good chromium@489560 0.549905 +- 0.0142352 6 good chromium@489562 0.552149 +- 0.0122021 6 good chromium@489563 0.575512 +- 0.0127663 6 bad <-- chromium@489568 0.57268 +- 0.0146034 6 bad To Run This Test src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=video.html.src.tulip2.mp4 media.android.tough_video_cases_tbmv2 More information on addressing performance regressions: http://g.co/ChromePerformanceRegressions Debug information about this bisect: https://chromeperf.appspot.com/buildbucket_job_status/8972518853802617440 For feedback, file a bug with component Speed>Bisection
,
Jul 31 2017
Hi Sunny, it looks like your change caused a regression: Chrome's total CPU usage during video playback increased by around 4% or 5%. We know this because we tested both for a build of Chrome right after your change and for a build of Chrome right before your change. We ran the test 6 times for each build and ran statistical test to make sure the difference was significant. This is a pretty large regression.
,
Jul 31 2017
I'm setting this as a P1 for M62 since we don't want to ship a 5% CPU usage increase. Feel free to re-prioritize if this doesn't make sense.
,
Jul 31 2017
This CL can't catch a break, apparently. Let me know if there is a way I can help at all!
,
Jul 31 2017
This is probably the same regression as https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDgiaqDzAgM, which is chromium-rel-mac-retina cpu_time_percentage_avg/video.html?src_crowd1080.mp4 test).
,
Jul 31 2017
Given that I don't see any perf alerts for Windows or Linux, this is probably related to either the use of GLImages to bind textures, or the overlay path. crouleau@: Does this test play in full-screen? (If not, then overlays would not be used on Android.)
,
Aug 1 2017
It is not fullscreen. Feel free to run the test yourself. The command is listed in #3.
,
Aug 1 2017
Issue 750914 has been merged into this issue.
,
Aug 1 2017
On windows with overlays enabled I'm seeing VideoFrame::UpdateReleaseSyncToken (called from VideoResourceUpdater::ReturnTexture) is now generating and validating a sync token. It didn't used to do that, because it would just use the sync token from the browser process.
,
Aug 1 2017
Thanks for finding the cause of the regression. I'll add the sync token optimization back again (and TBR you) and we'll see if that fixes the regression.
,
Aug 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/390899a37ee3433a19d3a04610748bbdf6500fce commit 390899a37ee3433a19d3a04610748bbdf6500fce Author: Sunny Sachanandani <sunnyps@chromium.org> Date: Wed Aug 02 23:58:21 2017 cc: Skip generating video release sync token if not needed. I removed the optimizations in SyncPointClientImpl where it'd skip generating sync tokens if there was no existing release sync token. That caused a 5% cpu time regression in the video benchmarks. I also added some tests to verify that sync tokens are being generated at the right places. R=jbauman BUG= 750872 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: If739e3fb6705cf8b0739fc029ed3bd75dc43eabf Reviewed-on: https://chromium-review.googlesource.com/596424 Reviewed-by: John Bauman <jbauman@chromium.org> Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org> Cr-Commit-Position: refs/heads/master@{#491560} [modify] https://crrev.com/390899a37ee3433a19d3a04610748bbdf6500fce/cc/resources/video_resource_updater.cc [modify] https://crrev.com/390899a37ee3433a19d3a04610748bbdf6500fce/cc/resources/video_resource_updater_unittest.cc
,
Aug 8 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8971786355215889632
,
Aug 8 2017
Seems to have recovered. I've started a bisect to confirm.
,
Aug 9 2017
=== Auto-CCing suspected CL author sunnyps@chromium.org === Hi sunnyps@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 : Sunny Sachanandani Commit : 390899a37ee3433a19d3a04610748bbdf6500fce Date : Wed Aug 02 23:58:21 2017 Subject: cc: Skip generating video release sync token if not needed. Bisect Details Configuration: android_nexus5X_perf_bisect Benchmark : media.android.tough_video_cases_tbmv2 Metric : cpu_time_percentage_avg/video.html?src_tulip2.mp4 Change : 5.01% | 0.586312925831 -> 0.556929312183 Revision Result N chromium@491494 0.586313 +- 0.0123468 6 good chromium@491553 0.583372 +- 0.0205077 9 good chromium@491557 0.574472 +- 0.0218791 8 good chromium@491559 0.575781 +- 0.0167435 6 good chromium@491560 0.558525 +- 0.0165385 6 bad <-- chromium@491561 0.557589 +- 0.0201344 9 bad chromium@491568 0.555793 +- 0.00986087 6 bad chromium@491582 0.560068 +- 0.0148535 6 bad chromium@491611 0.556929 +- 0.0150285 6 bad To Run This Test src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=video.html.src.tulip2.mp4 media.android.tough_video_cases_tbmv2 More information on addressing performance regressions: http://g.co/ChromePerformanceRegressions Debug information about this bisect: https://chromeperf.appspot.com/buildbucket_job_status/8971786355215889632 For feedback, file a bug with component Speed>Bisection
,
Aug 9 2017
good == bad in the above bisect |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Jul 31 2017