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

Issue 770832 link

Starred by 3 users

Issue metadata

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



Sign in to add a comment

6.3%-24.1% regression in rasterize_and_record_micro.top_25 at 505470:505503

Project Member Reported by majidvp@chromium.org, Oct 2 2017

Issue description

Grouped with regressions in: 
 -
 thread_times.tough_scrolling_cases/thread_raster_cpu_time_per_frame
 - smoothness.tough_animation_cases

which I think are related.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=770832

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


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

chromium-rel-mac11-air
chromium-rel-win7-dual
Cc: jsc...@chromium.org
Owner: jsc...@chromium.org
Status: Assigned (was: Untriaged)

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

Hi jschuh@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 : Justin Schuh
  Commit : 3725ec39917fea4cc350f0abb8dde4552e955054
  Date   : Sun Oct 01 03:44:27 2017
  Subject: Make ClampedNumeric and CheckedNumeric constexpr

Bisect Details
  Configuration: win_perf_bisect
  Benchmark    : rasterize_and_record_micro.top_25
  Metric       : record_time/file___static_top_25_twitter.html
  Change       : 27.84% | 0.111333333333 -> 0.142333333333

Revision             Result                       N
chromium@505470      0.111333 +- 0.0011547        6      good
chromium@505487      0.1125 +- 0.003937           6      good
chromium@505491      0.111667 +- 0.0011547        6      good
chromium@505493      0.112333 +- 0.00522813       6      good
chromium@505494      0.143333 +- 0.00416333       6      bad       <--
chromium@505495      0.142167 +- 0.000912871      6      bad
chromium@505503      0.142333 +- 0.0011547        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=file...static.top.25.twitter.html rasterize_and_record_micro.top_25

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

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


For feedback, file a bug with component Speed>Bisection
The MSVC performance regression isn't surprising, and should be fixed by reordering the saturation calculation. The Mac one doesn't seem like it should be related, since Clang takes the intrinsic path.
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 3 2017

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

commit ffc71c032540fc188f8b8104b1b26387d3e18a1b
Author: Justin Schuh <jschuh@chromium.org>
Date: Tue Oct 03 17:05:26 2017

Reorder saturation calculation for ClampAddOp and ClampSupOp

The order of operations changed in crrev.com/505494 with C++14
constexpr optimizations. However, that change appears to have regressed
performance in some cases. So, this CL tweaks the code to match the
old structure and trigger the same compiler optimization heuristics.

Bug:  770832 
Change-Id: I28f57ddf53825619eb73c9a92c663fc8463e16a7
Reviewed-on: https://chromium-review.googlesource.com/696721
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Justin Schuh <jschuh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506078}
[modify] https://crrev.com/ffc71c032540fc188f8b8104b1b26387d3e18a1b/base/numerics/clamped_math_impl.h

Cc: e...@chromium.org wangxianzhu@chromium.org
 Issue 771197  has been merged into this issue.
Cc: hablich@chromium.org
 Issue 771528  has been merged into this issue.
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 4 2017

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

commit 95a989c109f41b856fba698bd0c96bfcd69f78d9
Author: Justin Schuh <jschuh@chromium.org>
Date: Wed Oct 04 23:24:37 2017

Revert "Reorder saturation calculation for ClampAddOp and ClampSupOp"

This reverts commit ffc71c032540fc188f8b8104b1b26387d3e18a1b.

Reason for revert: Didn't improve the perf issue.

Original change's description:
> Reorder saturation calculation for ClampAddOp and ClampSupOp
> 
> The order of operations changed in crrev.com/505494 with C++14
> constexpr optimizations. However, that change appears to have regressed
> performance in some cases. So, this CL tweaks the code to match the
> old structure and trigger the same compiler optimization heuristics.
> 
> Bug:  770832 
> Change-Id: I28f57ddf53825619eb73c9a92c663fc8463e16a7
> Reviewed-on: https://chromium-review.googlesource.com/696721
> Reviewed-by: Tom Sepez <tsepez@chromium.org>
> Commit-Queue: Justin Schuh <jschuh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#506078}

TBR=jschuh@chromium.org,tsepez@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  770832 
Change-Id: I076c93215b422e321ad4bf39a72bab144cc0195d
Reviewed-on: https://chromium-review.googlesource.com/700775
Reviewed-by: Justin Schuh <jschuh@chromium.org>
Commit-Queue: Justin Schuh <jschuh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506559}
[modify] https://crrev.com/95a989c109f41b856fba698bd0c96bfcd69f78d9/base/numerics/clamped_math_impl.h

Cc: jarin@google.com jarin@chromium.org
 Issue 772279  has been merged into this issue.
 Issue 772279  has been merged into this issue.
 Issue 772279  has been merged into this issue.
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 12 2017

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

commit 5ec7403b4e7843336ec726a4668ae78cc5ed53e2
Author: Justin Schuh <jschuh@chromium.org>
Date: Thu Oct 12 23:58:42 2017

Simplify ClampedSubOp and ClampedAddOp

This is a speculative change to track down a potential performance
regression on MSVC. It reverts changes from crrev.com/505494 such that
the remaining change has no other effect than applying constexpr.

Bug:  770832 
Change-Id: Ib700270bbf59002336f16b63acbb3b91c00a24dd
Reviewed-on: https://chromium-review.googlesource.com/711114
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Commit-Queue: Justin Schuh <jschuh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508551}
[modify] https://crrev.com/5ec7403b4e7843336ec726a4668ae78cc5ed53e2/base/numerics/clamped_math_impl.h

The MSVC regression is fixed (and now constexpr appears to be providing a performance win on average). My best guess for what happened is that the MSVC optimizer just wasn't doing a good job of folding the compile-time constants in the more complex saturation computation.

The mac-air11 regression just doesn't make any sense to me, and I don't think is related to my change, because clang is doing the right thing everywhere else. So, unless anyone has any bright ideas I'm going to close this.
Status: Fixed (was: Assigned)

Sign in to add a comment