Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user
Status: Fixed
Owner:
Closed: Jan 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression

Blocking:
issue 559247



Sign in to add a comment
3.7%-35.6% regression in rasterize_and_record_micro.top_25_smooth at 366902:366904
Reported by rschoen@google.com, Dec 27 2015 Back to list
See the link to graphs below.
 
Owner: tha...@chromium.org
thakis, pretty sure this is e13537fe418eff11d3cab9077f6a647d7c74f103. Was this expected?
Project Member Comment 3 by 42576172...@developer.gserviceaccount.com, Dec 27 2015
Cc: tha...@chromium.org

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

Hi thakis@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 status: Completed
Bisect job ran on: linux_perf_bisect



===== BISECT JOB RESULTS =====
Status: Positive: A suspected commit was found.

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --also-run-disabled-tests rasterize_and_record_micro.key_silk_cases
Test Metric: record_time/record_time
Relative Change: 37.31%
Score: 99.9
Retested CL with revert: Not Implemented.


===== SUSPECTED CL(s) =====
Subject : tcmalloc: Use C++11 atomics where appropriate.
Author  : thakis, thakis@chromium.org
Commit description:

Ports these CLs to tcmalloc:
https://codereview.chromium.org/636783002/
https://codereview.chromium.org/1466833002/ (except mac)

No intended behavior change, but it should remove
the static initializer in atomicops_internals_x86_gcc.h
on Linux.  It's also less code.

BUG=94925,559247

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

Cr-Commit-Position: refs/heads/master@{#366904}
Commit  : e13537fe418eff11d3cab9077f6a647d7c74f103
Date    : Sat Dec 26 18:19:59 2015


===== TESTED REVISIONS =====
Depot    Revision Mean Value Std. Dev. Num Values Good?
chromium r366903  0.049198   0.000834  4          good

chromium r366904  0.067552   0.000193  4          bad   <-




| 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

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

Comment 4 by tha...@chromium.org, Dec 27 2015
Cc: jfb@chromium.org
No. Will revert Tuesday. We should probably not ist the _portable atomics until this is understood and fixed then.
Project Member Comment 5 by 42576172...@developer.gserviceaccount.com, Dec 28 2015
 Issue 572712  has been merged into this issue.
Project Member Comment 6 by 42576172...@developer.gserviceaccount.com, Dec 29 2015
 Issue 572789  has been merged into this issue.
Comment 7 by tha...@chromium.org, Dec 29 2015
Blocking: chromium:559247
Status: Fixed
The recovery after that revert is visible in the graphs: https://chromeperf.appspot.com/group_report?bug_id=572525

Thanks thakis@ :-)
I wonder if it was caused by issue 592903 (additional mfences as was discovered in  issue 593344 ). If so, then this issue only affects Linux?
That's the working theory (and all the graphs above are on linux), but we never investigated in depth.
Does this mean https://codereview.chromium.org/1549913002 is completely dead in the water, or will try again at some point?
I think we can try again now that we have the new sysroot. I think the old libstdc++ had an issue causing this. Wasn't to give it a try?
https://codereview.chromium.org/1549913002 is your CL. You should take the credit. :)
Project Member Comment 15 by bugdroid1@chromium.org, Apr 13
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2f6d8f01d9087e8bebab5b7d2d25b28d657dbbb7

commit 2f6d8f01d9087e8bebab5b7d2d25b28d657dbbb7
Author: thakis <thakis@chromium.org>
Date: Thu Apr 13 16:44:17 2017

tcmalloc: Use C++11 atomics where appropriate.

Reland now that we have a newer sysroot.

Ports these CLs to tcmalloc:
https://codereview.chromium.org/636783002/
https://codereview.chromium.org/1466833002/ (except mac)

No intended behavior change, but it should remove
the static initializer in atomicops_internals_x86_gcc.h
on Linux.  It's also less code.

BUG=94925,559247, 572525 

Committed: https://crrev.com/e13537fe418eff11d3cab9077f6a647d7c74f103
Cr-Original-Commit-Position: refs/heads/master@{#366904}
Review-Url: https://codereview.chromium.org/1549913002
Cr-Commit-Position: refs/heads/master@{#464440}

[modify] https://crrev.com/2f6d8f01d9087e8bebab5b7d2d25b28d657dbbb7/base/allocator/BUILD.gn
[modify] https://crrev.com/2f6d8f01d9087e8bebab5b7d2d25b28d657dbbb7/third_party/tcmalloc/README.chromium
[delete] https://crrev.com/c7578730e5e3bab215c3997c4127ea42d620952a/third_party/tcmalloc/chromium/src/base/atomicops-internals-arm-generic.h
[delete] https://crrev.com/c7578730e5e3bab215c3997c4127ea42d620952a/third_party/tcmalloc/chromium/src/base/atomicops-internals-arm-v6plus.h
[delete] https://crrev.com/c7578730e5e3bab215c3997c4127ea42d620952a/third_party/tcmalloc/chromium/src/base/atomicops-internals-linuxppc.h
[delete] https://crrev.com/c7578730e5e3bab215c3997c4127ea42d620952a/third_party/tcmalloc/chromium/src/base/atomicops-internals-x86.cc
[delete] https://crrev.com/c7578730e5e3bab215c3997c4127ea42d620952a/third_party/tcmalloc/chromium/src/base/atomicops-internals-x86.h
[modify] https://crrev.com/2f6d8f01d9087e8bebab5b7d2d25b28d657dbbb7/third_party/tcmalloc/chromium/src/base/atomicops.h
[add] https://crrev.com/2f6d8f01d9087e8bebab5b7d2d25b28d657dbbb7/third_party/tcmalloc/chromium/src/base/atomicops_internals_portable.h

Project Member Comment 16 by bugdroid1@chromium.org, Apr 13
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b20672f0a5080fe4318e049bd408db1a01105904

commit b20672f0a5080fe4318e049bd408db1a01105904
Author: thakis <thakis@chromium.org>
Date: Thu Apr 13 17:01:07 2017

Revert of tcmalloc: Use C++11 atomics where appropriate. (patchset #9 id:160001 of https://codereview.chromium.org/1549913002/ )

Reason for revert:
Doesn't build on 32-bit: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.linux%2FLinux_Builder__dbg__32_%2F66018%2F%2B%2Frecipes%2Fsteps%2Fcompile%2F0%2Fstdout

../../build/linux/debian_jessie_i386-sysroot/usr/lib/gcc/i586-linux-gnu/4.8/../../../../include/c++/4.8/bits/atomic_base.h:506: error: undefined reference to '__atomic_load_8'
../../build/linux/debian_jessie_i386-sysroot/usr/lib/gcc/i586-linux-gnu/4.8/../../../../include/c++/4.8/bits/atomic_base.h:486: error: undefined reference to '__atomic_store_8'

Original issue's description:
> tcmalloc: Use C++11 atomics where appropriate.
>
> Reland now that we have a newer sysroot.
>
> Ports these CLs to tcmalloc:
> https://codereview.chromium.org/636783002/
> https://codereview.chromium.org/1466833002/ (except mac)
>
> No intended behavior change, but it should remove
> the static initializer in atomicops_internals_x86_gcc.h
> on Linux.  It's also less code.
>
> BUG=94925,559247, 572525 
>
> Committed: https://crrev.com/e13537fe418eff11d3cab9077f6a647d7c74f103
> Cr-Original-Commit-Position: refs/heads/master@{#366904}
> Review-Url: https://codereview.chromium.org/1549913002
> Cr-Commit-Position: refs/heads/master@{#464440}
> Committed: https://chromium.googlesource.com/chromium/src/+/2f6d8f01d9087e8bebab5b7d2d25b28d657dbbb7

TBR=jfb@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=94925,559247, 572525 

Review-Url: https://codereview.chromium.org/2818713003
Cr-Commit-Position: refs/heads/master@{#464445}

[modify] https://crrev.com/b20672f0a5080fe4318e049bd408db1a01105904/base/allocator/BUILD.gn
[modify] https://crrev.com/b20672f0a5080fe4318e049bd408db1a01105904/third_party/tcmalloc/README.chromium
[add] https://crrev.com/b20672f0a5080fe4318e049bd408db1a01105904/third_party/tcmalloc/chromium/src/base/atomicops-internals-arm-generic.h
[add] https://crrev.com/b20672f0a5080fe4318e049bd408db1a01105904/third_party/tcmalloc/chromium/src/base/atomicops-internals-arm-v6plus.h
[add] https://crrev.com/b20672f0a5080fe4318e049bd408db1a01105904/third_party/tcmalloc/chromium/src/base/atomicops-internals-linuxppc.h
[add] https://crrev.com/b20672f0a5080fe4318e049bd408db1a01105904/third_party/tcmalloc/chromium/src/base/atomicops-internals-x86.cc
[add] https://crrev.com/b20672f0a5080fe4318e049bd408db1a01105904/third_party/tcmalloc/chromium/src/base/atomicops-internals-x86.h
[modify] https://crrev.com/b20672f0a5080fe4318e049bd408db1a01105904/third_party/tcmalloc/chromium/src/base/atomicops.h
[delete] https://crrev.com/c08796e46810403645b6cf2cc4f8890c78a361de/third_party/tcmalloc/chromium/src/base/atomicops_internals_portable.h

Sign in to add a comment