tcmalloc: Use C++11 atomics where appropriate.

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

[Nico, 2015-12-25 23:14:49]

Description was changed from

==========
tcmalloc: Use C++11 atomics where appropriate.

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
==========

to

==========
tcmalloc: Use C++11 atomics where appropriate.

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
==========

[Nico, 2015-12-25 23:35:39]

thakis@chromium.org changed reviewers:
+ jfb@chromium.org

[Nico, 2015-12-25 23:35:40]

wdyt?

[JF, 2015-12-26 00:27:24]

Looks good overall, though the comment confuses me a bit.

https://codereview.chromium.org/1549913002/diff/140001/third_party/tcmalloc/c...
File third_party/tcmalloc/chromium/src/base/atomicops_internals_portable.h
(right):

https://codereview.chromium.org/1549913002/diff/140001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/base/atomicops_internals_portable.h:39:
#define BASE_HAS_ATOMIC64 1  // Use only in tests and base/atomic*
I'm not sure I understand the comment: isn't this force-defining the 64-bit
versions of atomic functions for all users?

[Nico, 2015-12-26 04:13:03]

https://codereview.chromium.org/1549913002/diff/140001/third_party/tcmalloc/c...
File third_party/tcmalloc/chromium/src/base/atomicops_internals_portable.h
(right):

https://codereview.chromium.org/1549913002/diff/140001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/base/atomicops_internals_portable.h:39:
#define BASE_HAS_ATOMIC64 1  // Use only in tests and base/atomic*
On 2015/12/26 00:27:24, JF wrote:
> I'm not sure I understand the comment: isn't this force-defining the 64-bit
> versions of atomic functions for all users?

It's like this in the x86 version this patch deleted, and it's used in
atomicops.h. I understood the comment to mean that you shouldn't check for this
define except in comments and (tcmalloc's) base/.

[JF, 2015-12-26 15:56:15]

https://codereview.chromium.org/1549913002/diff/140001/third_party/tcmalloc/c...
File third_party/tcmalloc/chromium/src/base/atomicops_internals_portable.h
(right):

https://codereview.chromium.org/1549913002/diff/140001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/base/atomicops_internals_portable.h:39:
#define BASE_HAS_ATOMIC64 1  // Use only in tests and base/atomic*
On 2015/12/26 04:13:03, Nico wrote:
> On 2015/12/26 00:27:24, JF wrote:
> > I'm not sure I understand the comment: isn't this force-defining the 64-bit
> > versions of atomic functions for all users?
> 
> It's like this in the x86 version this patch deleted, and it's used in
> atomicops.h. I understood the comment to mean that you shouldn't check for
this
> define except in comments and (tcmalloc's) base/.

Is it required though? If so, could we only support 64-bit atomics on x86-64?

I'd wary of supporting 64-bit atomics on non-64-bit platforms: older MIPS don't
have lock-free 64-bit atomics, and ARM has fairly expensive ones (they're
useful, but perf really needs to be measured when used).

[Nico, 2015-12-26 16:35:05]

https://codereview.chromium.org/1549913002/diff/140001/third_party/tcmalloc/c...
File third_party/tcmalloc/chromium/src/base/atomicops_internals_portable.h
(right):

https://codereview.chromium.org/1549913002/diff/140001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/base/atomicops_internals_portable.h:39:
#define BASE_HAS_ATOMIC64 1  // Use only in tests and base/atomic*
On 2015/12/26 15:56:15, JF wrote:
> On 2015/12/26 04:13:03, Nico wrote:
> > On 2015/12/26 00:27:24, JF wrote:
> > > I'm not sure I understand the comment: isn't this force-defining the
64-bit
> > > versions of atomic functions for all users?
> > 
> > It's like this in the x86 version this patch deleted, and it's used in
> > atomicops.h. I understood the comment to mean that you shouldn't check for
> this
> > define except in comments and (tcmalloc's) base/.
> 
> Is it required though? If so, could we only support 64-bit atomics on x86-64?
> 
> I'd wary of supporting 64-bit atomics on non-64-bit platforms: older MIPS
don't
> have lock-free 64-bit atomics, and ARM has fairly expensive ones (they're
> useful, but perf really needs to be measured when used).

In practice, there are three non-test uses of atomicops in tcmalloc
(https://code.google.com/p/chromium/codesearch#search/&q=atomicops.h%5C%22%20f...):

1. spinlock_internal uses an atomic32:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/tcmall...

2. vdso_support (only used on 32-bit linux and can probably be removed there
too) uses a call to MemoryBarrier:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/tcmall...

3. AtomicPtr uses an AtomicWord:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/tcmall...

So 64-bit atomics are only used on 64-bit platforms, via the third use (if that
code is life at all). tcmalloc doesn't change much, so I don't think this list
of uses will fluctuate a lot.

The old code used to define BASE_HAS_ATOMIC64 for 32-bit x86 too, so I figured
I'd do this too, and if this is defined then atomicops defines functions that
forward from the global namespace to base::subtle (with a TODO to remove these
forwarders). I could delete those, but I'd prefer if we didn't diverge more from
upstream tcmalloc than necessary.

[JF, 2015-12-26 16:58:37]

lgtm

https://codereview.chromium.org/1549913002/diff/140001/third_party/tcmalloc/c...
File third_party/tcmalloc/chromium/src/base/atomicops_internals_portable.h
(right):

https://codereview.chromium.org/1549913002/diff/140001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/base/atomicops_internals_portable.h:39:
#define BASE_HAS_ATOMIC64 1  // Use only in tests and base/atomic*
On 2015/12/26 16:35:05, Nico wrote:
> On 2015/12/26 15:56:15, JF wrote:
> > On 2015/12/26 04:13:03, Nico wrote:
> > > On 2015/12/26 00:27:24, JF wrote:
> > > > I'm not sure I understand the comment: isn't this force-defining the
> 64-bit
> > > > versions of atomic functions for all users?
> > > 
> > > It's like this in the x86 version this patch deleted, and it's used in
> > > atomicops.h. I understood the comment to mean that you shouldn't check for
> > this
> > > define except in comments and (tcmalloc's) base/.
> > 
> > Is it required though? If so, could we only support 64-bit atomics on
x86-64?
> > 
> > I'd wary of supporting 64-bit atomics on non-64-bit platforms: older MIPS
> don't
> > have lock-free 64-bit atomics, and ARM has fairly expensive ones (they're
> > useful, but perf really needs to be measured when used).
> 
> In practice, there are three non-test uses of atomicops in tcmalloc
>
(https://code.google.com/p/chromium/codesearch#search/&q=atomicops.h%5C%22%20f...):
> 
> 1. spinlock_internal uses an atomic32:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/tcmall...
> 
> 2. vdso_support (only used on 32-bit linux and can probably be removed there
> too) uses a call to MemoryBarrier:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/tcmall...
> 
> 3. AtomicPtr uses an AtomicWord:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/tcmall...
> 
> So 64-bit atomics are only used on 64-bit platforms, via the third use (if
that
> code is life at all). tcmalloc doesn't change much, so I don't think this list
> of uses will fluctuate a lot.
> 
> The old code used to define BASE_HAS_ATOMIC64 for 32-bit x86 too, so I figured
> I'd do this too, and if this is defined then atomicops defines functions that
> forward from the global namespace to base::subtle (with a TODO to remove these
> forwarders). I could delete those, but I'd prefer if we didn't diverge more
from
> upstream tcmalloc than necessary.

OK thanks for looking into it!

Yes, I think 2 can also be removed, but agree with you that less divergence is
good.

[Nico, 2015-12-26 17:13:44]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2015-12-26 17:13:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1549913002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1549913002/140001

[commit-bot: I haz the power, 2015-12-26 18:19:19]

Description was changed from

==========
tcmalloc: Use C++11 atomics where appropriate.

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
==========

to

==========
tcmalloc: Use C++11 atomics where appropriate.

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
==========

[commit-bot: I haz the power, 2015-12-26 18:19:20]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-12-26 18:20:10]

Description was changed from

==========
tcmalloc: Use C++11 atomics where appropriate.

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
==========

to

==========
tcmalloc: Use C++11 atomics where appropriate.

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

Committed: https://crrev.com/e13537fe418eff11d3cab9077f6a647d7c74f103
Cr-Commit-Position: refs/heads/master@{#366904}
==========

[commit-bot: I haz the power, 2015-12-26 18:20:11]

Patchset 8 (id:??) landed as
https://crrev.com/e13537fe418eff11d3cab9077f6a647d7c74f103
Cr-Commit-Position: refs/heads/master@{#366904}

[Nico, 2015-12-29 14:26:40]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1557433002/ by thakis@chromium.org.

The reason for reverting is: Caused performance problems (see
http://crbug.com/572525 and dupes).

[Nico, 2017-04-13 15:33:17]

Description was changed from

==========
tcmalloc: Use C++11 atomics where appropriate.

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

Committed: https://crrev.com/e13537fe418eff11d3cab9077f6a647d7c74f103
Cr-Commit-Position: refs/heads/master@{#366904}
==========

to

==========
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

Committed: https://crrev.com/e13537fe418eff11d3cab9077f6a647d7c74f103
Cr-Commit-Position: refs/heads/master@{#366904}
==========

[Nico, 2017-04-13 15:33:27]

The CQ bit was checked by thakis@chromium.org

[Nico, 2017-04-13 15:33:28]

The patchset sent to the CQ was uploaded after l-g-t-m from jfb@chromium.org
Link to the patchset: https://codereview.chromium.org/1549913002/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-13 15:34:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2017-04-13 15:35:13]

Description was changed from

==========
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

Committed: https://crrev.com/e13537fe418eff11d3cab9077f6a647d7c74f103
Cr-Commit-Position: refs/heads/master@{#366904}
==========

to

==========
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-Commit-Position: refs/heads/master@{#366904}
==========

[commit-bot: I haz the power, 2017-04-13 16:44:22]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1492097607883640,
"parent_rev": "c7578730e5e3bab215c3997c4127ea42d620952a", "commit_rev":
"2f6d8f01d9087e8bebab5b7d2d25b28d657dbbb7"}

[commit-bot: I haz the power, 2017-04-13 16:45:24]

Description was changed from

==========
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-Commit-Position: refs/heads/master@{#366904}
==========

to

==========
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/+/2f6d8f01d9087e8bebab5b7d2d25...
==========

[commit-bot: I haz the power, 2017-04-13 16:45:25]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/2f6d8f01d9087e8bebab5b7d2d25...

[Nico, 2017-04-13 16:59:36]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/2818713003/ by thakis@chromium.org.

The reason for reverting is: Doesn't build on 32-bit:
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.linux%2FLinux_B...

../../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'
.

[findit-for-me, 2017-04-13 17:02:44]

Findit(https://goo.gl/kROfz5) confirmed this CL at revision 464440 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...