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

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICguLGopgoM


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

android-nexus6

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

Hi primiano@chromium.org, the bisect results pointed to your CL below as possibly
causing a regression. Please have a look at this info and see whether
your CL be related.


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Reland of Enable allocator shim for Android (crrev.com/1875043003)
Author  : primiano
Commit description:
  
Reason for reland:
The original CL was reverted by crrev.com/1881523003 because it broke
clang builds. This CL contains the fix for clang (see diff between
patch-set 1 and 2).
The culprit of the fix is that LLVM libc++ uses _NOEXCEPT instead
of glibc's __THROW. The absence of it was causing errors like the
following:

../../base/allocator/allocator_shim_override_cpp_symbols.h:20:25: error: function previously declared with an explicit exception specification redeclared with an implicit exception specification [-Werror,-Wimplicit-exception-spec-mismatch]
SHIM_ALWAYS_EXPORT void operator delete(void* p) __THROW

Original issue's description:
> Enable allocator shim for Android
>
> This is a follow-up to crrev.com/1719433002, which introduced the
> shim for Android, and enables it by default by setting
> use_experimental_allocator_shim=true for Android.
>
> Build/Perf sheriffs heads up
> ----------------------------
> If you see any build error or crash related with __wrap_malloc,
> __wrap_free, __real_malloc, __real_free, etc this CL is to blame.
>
> Performance considerations
> ------------------------
> Binary size diff (GN, arm, static, official build): 24k
>
> I did a mixture of local and trybots run to estimate the perf impact
> of this change. Didn't get any conclusive data, everything I tried
> seems in the same ballpark, below noise levels. More in details:
>
> cc_perftests.PrepareTiles on a Nexus 4.
> Rationale of the choice: in a previous CL ( crbug.com/593344 ), this
> benchmark revealed the presence of two mfences in the malloc path.
> Results: https://goo.gl/8VC3Jp in the same ballpark.
>
> page-cycler on Nexus 9 via trybots:
> Results: http://goo.gl/J3i50a seems to suggest that this CL improves
> both warm and cold times in most cases. I doubt it, more likely it's
> noise.
>
> All the other perf trybots failed. The perf waterfall seems to be in a
> bad state in these days.
>
> BUG=550886,598075
> TEST=base_unittests --gtest_filter=AllocatorShimTest.*
> TBR=thakis@chromium.org
>
> Committed: https://crrev.com/ebb95496c73dc0d5ce83968ac619921f154305f7
> Cr-Commit-Position: refs/heads/master@{#386386}

BUG=550886,598075
CC=dalecurtis@chromium.org

Review URL: https://codereview.chromium.org/1875173002

Cr-Commit-Position: refs/heads/master@{#386444}
Commit  : 88bf797383cd290d9bd23db0045c5fb23e010beb
Date    : Mon Apr 11 19:51:08 2016


===== TESTED REVISIONS =====
Revision                Mean Value  Std. Dev.   Num Values  Good?
chromium@386410         0.5514      0.000563    5           good
chromium@386437         0.551931    0.001691    5           good
chromium@386441         0.551883    0.002161    5           good
chromium@386443         0.550469    0.001313    5           good
chromium@386444         0.567883    0.001512    5           bad         <-
chromium@386451         0.567566    0.000609    5           bad
chromium@386464         0.5668      0.000899    5           bad

Bisect job ran on: android_nexus6_perf_bisect
Bug ID: 604016

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --also-run-disabled-tests rasterize_and_record_micro.key_silk_cases
Test Metric: record_time/record_time
Relative Change: 2.79%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus6_perf_bisect/builds/2137
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9015283961570415440


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=604016

| 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 Tests>AutoBisect.  Thank you!

+vmpstr (owner of the benchmark), +sullivan, +rschoen

So here's the situation: it is very likely that my change can have caused that regression. However, that is a pretty across-the-board change, as it changes the way malloc() are handled on Android (which is the only platform on which we didn't previously had a malloc interposition). 
I looked into the generated assembly before doing that change to make sure it was as less perf-intrusive as possible (which made me find  Issue 593344 ). I can't possibly do something more just for this benchmark.

My arguments here are:

1) I looked at various benchmarks after landing my change, and couldn't spot any general benchmark (pagecyclers and such) affected by this change. See
https://chromeperf.appspot.com/report?sid=bae6c680b06a1869ef1a79fca7677afb90e3ff38ad27b4f30f6ce6b8495de961&start_rev=386000&end_rev=386667
(The first reland was on #386386, which is in the middle of that range)
This makes me believe that this is affecting only micro benchmarks.

2) This micro benchmark on Nexus 6 seems the only thing that triggered an alert. For the same benchmark couldn't spot anything on N5x and N9 (the other devices seem to either not have the data or have missed huge batches)
https://chromeperf.appspot.com/report?sid=ccd86d6fe13388e83443a98cbf2b34b99f7737152e12c68da0b0ee5b1dc988dc&start_rev=385847&end_rev=387894

3) If this benchmark is altered by my change, it is a sign that it heavily relies on malloc() in its fastpath, which in general is a bad thing and worth fixing (I mean the dependency on malloc).
The way to fix it is to have some instrumentation able to tell where malloc are happening, which is what this CL is for.

4) The micro-benchmark itself is pretty noisy, not sure why it triggered an alert on my cl but not on the other spikes (I guess mine happened to be sightlier spikier than the others)?

5) This fixes the Issue 598075 (which is a release blocker), and I have <12h to merge to M51.

So in essence, there is really nothing I can do in my cl to improve this micro benchmark (neither I have a practical way to check all the possible benchmarks before any variation of this). I suppose the point of this microbenchmark is to check for regressions in the cc, not regressions induced by other pieces of the codebase.

Opinions? I'd be honestly tempted to mark this as wontfix or leave it open to figure out, once we have all the tracing pieces, why that microbenchmark relies so much on malloc()-s.

I think the fact that this is a less than 3% regression on a single device (we test 7 devices) and no higher-level metrics like PLT were affected makes this okay to WontFix. vmpstr, Is that okay with you?

sgtm