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

Issue 708252 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug



Sign in to add a comment

10-20% regression in CalcDrawProps time

Project Member Reported by ajuma@chromium.org, Apr 4 2017

Issue description

CalcDrawProps time has regressed by ~15% on Android Dev. Creating this bug so we can run bisects on the perfbots.
 

=== BISECT JOB RESULTS ===
NO Perf regression found

Bisect Details
  Configuration: linux_perf_bisect
  Benchmark    : cc_perftests
  Metric       : calc_draw_props_time/10_10

Revision             Result                  N
chromium@457298      22.145 +- 1.02685       21      good
chromium@458255      22.1382 +- 1.04603      21      bad

To Run This Test
  ./src/out/Release/cc_perftests --test-launcher-print-test-stdio=always --verbose

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8983218608438517984

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5816063981256704


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

=== BISECT JOB RESULTS ===
NO Perf regression found

Bisect Details
  Configuration: linux_perf_bisect
  Benchmark    : cc_perftests
  Metric       : calc_draw_props_time/10_10

Revision             Result                  N
chromium@457953      25.0974 +- 2.14526      21      good
chromium@458590      25.0659 +- 2.42065      21      bad

To Run This Test
  ./src/out/Release/cc_perftests --test-launcher-print-test-stdio=always --verbose

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8983149733464493072

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5891943436910592


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

Comment 6 by ajuma@chromium.org, Apr 7 2017

Description: Show this description
Cc: jaydasika@chromium.org
Owner: jaydasika@chromium.org

=== Auto-CCing suspected CL author jaydasika@chromium.org ===

Hi jaydasika@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 : jaydasika
  Commit : 3a6b144350b8c18f94c48143414031627572dd41
  Date   : Tue Mar 21 23:11:19 2017
  Subject: cc : Clean up cc clip tree

Bisect Details
  Configuration: linux_perf_bisect
  Benchmark    : cc_perftests
  Metric       : calc_draw_props_time/10_10
  Change       : 9.01% | 24.8878240585 -> 27.1309124629

Revision             Result                   N
chromium@457953      24.8878 +- 0.242215      6      good
chromium@458363      24.9003 +- 0.704082      6      good
chromium@458568      24.625 +- 0.782547       9      good
chromium@458594      25.1447 +- 2.72792       9      good
chromium@458596      25.4285 +- 4.61899       9      good
chromium@458597      27.3514 +- 0.488557      9      bad       <--
chromium@458598      27.4423 +- 0.393615      9      bad
chromium@458601      27.3568 +- 0.842382      9      bad
chromium@458607      27.4433 +- 0.695389      9      bad
chromium@458620      27.4505 +- 0.729159      9      bad
chromium@458671      27.4694 +- 0.36264       6      bad
chromium@458773      27.4763 +- 0.343013      6      bad
chromium@459592      27.1309 +- 0.30064       6      bad

To Run This Test
  ./src/out/Release/cc_perftests --test-launcher-print-test-stdio=always --verbose

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8983034680766855984

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5812691559514112


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

Comment 8 by ajuma@chromium.org, Apr 7 2017

Cc: weiliangc@chromium.org
Summary: 10-20% regression in CalcDrawProps time (was: Regression in CalcDrawProps time)
The regression is about 20% at the 95th percentile on Android Dev according to UMA, and about 10% according to the bisect in #7.

Comment 9 by ajuma@chromium.org, Apr 7 2017

Components: Internals>Compositing
Labels: -Pri-3 M-59 Pri-1
Cc: chrishtr@chromium.org enne@chromium.org
Owner: ----
Status: Available (was: Assigned)
This regression is from the new clips cache patch. We did expect a little (5-10%) regression in CDP time with that patch based on experiments on cluster telemetry but not a 20% regression. But the 5-10% estimation was on the average and not on the 95th percentile.

I am unsure if I will be able to look at this. So, changing the status to Available.
Owner: enne@chromium.org
Status: Assigned (was: Available)
Enne noticed one thing which might be improved, sending to them for next
step.
Project Member

Comment 12 by bugdroid1@chromium.org, Jun 6 2017

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

commit 3b1aebb2d7e44f3e5d03dadb62b1f67a42612519
Author: Adrienne Walker <enne@chromium.org>
Date: Tue Jun 06 01:10:43 2017

Speed up property tree clip map caching

This looks like a ~3% win on Linux for cdp speed by removing the
heap allocation and storing cached vectors local to the clip node.
I don't know if this will fix the entire regression, but seems
faster.

Other things tried locally were having a global map and a global
vector of clips, and neither of these were huge wins.

Bug:  708252 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I8df7b92535c010cf21de56199bf3f731cc004b44
Reviewed-on: https://chromium-review.googlesource.com/506467
Commit-Queue: enne <enne@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477138}
[modify] https://crrev.com/3b1aebb2d7e44f3e5d03dadb62b1f67a42612519/cc/trees/clip_node.cc
[modify] https://crrev.com/3b1aebb2d7e44f3e5d03dadb62b1f67a42612519/cc/trees/clip_node.h
[modify] https://crrev.com/3b1aebb2d7e44f3e5d03dadb62b1f67a42612519/cc/trees/draw_property_utils.cc
[modify] https://crrev.com/3b1aebb2d7e44f3e5d03dadb62b1f67a42612519/cc/trees/property_tree.cc

Comment 13 by enne@chromium.org, Sep 19 2017

Status: Fixed (was: Assigned)
https://chromeperf.appspot.com/report?sid=ad5f4878b439567079570ef5c376ea2037658fc979113f6941f4bed35be29000&start_rev=441040&end_rev=502251

This change fixed the original regression.  Additionally switching from gcc to clang was a similar win.

Sign in to add a comment