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

Issue 750872 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Aug 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug-Regression



Sign in to add a comment

5.2% regression in media.android.tough_video_cases_tbmv2 at 489547:489568

Project Member Reported by majidvp@google.com, Jul 31 2017

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Jul 31 2017

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

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


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

android-nexus5X
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Jul 31 2017

Cc: sunn...@chromium.org
Owner: sunn...@chromium.org

=== 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

Comment 4 by crouleau@google.com, Jul 31 2017

Cc: sande...@chromium.org piman@chromium.org jbau...@chromium.org
Labels: OS-Android
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.

Comment 5 by crouleau@google.com, Jul 31 2017

Labels: -Pri-2 Pri-1
Status: Assigned (was: Untriaged)
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.
This CL can't catch a break, apparently. Let me know if there is a way I can help at all!
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).
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.)
It is not fullscreen.

Feel free to run the test yourself. The command is listed in #3.
 Issue 750914  has been merged into this issue.
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.
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.
Project Member

Comment 13 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)
Seems to have recovered. I've started a bisect to confirm.

=== 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
Status: Verified (was: Fixed)
good == bad in the above bisect

Sign in to add a comment