New issue
Advanced search Search tips

Issue 738085 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

5.6%-69.5% regression in media_perftests at 480054:480179

Project Member Reported by wolenetz@chromium.org, Jun 29 2017

Issue description

These 5 alerts look valid, but there is no ref trace. Bisecting...
 
Cc: crouleau@chromium.org
Paraphrasing chat with crouleau@ w.r.t. missing ref trace for media_perftests:
These tests may have had ref traces before the switch to recipes, but now that's broken. See bug 714812 (it'd be great if that were fixed, otherwise bisect is our best tool, assuming bisect succeeds).
Project Member

Comment 4 by 42576172...@developer.gserviceaccount.com, Jun 29 2017

Cc: p...@chromium.org
Owner: p...@chromium.org

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

Hi pcc@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 : pcc
  Commit : 5cb9983f12f300422c16ba812c2d5cdeb278cb74
  Date   : Fri Jun 16 20:13:24 2017
  Subject: Enable ThinLTO for POSIX LTO by default on Linux.

Bisect Details
  Configuration: linux_perf_bisect
  Benchmark    : media_perftests
  Metric       : audio_converter/convert
  Change       : 4.87% | 145292.233708 -> 138220.511887

Revision             Result                 N
chromium@480053      145292 +- 519.336      6      good
chromium@480116      144511 +- 4465.7       6      good
chromium@480132      145085 +- 1384.66      6      good
chromium@480140      145334 +- 569.72       6      good
chromium@480142      144241 +- 5550.62      6      good
chromium@480143      145462 +- 306.456      6      good
chromium@480144      138138 +- 195.584      6      bad       <--
chromium@480148      138087 +- 835.455      6      bad
chromium@480179      138221 +- 422.858      6      bad

To Run This Test
  ./src/out/Release/media_perftests --single-process-tests

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

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


For feedback, file a bug with component Speed>Bisection

Comment 5 by p...@chromium.org, Jun 29 2017

Status: WontFix (was: Untriaged)
I think this is to be expected. In the configuration we use, ThinLTO runs a slightly different set of compiler passes to regular LTO, so the generated code will be slightly different. By the law of averages we can expect to see one or two regressions (and in this case there seems to be just one).

So I think there's nothing to do here.
Cc: dalecur...@chromium.org
+dalecurtis for audio expertise: please reopen if this metric is important enough to fix this, otherwise ignore.
These are pretty dramatic drops (~50%) in auto-vectorized C code. It's possible it doesn't matter in the macro scale. When compiled with clang we disable our hand-rolled intrinsics since clang used to do a better job:

https://chromeperf.appspot.com/report?sid=76ab05441018ef0637f83d680ece0918493f60caa468e7eb1063b39591f718cd

It looks like OSX + clang is still chugging along with equivalent performance on C vs SIMD. However linux has taken a nose dive since this change; so it seems we should switch back to the SIMD variants for linux + clang. Or is there a #define set when thin lto is in use?

Comment 8 by p...@chromium.org, Jun 29 2017

Status: Started (was: WontFix)
Taking a closer look at the ThinLTO pass pipeline, it looks like auto-vectorisation is being disabled entirely in our configuration, which seems consistent with your observation, and probably not what we want long term.

I will see if this can be addressed in the compiler somehow, but in the meantime I agree that we should switch back to SIMD when ThinLTO is enabled. There's no #define specifically for ThinLTO, but we could add our own here: https://cs.chromium.org/chromium/src/build/config/compiler/BUILD.gn?l=487
Thanks for taking a closer look. If we expect auto-vectorization to be resolved before the M61 branch point (Mid/Late July) I don't think it's worth adding a workaround for. If you don't think that'll happen we can switch it over with a OS_LINUX define in the meantime; the delta is fairly small between properly auto-vectorized and SIMD so it's fine to do this for all linux platforms.

Comment 10 by p...@chromium.org, Jun 29 2017

I wouldn't count on it being resolved by then. Since this would likely involve a change to the pass pipeline, it would need to be agreed to by other LLVM developers, and then the change would need to be rolled into our version of clang without causing any other issues which might require rollbacks.

It's unclear whether all of that will happen in time for the branch. I also can't promise making this a priority. So I'd suggest going for the workaround.
Hmm, I'd expect breaking auto-vectorization to be a significant perf loss in V8 perf benchmarks and hence a release blocker. Is this not the case?

In any case I'll prepare a small patch for this.

Comment 12 by p...@chromium.org, Jun 29 2017

It doesn't appear so. The perf dashboard for my change does not show any perf regressions on Linux other than for media_perftests: https://chromeperf.appspot.com/group_report?rev=480144

Maybe there's an unmonitored benchmark that I should be looking at, but then I'd expect monitoring on any benchmarks where regressions may turn into release blockers.
Project Member

Comment 13 by bugdroid1@chromium.org, Jun 30 2017

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

commit b5ac0cf6f7b0c31794f6e93d60193721830fb504
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Fri Jun 30 01:01:54 2017

Switch to intrinsics for vector_math primitives in clang on Linux.

The recent enablement of ThinLTO on Linux has broken the
auto vectorizer in clang, so switch back to using our intrinsics
based version for now.

BUG= 738085 
TEST=none

Change-Id: I9f4b182adf20c090e4933bbf671de3b1b5043147
Reviewed-on: https://chromium-review.googlesource.com/557059
Reviewed-by: Peter Collingbourne <pcc@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483567}
[modify] https://crrev.com/b5ac0cf6f7b0c31794f6e93d60193721830fb504/media/base/vector_math.cc

Owner: dalecur...@chromium.org
+Dale, could you please resolve this bug? Is it fixed?
Status: Fixed (was: Started)
It was tracking ThinLTO issues, but we can close this one as fixed. I've split the ThinLTO issues into issue 768418.

Sign in to add a comment