=== Auto-CCing suspected CL author fmalita@chromium.org ===
Hi fmalita@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 : Florin Malita
Commit : 4d3622e28d891b3a445275d132a6817bb83973ed
Date : Wed Jul 12 20:02:25 2017
Subject: Remove flag to stop relying on Skia's bilerp-hack
Bisect Details
Configuration: win_perf_bisect
Benchmark : smoothness.top_25_smooth
Metric : first_gesture_scroll_update_latency/https___www.google.com__hl_en_q_barack+obama
Change : 12.23% | 15.4445 -> 17.3336666667
Revision Result N
chromium@486037 15.4445 +- 0.0985571 6 good
chromium@486053 15.397 +- 0.0803243 6 good
chromium@486061 15.4008 +- 0.0601567 6 good
chromium@486065 15.4178 +- 0.103416 6 good
chromium@486067 15.3892 +- 0.0547616 6 good
chromium@486068 17.3627 +- 0.0782134 6 bad <--
chromium@486099 17.3418 +- 0.0580589 6 bad
chromium@486161 17.3337 +- 0.0786469 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=https...www.google.com..hl.en.q.barack.obama smoothness.top_25_smooth
More information on addressing performance regressions:
http://g.co/ChromePerformanceRegressions
Debug information about this bisect:
https://chromeperf.appspot.com/buildbucket_job_status/8973807571203713872
For feedback, file a bug with component Speed>Bisection
[Bulk Edit]
URGENT - PTAL.
Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch ASAP. Thank you!
Know that this issue shouldn't block the release? Remove the ReleaseBlock-Stable label or move to M62.
I think I see what's going on, but there's an unfortunate overlap which makes this tricky to recover. Looking at ESPN in particular (https://chromeperf.appspot.com/report?sid=52f7558b3e39560d90b6f9d742ed2d1bc47fb37a2abdd5605ed75c3db5cacf40), there are three significant changes:
1) https://chromium-review.googlesource.com/567267 [486068] - Remove flag to stop relying on Skia's bilerp-hack
This removes an old Skia heuristic to ignore subpixel image filtering in the absence of other transform components. The motivation is to align the behavior with Ganesh, and improve correctness.
As an unintended side effect, removing the heuristic also causes Skia to start filtering unnecessarily when the translation is integral. This is reflected as an 88% regression in rasterize_time/ESPN.
2) https://chromium-review.googlesource.com/575675 [487507] - Enable Skia's integral-translate bilerp optimization
This enables an attempted fix for the regression, by ignoring bilerp with integral translations. Unfortunately (current finding), the fix only works for clamp/clamp tiling -- for anything else we adjust the matrix scale before the integral check, defeating the fast path.
The change is reflected as a 7.7% progression in rasterize_time/ESPN.
3) https://chromium-review.googlesource.com/545816 [488431] - Enable Skia rasterpipeline for tiling
This switches asymmetric tiling (clamp/repeat, repeat/clamp) to Skia's new SIMD raster pipeline. The CL is reflected as an 18% progression in rasterize_time/ESPN, because the raster pipeline impl is checking for integral translations independently, and skipping bilerp.
I have a fix to cover the remainder of the regression from #1 (https://skia-review.googlesource.com/c/34840), but, since we're now using raster pipeline for asymmetric tiling, if doesn't help much at ToT. I.e. non-filtering RP is faster than filtering legacy, but slower than non-filtering legacy.
I think the fix makes sense on its own, and it'll probably show in the metrics, but to fully recover something like ESPN mixing repeat/no-repeat we'd have to land the fix AND revert #3 (which is tricky because IINM the support legacy Skia code is already gone).
Per offline chat, this seems to be significant regression and Stable blocker.
Please request a merge to M61 once change is well baked/verified in Canary and safe to merge to M61.
Please note we're only few weeks away from M61 Stable promotion so merge bar is VERY high.
After 494882, of the two tracked regressions related to https://chromium-review.googlesource.com/567267, LinkedIn recovered past the initial level (possibly due to overlap with mtklein's https://skia-review.googlesource.com/34746) while ESPN improved -- but is still ~33% in the red: https://chromeperf.appspot.com/report?sid=91b14e8cee481267b842ed142f64616cbccd4c77f3c10e80074fc4164d242401
I think merging the fix is a no-brainer, once it soaked in canary.
The question is what to do about #3 in c#14. I now believe it is still possible to revert cleanly and get back to baseline times (supporting Skia code not deleted yet), but, unlike the other fixes, doing so imposes an opportunity cost for Skia.
I'm tempted to stop here, wait for the cumulative metrics to pick up the existing fixes (while focusing on improving raster-pipeline on ToT), and then re-assess. But I could easily be swayed otherwise.
Before we approve merge to M61, please answer followings:
*Are the changes listed at #24 well baked/verified in Canary, having enough automation tests coverage and safe to merge to M61?
* Any other important details to justify the merge.
Please note We're only few weeks away from M61 Stable promotion, so merge bar is very high. Thank you.
This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop)
For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
The perf regressions in this issue and 753905 have been deemed serious enough to be release blockers. See also c#12.
https://skia-review.googlesource.com/34840 has been in Canary/ToT for a few days, verified perf-wise on the affected bots, and localized/safe to merge.
https://skia-review.googlesource.com/36120 is more recent, but verified on the affected perf bots and effectively just a revert of https://chromium-review.googlesource.com/545816 (hence also safe to merge).
Both changes affect image rasterization, for which we have plenty of layout test coverage.
I can wait with the actual merge until Monday, but trying to get approval/line things up in advance so we don't land too close to m61 stable.
Thanks!
Comment 1 by 42576172...@developer.gserviceaccount.com
, Jul 17 2017