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

Issue 759554 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug-Regression



Sign in to add a comment

1239% regression in thread_times.key_silk_cases at 497554:497661

Project Member Reported by kraynov@chromium.org, Aug 28 2017

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Aug 28 2017

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

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


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

android-webview-nexus6
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Aug 28 2017

Cc: sunn...@chromium.org
Owner: sunn...@chromium.org
Status: Assigned (was: Untriaged)

=== 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 : 9b8fb34db903643b759635086dcf0da90b1e3082
  Date   : Sat Aug 26 00:49:56 2017
  Subject: gpu: Command buffer multi flush.

Bisect Details
  Configuration: android_webview_nexus6_aosp_perf_bisect
  Benchmark    : thread_times.key_silk_cases
  Metric       : thread_GPU_cpu_time_per_frame/http___jsbin.com_UVIgUTa_38_quiet
  Change       : 1175.61% | 0.0169965633671 -> 0.216809052605

Revision             Result                        N
chromium@497553      0.0169966 +- 0.00336546       6      good
chromium@497607      0.00621899 +- 0.00168095      6      good
chromium@497614      0.0058753 +- 0.000946059      6      good
chromium@497615      0.218639 +- 0.0113983         6      bad       <--
chromium@497616      0.218331 +- 0.00906629        6      bad
chromium@497618      0.218995 +- 0.0121937         6      bad
chromium@497621      0.219154 +- 0.0101044         6      bad
chromium@497634      0.217932 +- 0.00817639        6      bad
chromium@497661      0.216809 +- 0.00765482        6      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-webview --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=http...jsbin.com.UVIgUTa.38.quiet thread_times.key_silk_cases

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8970010842110955184


For feedback, file a bug with component Speed>Bisection
Project Member

Comment 4 by 42576172...@developer.gserviceaccount.com, Aug 28 2017

 Issue 759572  has been merged into this issue.
Project Member

Comment 5 by 42576172...@developer.gserviceaccount.com, Aug 28 2017

 Issue 759578  has been merged into this issue.
Project Member

Comment 6 by 42576172...@developer.gserviceaccount.com, Aug 28 2017

 Issue 759595  has been merged into this issue.
Project Member

Comment 7 by 42576172...@developer.gserviceaccount.com, Aug 28 2017

 Issue 759652  has been merged into this issue.
Labels: -Pri-2 Pri-1
Project Member

Comment 9 by 42576172...@developer.gserviceaccount.com, Aug 29 2017

Cc: pmeenan@chromium.org
 Issue 760176  has been merged into this issue.
Project Member

Comment 10 by 42576172...@developer.gserviceaccount.com, Aug 29 2017

 Issue 760176  has been merged into this issue.
Project Member

Comment 11 by 42576172...@developer.gserviceaccount.com, Aug 29 2017

 Issue 760177  has been merged into this issue.
Project Member

Comment 12 by 42576172...@developer.gserviceaccount.com, Aug 29 2017

 Issue 760178  has been merged into this issue.
Project Member

Comment 13 by 42576172...@developer.gserviceaccount.com, Aug 30 2017

Cc: u...@chromium.org
 Issue 760525  has been merged into this issue.
Project Member

Comment 14 by bugdroid1@chromium.org, Aug 30 2017

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

commit c7a7949c958b4867600ca716968da430fd9b952f
Author: Sunny Sachanandani <sunnyps@chromium.org>
Date: Wed Aug 30 22:21:33 2017

cc: Flush command buffers when all tiles are done.

Command buffer multi flush requires explicit flushing of ordering
barriers. Flushing in PrepareSendToParent is not sufficient because
that will not be called when there's no damage.

Flush when all tiles are done and we have no other work. This fixes the
thread times regressions at least in thread_times.key_silk_cases
list_animation_simple.html on Nexus 6. Also remove the flush in
PrepareSendToParent as that's not required.

The regressions were caused by not marking tiles as ready to draw, and
then evicting those tiles because they weren't used. This would cause
tiles to be rastered over and over again.

R=piman
BUG= 759554 

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I66827a6686b1ebb396ff69ae0f4e40d6db29c63f
Reviewed-on: https://chromium-review.googlesource.com/641381
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498640}
[modify] https://crrev.com/c7a7949c958b4867600ca716968da430fd9b952f/cc/resources/layer_tree_resource_provider.cc
[modify] https://crrev.com/c7a7949c958b4867600ca716968da430fd9b952f/cc/tiles/tile_manager.cc

Project Member

Comment 15 by 42576172...@developer.gserviceaccount.com, Aug 30 2017

 Issue 760681  has been merged into this issue.
Cc: petermarshall@chromium.org
 Issue 761740  has been merged into this issue.
Cc: briander...@chromium.org
 Issue 762210  has been merged into this issue.
sunnyps: is this fixed? It looks like it recovers at r499845, but not at the CL in #14.
Status: Fixed (was: Assigned)
That was a followup fix for the same underlying issue since the CL in #14 wasn't sufficient.

Sign in to add a comment