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

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


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

android-nexus5
android-nexus6
android-nexus7v2
android-webview-nexus6
linux-release

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8958197340589164384

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

Hi gbiv@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 : George Burgess IV
  Commit : 274d75742451914f74bbad40c405da7f902dd232
  Date   : Wed Jan 03 23:22:46 2018
  Subject: Turn on AFDO for Chromium on Android

Bisect Details
  Configuration: android_nexus5_perf_bisect
  Benchmark    : rasterize_and_record_micro.top_25
  Metric       : rasterize_time/file___static_top_25_pinterest.html
  Change       : 38.37% | 6.064 -> 8.39066666667

Revision             Result                    N
chromium@526759      6.064 +- 0.118575         6      good
chromium@526826      6.01433 +- 0.0837934      6      good
chromium@526843      6.0095 +- 0.118108        6      good
chromium@526851      6.02 +- 0.0892412         6      good
chromium@526855      6.0255 +- 0.0750566       6      good
chromium@526857      6.02217 +- 0.153404       6      good
chromium@526858      8.40083 +- 0.0211857      6      bad       <--
chromium@526859      8.372 +- 0.0579483        6      bad
chromium@526892      8.39067 +- 0.0750689      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=file...static.top.25.pinterest.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/8958197340589164384


For feedback, file a bug with component Speed>Bisection

Ack; thanks.

Since the AFDO patch is in part meant to tease regressions like this out, I plan to leave it in over the weekend. We'll see what other bots get angry at me, and go from there. :)

 Issue 799636  has been merged into this issue.

 Issue 799630  has been merged into this issue.

 Issue 799641  has been merged into this issue.

 Issue 800060  has been merged into this issue.

 Issue 800052  has been merged into this issue.

 Issue 800100  has been merged into this issue.

 Issue 800099  has been merged into this issue.

 Issue 800058  has been merged into this issue.

Hi,

How do I go about selectively disowning some of these regressions? e.g. Mac/Windows/Linux shouldn't have been touched by AFDO, and some of the Android regressions in this list seem to have appeared before the AFDO CL landed.

I'd click the little red Xes, but I'm unsure if that drops the regression on the floor, or if it'll make the bots look at it again.

Thanks!

Hi gbiv@:

To disassociate any test which seems unrelated to your CL,
- go to https://chromeperf.appspot.com/group_report?bug_id=799629
- click on the numbers on the leftmost column to expand all rows in all groups (and then possibly sort by the "Bot" column)
- click the 'x' next to the bug #.

Got it. Thanks again! :)

I think we more or less converged on blink_perf being too micro-benchmarky to optimize for. I would not worry about them. Smoothness/rasterize/thread_times - would need a closer look IMO.

Sounds good.

I'm very actively looking at this, but because the branch point is looming: are these regressions bad enough that we should pull the AFDO patch out, or is it something we want to keep despite the regressions?

(In either case, I think it's highly likely that we can work around these regressions, maybe at the cost of a bit of binary size. I assume that if we end up doing that, we could punt the fixes into the M65 branch)

To keep this up-to-date, we decided to keep AFDO in for the branch, despite these regressions. I tried gathering a profile of these benchmarks + applying it to Chrome, and it did help out a bit for some regress metrics, but it actually caused us to regress others more badly.

So, I assume we're hitting some highly Android (or ARM) specific code at some point. If that is the case, we can turn the perf-regressing (and size-improving) options off for that specific piece of code, and see what happens. Stay tuned. :)

Narrowed this down to AFDO on Skia and Blink.

tl;dr: I plan to mark a few pieces of Skia as "inaccurate" for AFDO. I could do a similar thing for blink, but it may interact poorly with ThinLTO (which should hopefully be coming soon), so I'm going to wait on that for a bit, in hopes that I'm not fixing it just to regress 5 things just like it in a different way...

CL: https://chromium-review.googlesource.com/c/chromium/src/+/917201

We have two ways of fixing these: marking AFDO as inaccurate, and just turning AFDO off. "Inaccurate" AFDO means size regressions that come with potential speedups, and turning AFDO off can work if our profiles are very incorrect.

Marking Skia inaccurate gives us the best results on everything but thread_times.tough_scrolling_cases locally, but adds 428KB to libchrome.so. If we split Skia up a bit and only mark the core, effects, and opts parts as inaccurate, we see almost the same speedup for 212KB. This alone gets rid of (or at least heavily mitigates) most regressions, except for thread_times.tough_scrolling_cases/thread_raster_cpu_time_per_frame.

thread_times.tough_scrolling_cases/thread_raster_cpu_time_per_frame is helped a bit by Skia (+290% regression -> +248% regression), but nowhere near as much as removing AFDO for blink. It looks like ThinLTO is going to regress it further (e.g. our regression in the absolute worst case will go from +248% to +384% with ThinLTO on, compared to a non-AFDO build). On the bright side, thread_raster_cpu_time is a fraction of the total CPU time, which is overall a wash with AFDO + these Skia changes, and improved overall with ThinLTO.

In any case, the full results of the benchmarks are attached. "ARMFresh" is just your standard ARM release build; "ARMFreshAFDO" is ARMFresh + AFDO on; "ARMFreshAFDOSkiaExperimental" is ARM + AFDO + CL:917201 applied; and "ARMFreshAFDOSkiaExperimentalThinLTO" is that, but with ThinLTO, too.

Deleted: results_799629.tar.gz 3.6 MB

results_799629.tar.gz 3.6 MB Download

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

commit 2e17642e132375121818223a87d8eb02efbf4004
Author: George Burgess IV <gbiv@chromium.org>
Date: Thu Feb 15 00:24:46 2018

Split AFDO into its own config; make skia use inaccurate AFDO

This patch moves clang's AFDO into the "afdo" config, and pushes the
-fprofile-sample-accurate flag into an "afdo_accurate" config.

As discussed in the bug, the regressions we're seeing are heavily because
we're applying 'accurate' AFDO to both Skia and Blink. Marking profiles
inaccurate for all of Skia costs >400KB binary size, which is
undesirable. If we instead split a few particularly hot bits out, we can
see most of the benefit for ~half the binary size increase.

While the split of clang's AFDO into the afdo config isn't strictly
necessary, it probably doesn't hurt, and will help experimentation
with issues like this in the future, if they arise.

Bug: 799629
Test: Ran benchmarks; scores are better (as noted in the bug)
Change-Id: I4f1f8767cfb70fcc733ab468fb53347857d2fd98
Reviewed-on: https://chromium-review.googlesource.com/917201
Commit-Queue: George Burgess <gbiv@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Mike Klein <mtklein@chromium.org>
Reviewed-by: Egor Pasko <pasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536905}
[modify] https://crrev.com/2e17642e132375121818223a87d8eb02efbf4004/base/allocator/BUILD.gn
[modify] https://crrev.com/2e17642e132375121818223a87d8eb02efbf4004/build/config/BUILDCONFIG.gn
[modify] https://crrev.com/2e17642e132375121818223a87d8eb02efbf4004/build/config/compiler/BUILD.gn
[modify] https://crrev.com/2e17642e132375121818223a87d8eb02efbf4004/build/config/compiler/compiler.gni
[modify] https://crrev.com/2e17642e132375121818223a87d8eb02efbf4004/skia/BUILD.gn