New issue
Advanced search Search tips

Issue 593872 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

6.2%-6.4% regression in rasterize_and_record_micro.top_25_smooth at 380376:380384

Project Member Reported by chrisphan@chromium.org, Mar 10 2016

Issue description

See the link to graphs below.
 
Project Member

Comment 2 by 42576172...@developer.gserviceaccount.com, Mar 10 2016

Cc: primiano@chromium.org
Owner: primiano@chromium.org

=== 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 : Enable the allocator shim on Linux Desktop / CrOS
Author  : primiano
Commit description:
  
This CL enables the shim introduced in crrev.com/1675143004 by
default, only Linux desktop and CrOS.
This CL is functionally a no-op, in the sense that does not change
the default allocator (which remains tcmalloc on Linux/CrOS) and does
not introduce any new feature (other than the possibility, for future
CLs to install hooks in the allocator path).

See crrev.com/1675143004 for a longer description about the allocator
shim and the design doc bit.ly/allocator-shim.

Note for stability sheriffs
---------------------------
If you see suspicious crashes in tcmalloc (especially in free/tc_free)
there are pretty good chances that something unexpectedly went wrong
here and this CL is the culprit.

Note for perf sheriffs
----------------------
There is a possibility that this CL might cause a regression on the
perf waterfalls (only on Linux/CrOS bots) due to  crbug.com/593344 .
The telemetry tests I tried to run locally were all inconclusive.
Should a regression happen, a temporary workaround is possible as
discussed in https://codereview.chromium.org/1675143004/#msg38.

BUG=550886
TEST=base_unittests (AllocatorShimTest.*,OutOfMemoryDeathTest.*,TCMalloc*)

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

Cr-Commit-Position: refs/heads/master@{#380377}
Commit  : 8712d70e87c419af545e2bad088126d0e19eb384
Date    : Thu Mar 10 09:19:17 2016


===== TESTED REVISIONS =====
Revision                Mean Value  Std. Dev.   Num Values  Good?
chromium@380375         0.237594    0.000376    8           good
chromium@380376         0.237318    0.000645    8           good
chromium@380377         0.2539      0.000201    5           bad
chromium@380378         0.254167    0.000861    5           bad
chromium@380380         0.254767    0.002138    5           bad
chromium@380384         0.253797    0.00068     8           bad

Bisect job ran on: linux_perf_bisect
Bug ID: 593872

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

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bisect/builds/6303
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9018546545859357648


| 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 label Cr-Tests-AutoBisect.  Thank you!
Status: Started (was: Assigned)
Yup I know. See my recent email to perf-sheriffs@ (https://groups.google.com/a/chromium.org/d/msg/perf-sheriffs/iYdqGExyCXY/FBJ9FZkyCwAJ)

-----
TL;DR
If you are not perf-sheriffing you can stop reading here.
crrev.com/1675143004 (#380196) caused a perf regression on linux-release.
crrev.com/1777363002 should fix it

As speculated in the commit message, crrev.com/1675143004 ended up causing a visible across-the board perf regression (example) due to  crbug.com/593344 . 
I was hoping that would have not been visible (the local benchmarks I tried before landing were noisy), but that was not the case.
Not sure if the GASP alert system realized that already, I think it will do that soon. The regression is linux-only.
I have a cure pending review in, that should fix it crrev.com/1777363002.

So, if you are sheriffing and see an across-the-board perf regression on Linux around #380196, (I guess all page cyclers might be affected) that's me and should go away once crrev.com/1777363002 lands.
Apology for the inconvenience, I tried to be as much proactive as possible but... perf benchmarks are hard.
-----

Project Member

Comment 4 by bugdroid1@chromium.org, Mar 11 2016

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

commit d76421171daa1327f8e1c10ee710ebf911a90d2f
Author: primiano <primiano@chromium.org>
Date: Fri Mar 11 11:21:49 2016

Improve performance of the malloc shim layer

This fixes the perf regression introduced by crrev.com/1781573002
As speculated,  crbug.com/593344  (NoBarrier_Load being double-fenced)
causes a visible perf regression, as it causes the addition of two
fences in the malloc fast path.
This CL adds a workaraound that falls-back on a raw volatile ptr read
on Linux+Clang, relying on the fact that on the architectures we care
about a load of an aligned pointer is intrinsically atomic [1,2].

Perf regression:
https://chromeperf.appspot.com/report?sid=1237900c90f9c5e5320a87af8d9e8828fbd1794af07f0a4a335f1fc2a45f120a&start_rev=379480&end_rev=380417

I verified manually that the regression goes away on cc_perftests with
this patch.

[1] Chapter 7 of Part 3A - System Programming Guide
    http://download.intel.com/design/processor/manuals/253668.pdf

[2] A3.5.3 Atomicity in the ARM architecture, ARM Architecture Reference Manual, ARMv7-A and ARMv7-R edition
    http://liris.cnrs.fr/~mmrissa/lib/exe/fetch.php?media=armv7-a-r-manual.pdf

BUG=550886, 593872 
TEST=cc_perftestsfast --gtest_filter=*PrepareTiles*

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

Cr-Commit-Position: refs/heads/master@{#380600}

[modify] https://crrev.com/d76421171daa1327f8e1c10ee710ebf911a90d2f/base/allocator/allocator_shim.cc

Status: Fixed (was: Started)
Looks like all alerts are recovered.
chrisphan@ I'll mark this as fixed.

Sign in to add a comment