tcmalloc: Use futex syscall in SpinLockDelay() for ARM.

tcmalloc: Use futex syscall in SpinLockDelay() for ARM.

SpinLockDelay() was avoiding using futex() when compiled for ARM.
The alternative implementation was sleeping for 2 ms. This caused
performance issues in Telemetry's SvgCubics benchmark. The score for
this benchmark more than doubles when this patch is applied (scores were
measured on veyron_jerry, veyron_minnie, daisy). Other benchmarks
(blink_perf.bindings/post-message, blink_perf.canvas/draw-static-webgl-
to-hw-accelerated-canvas-2d) benefit from this change to a lesser
extent. This patch also removes a reference to futex1 in a comment in
linux-syscall_support.h, as this is not a valid syscall name.

Note that a similar fix was pushed in the upstream version of tcmalloc
(issue-693, https://github.com/gperftools/gperftools/commit/7df7f14).

BUG=

Change-Id: I390ac51ed5e1b0ad021ac63eaf3bce81cdca8599
Committed: https://crrev.com/7f1336053e39737671a52ca7a265007e9024ee8a
Committed: https://crrev.com/e787102a66a74471263456e204209ea6b29e2d53
Cr-Original-Commit-Position: refs/heads/master@{#432628}
Cr-Commit-Position: refs/heads/master@{#437099}

[fnch, 2016-10-27 12:28:28]

Description was changed from

==========
tcmalloc: Use futex syscall in SpinLockDelay() for ARM.

SpinLockDelay() was avoiding using futex() when compiled for ARM.
The alternative implementation was sleeping for 2 ms. This caused
performance issues in Telemetry's SvgCubics benchmark. The score for
this benchmark more than doubles when this patch is applied (scores were
measured on veyron_jerry, veyron_minnie, daisy). Other benchmarks
(blink_perf.bindings/post-message, blink_perf.canvas/draw-static-webgl-
to-hw-accelerated-canvas-2d) benefit from this change to a lesser
extent. This patch also removes a reference to futex1 in a comment in
linux-syscall_support.h, as this is not a valid syscall name.

Note that a similar fix was pushed in the upstream version of tcmalloc
(issue-693, https://github.com/gperftools/gperftools/commit/7df7f14).

BUG=

Change-Id: I390ac51ed5e1b0ad021ac63eaf3bce81cdca8599
==========

to

==========
tcmalloc: Use futex syscall in SpinLockDelay() for ARM.

SpinLockDelay() was avoiding using futex() when compiled for ARM.
The alternative implementation was sleeping for 2 ms. This caused
performance issues in Telemetry's SvgCubics benchmark. The score for
this benchmark more than doubles when this patch is applied (scores were
measured on veyron_jerry, veyron_minnie, daisy). Other benchmarks
(blink_perf.bindings/post-message, blink_perf.canvas/draw-static-webgl-
to-hw-accelerated-canvas-2d) benefit from this change to a lesser
extent. This patch also removes a reference to futex1 in a comment in
linux-syscall_support.h, as this is not a valid syscall name.

Note that a similar fix was pushed in the upstream version of tcmalloc
(issue-693, https://github.com/gperftools/gperftools/commit/7df7f14).

BUG=

Change-Id: I390ac51ed5e1b0ad021ac63eaf3bce81cdca8599
==========

[fnch, 2016-10-27 12:28:28]

matteo.franchin@arm.com changed reviewers:
+ wfh@chromium.org

[adenilson.cavalcanti, 2016-10-27 16:03:09]

Kicked the bots, let's see how it goes.

[fnch, 2016-10-28 13:01:49]

The CQ bit was checked by matteo.franchin@arm.com

[fnch, 2016-10-28 13:01:53]

The CQ bit was unchecked by matteo.franchin@arm.com

[fnch, 2016-10-28 13:12:20]

Rebased with master.

Many of the bots failed to apply the patch with the error
"[...]spinlock_linux-inl.h: does not exist in index". This is puzzling as some
other bots succeeded and the patch rebases cleanly on top of master even today.
This seems to imply that the bots were not testing the same code base. Maybe
this was a problem with the infrastructure.
Cheers,
Matteo

[fnch, 2016-10-28 18:23:12]

The CQ bit was checked by matteo.franchin@arm.com

[fnch, 2016-10-28 18:23:20]

The CQ bit was unchecked by matteo.franchin@arm.com

[cavalcantii1, 2016-10-31 17:43:19]

The CQ bit was checked by cavalcantii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 17:43:43]

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

[commit-bot: I haz the power, 2016-10-31 17:55:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-31 17:55:43]

Dry run: This issue passed the CQ dry run.

[fnch, 2016-10-31 18:40:15]

matteo.franchin@arm.com changed reviewers:
+ thakis@chromium.org

[fnch, 2016-10-31 18:45:35]

Please, have a look at this patch and let me know what you think. I'll try to
address all comments/requests as soon as possible (from the UK time zone).
Many thanks,
Matteo

[fnch, 2016-11-15 18:49:11]

ping

[cavalcantii1, 2016-11-15 18:52:44]

The CQ bit was checked by cavalcantii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 18:53:14]

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

[commit-bot: I haz the power, 2016-11-15 19:03:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 19:03:43]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2016-11-16 13:35:27]

primiano@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-11-16 13:35:27]

Can you update also third_party/tcmalloc/README.chromium to mention the fact
that this is bakporting 7df7f14 from upstream?

Looks like there are some interesting performance wins with this.

Thanks

[fnch, 2016-11-16 13:38:23]

Hi Primiano,

On 16/11/16 13:35, primiano@chromium.org wrote:
> Can you update also third_party/tcmalloc/README.chromium to mention the fact
> that this is bakporting 7df7f14 from upstream?

Sure, I'll do it soon. Thanks for looking into it :-)
Cheers,
Matteo

>
> Looks like there are some interesting performance wins with this.
>
> Thanks
>
> https://codereview.chromium.org/2457473003/
>
>
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[fnch, 2016-11-16 14:51:18]

Added line to README.chromium as suggested by Primiano.

[Primiano Tucci (use gerrit), 2016-11-16 16:07:09]

On 2016/11/16 14:51:18, fnch wrote:
> Added line to README.chromium as suggested by Primiano.

non-owner LGTM.
Will any concerns on this? Seems upstreamed and a good perf improvement.

[Will Harris, 2016-11-16 18:18:36]

I'm not familiar with this code too much - so maybe a dumb question but do all
arm processors we need to support, support this?

[cavalcantii1, 2016-11-16 18:56:11]

The CQ bit was checked by cavalcantii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 18:57:19]

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

[fnch, 2016-11-16 19:00:50]

The futex syscall is part of the functionality provided by the Linux kernel. It
is reasonable to expect that it will be available on ARM platforms where Linux
is the underlying kernel. Chatting with some of our kernel guys, I learnt that
it is theoretically possible to compile the Linux kernel without futex support.
But this is not ARM specific, requires enabling expert mode, and such kernels
"may not run glibc-based applications correctly" (I am here citing the Linux
kernel doc). I can also add that mutexes are implemented in bionic (Android's
libc) using futexes.

Futexes were introduced in the kernel starting from Linux 2.6.x in 2003. I would
say that, unless you have to support extremely old Linux kernels, it should be
safe to use them :-)

[commit-bot: I haz the power, 2016-11-16 19:33:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-16 19:33:55]

Dry run: This issue passed the CQ dry run.

[Will Harris, 2016-11-16 20:29:54]

lgtm then

[cavalcantii1, 2016-11-16 20:58:15]

As the patch was lgtm-ed and all bots are green, I'm adding it to the CQ.

Good job, Matteo.

[cavalcantii1, 2016-11-16 20:58:19]

The CQ bit was checked by cavalcantii@chromium.org

[commit-bot: I haz the power, 2016-11-16 20:59:33]

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

[commit-bot: I haz the power, 2016-11-16 21:11:36]

Description was changed from

==========
tcmalloc: Use futex syscall in SpinLockDelay() for ARM.

SpinLockDelay() was avoiding using futex() when compiled for ARM.
The alternative implementation was sleeping for 2 ms. This caused
performance issues in Telemetry's SvgCubics benchmark. The score for
this benchmark more than doubles when this patch is applied (scores were
measured on veyron_jerry, veyron_minnie, daisy). Other benchmarks
(blink_perf.bindings/post-message, blink_perf.canvas/draw-static-webgl-
to-hw-accelerated-canvas-2d) benefit from this change to a lesser
extent. This patch also removes a reference to futex1 in a comment in
linux-syscall_support.h, as this is not a valid syscall name.

Note that a similar fix was pushed in the upstream version of tcmalloc
(issue-693, https://github.com/gperftools/gperftools/commit/7df7f14).

BUG=

Change-Id: I390ac51ed5e1b0ad021ac63eaf3bce81cdca8599
==========

to

==========
tcmalloc: Use futex syscall in SpinLockDelay() for ARM.

SpinLockDelay() was avoiding using futex() when compiled for ARM.
The alternative implementation was sleeping for 2 ms. This caused
performance issues in Telemetry's SvgCubics benchmark. The score for
this benchmark more than doubles when this patch is applied (scores were
measured on veyron_jerry, veyron_minnie, daisy). Other benchmarks
(blink_perf.bindings/post-message, blink_perf.canvas/draw-static-webgl-
to-hw-accelerated-canvas-2d) benefit from this change to a lesser
extent. This patch also removes a reference to futex1 in a comment in
linux-syscall_support.h, as this is not a valid syscall name.

Note that a similar fix was pushed in the upstream version of tcmalloc
(issue-693, https://github.com/gperftools/gperftools/commit/7df7f14).

BUG=

Change-Id: I390ac51ed5e1b0ad021ac63eaf3bce81cdca8599
==========

[commit-bot: I haz the power, 2016-11-16 21:11:45]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-16 21:17:14]

Description was changed from

==========
tcmalloc: Use futex syscall in SpinLockDelay() for ARM.

SpinLockDelay() was avoiding using futex() when compiled for ARM.
The alternative implementation was sleeping for 2 ms. This caused
performance issues in Telemetry's SvgCubics benchmark. The score for
this benchmark more than doubles when this patch is applied (scores were
measured on veyron_jerry, veyron_minnie, daisy). Other benchmarks
(blink_perf.bindings/post-message, blink_perf.canvas/draw-static-webgl-
to-hw-accelerated-canvas-2d) benefit from this change to a lesser
extent. This patch also removes a reference to futex1 in a comment in
linux-syscall_support.h, as this is not a valid syscall name.

Note that a similar fix was pushed in the upstream version of tcmalloc
(issue-693, https://github.com/gperftools/gperftools/commit/7df7f14).

BUG=

Change-Id: I390ac51ed5e1b0ad021ac63eaf3bce81cdca8599
==========

to

==========
tcmalloc: Use futex syscall in SpinLockDelay() for ARM.

SpinLockDelay() was avoiding using futex() when compiled for ARM.
The alternative implementation was sleeping for 2 ms. This caused
performance issues in Telemetry's SvgCubics benchmark. The score for
this benchmark more than doubles when this patch is applied (scores were
measured on veyron_jerry, veyron_minnie, daisy). Other benchmarks
(blink_perf.bindings/post-message, blink_perf.canvas/draw-static-webgl-
to-hw-accelerated-canvas-2d) benefit from this change to a lesser
extent. This patch also removes a reference to futex1 in a comment in
linux-syscall_support.h, as this is not a valid syscall name.

Note that a similar fix was pushed in the upstream version of tcmalloc
(issue-693, https://github.com/gperftools/gperftools/commit/7df7f14).

BUG=

Change-Id: I390ac51ed5e1b0ad021ac63eaf3bce81cdca8599
Committed: https://crrev.com/7f1336053e39737671a52ca7a265007e9024ee8a
Cr-Commit-Position: refs/heads/master@{#432628}
==========

[commit-bot: I haz the power, 2016-11-16 21:17:15]

Patchset 5 (id:??) landed as
https://crrev.com/7f1336053e39737671a52ca7a265007e9024ee8a
Cr-Commit-Position: refs/heads/master@{#432628}

[llozano, 2016-11-18 18:59:33]

On 2016/11/16 14:51:18, fnch wrote:
> Added line to README.chromium as suggested by Primiano.

[llozano, 2016-11-18 19:01:51]

On 2016/11/16 21:17:15, commit-bot: I haz the power wrote:
> Patchset 5 (id:??) landed as
> https://crrev.com/7f1336053e39737671a52ca7a265007e9024ee8a
> Cr-Commit-Position: refs/heads/master@{#43262I

I believe this breaks ChromeOS clang build.

see https://bugs.chromium.org/p/chromium/issues/detail?id=666839

I cannot assign to matteo or fnch.

[cavalcantii1, 2016-11-18 20:16:46]

Concerning assigning bugs, I will request edit-bug access for Matteo.

Next: on the regression, I posted a few comments in the aforementioned bug.

[cavalcantii1, 2016-11-18 20:59:48]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2514973002/ by cavalcantii@chromium.org.

The reason for reverting is: It seems that a ChromeOS build with clang has
failed, details on: https://bugs.chromium.org/p/chromium/issues/detail?id=666839

We will revert the patch while the issue is being investigated..

[fnch, 2016-11-21 15:26:42]

Here is an explanation of the issue which caused this CL to be reverted.

The issue occurs when compiling Chrome for ChromiumOS on Linux with clang (this
is not the default, but can be enabled by setting the environment variable
USE=clang).

The issue is caused by a bug in clang integrated assembler, which hit Chromium's
tcmalloc some more times in the past (BUGS 564059 and 564910) and was reported
here https://llvm.org/bugs/show_bug.cgi?id=31058. The issue does not affect gcc
builds.

This Clang bug causes failure to compile Chromium for ChromiumOS on ARM with
-O0, even when my patch is not applied. When compiling with -O2 (the default),
however, the situation is different. The static variable have_futex in
spinlock_linux-inl.h, which before my patch was guaranteed to be false, is now
initialised to true. As a consequence, Clang stops eliminating code that relies
on have_futex being true. In particular, Clang ends up compiling the function
base::internal::SuggestedDelayNS() which uses base::subtle::NoBarrier_Load() and
base::subtle::NoBarrier_Store(). These two functions are defined in
atomicops-internals-arm-v6plus.h and are implemented using inline assembly
containing ldrexd and strexd. These are the instructions that clang is
erroneously complaining about.

[Will Harris, 2016-11-21 17:32:46]

On 2016/11/21 15:26:42, fnch wrote:
> Here is an explanation of the issue which caused this CL to be reverted.
> 
> The issue occurs when compiling Chrome for ChromiumOS on Linux with clang
(this
> is not the default, but can be enabled by setting the environment variable
> USE=clang).
> 
> The issue is caused by a bug in clang integrated assembler, which hit
Chromium's
> tcmalloc some more times in the past (BUGS 564059 and 564910) and was reported
> here https://llvm.org/bugs/show_bug.cgi?id=31058. The issue does not affect
gcc
> builds.
> 
> This Clang bug causes failure to compile Chromium for ChromiumOS on ARM with
> -O0, even when my patch is not applied. When compiling with -O2 (the default),
> however, the situation is different. The static variable have_futex in
> spinlock_linux-inl.h, which before my patch was guaranteed to be false, is now
> initialised to true. As a consequence, Clang stops eliminating code that
relies
> on have_futex being true. In particular, Clang ends up compiling the function
> base::internal::SuggestedDelayNS() which uses base::subtle::NoBarrier_Load()
and
> base::subtle::NoBarrier_Store(). These two functions are defined in
> atomicops-internals-arm-v6plus.h and are implemented using inline assembly
> containing ldrexd and strexd. These are the instructions that clang is
> erroneously complaining about.

What are the next steps here to try and re-land this? Does the llvm bug need
some attention? Unfortunately I can't visit llvm.org/bugs since the SSL cert is
invalid :(

[Primiano Tucci (use gerrit), 2016-11-21 19:22:07]

>
> What are the next steps here to try and re-land this? Does the llvm bug need
> some attention?

As I suggested in crbug.com/666839 this should be an upstream bug on
https://github.com/gperftools/gperftools/issues .
It might be just a matter of a missing ".arm" directive in the assembly.

> Unfortunately I can't visit llvm.org/bugs since the SSL cert is invalid :(

It is *just* still using SHA1. There is a "load page (unsafe)" link in the
interstitial no?
Anyways, I wayback-ed it for you
https://web.archive.org/web/20161121191813/https://llvm.org/bugs/show_bug.cgi...

[fnch, 2016-11-21 19:22:39]

Hi Will,
I am considering possible options. One would be to change the compilation flags
and use -no-integrated-as to disable clang integrated assembler (as suggested by
mcgrathr in https://bugs.chromium.org/p/chromium/issues/detail?id=564059). This
seems to work. The option scares me a bit, though. Changing the compilation
flags is a global change that may hit us back from everywhere in the code base.

Another option is replacing the code in SuggestedDelayNS() to use std::atomic. I
would be prudent and use an #ifdef to do this only for ARM, when compiled via
clang on Linux. Unfortunately, <atomic> seems to be banned from the Chromium
codebase: https://chromium-cpp.appspot.com/, I suspect I should rather use
src/base/atomicops.h (I mean the one outside the tcmalloc tree). If I understand
correctly this one uses <atomic> only when it is a good idea to do so (i.e. on
compilers != MSVC). I suspect something like that was already in jfb plans (see
https://bugs.chromium.org/p/chromium/issues/detail?id=564910, where he says he
intends to delete atomicops.h).

It is also worth noticing that the CL at
https://codereview.chromium.org/2515503002 should have fixed the bug. The last
experiments I did revealed that ChromiumOS has some compiler wrappers that
intercept any attempt to call the compilers and hiddenly insert and modify the
compiler flags. It seems this is what is preventing this CL from fixing
compilation on ChromiumOS, as it should. More details at
https://bugs.chromium.org/p/chromium/issues/detail?id=666839.

[llozano, 2016-11-22 00:39:05]

On 2016/11/21 19:22:39, fnch wrote:
> Hi Will,
> I am considering possible options. One would be to change the compilation
flags
> and use -no-integrated-as to disable clang integrated assembler (as suggested
by
> mcgrathr in https://bugs.chromium.org/p/chromium/issues/detail?id=564059).
This
> seems to work. The option scares me a bit, though. Changing the compilation
> flags is a global change that may hit us back from everywhere in the code
base.
> 
> Another option is replacing the code in SuggestedDelayNS() to use std::atomic.
I
> would be prudent and use an #ifdef to do this only for ARM, when compiled via
> clang on Linux. Unfortunately, <atomic> seems to be banned from the Chromium
> codebase: https://chromium-cpp.appspot.com/, I suspect I should rather use
> src/base/atomicops.h (I mean the one outside the tcmalloc tree). If I
understand
> correctly this one uses <atomic> only when it is a good idea to do so (i.e. on
> compilers != MSVC). I suspect something like that was already in jfb plans
(see
> https://bugs.chromium.org/p/chromium/issues/detail?id=564910, where he says he
> intends to delete atomicops.h).
> 
> It is also worth noticing that the CL at
> https://codereview.chromium.org/2515503002 should have fixed the bug. The last
> experiments I did revealed that ChromiumOS has some compiler wrappers that
> intercept any attempt to call the compilers and hiddenly insert and modify the
> compiler flags. It seems this is what is preventing this CL from fixing
> compilation on ChromiumOS, as it should. More details at
> https://bugs.chromium.org/p/chromium/issues/detail?id=666839.

the bug is in the integrated assembler. That is what should be worked around.

please monitor https://codereview.chromium.org/2515503002/

[fnch, 2016-11-22 10:36:06]

Thanks, llozano.
The solution you favour sounds good to me. We can push this CL again, once the
clang integrated assembler is disabled and we are confident that this doesn't
cause any additional regressions.

[llozano, 2016-11-22 22:53:09]

On 2016/11/22 10:36:06, fnch wrote:
> Thanks, llozano.
> The solution you favour sounds good to me. We can push this CL again, once the
> clang integrated assembler is disabled and we are confident that this doesn't
> cause any additional regressions.

there is a fix in https://codereview.chromium.org/2522973002/

I have tried and it seems to work for me. 
Matteo, please give a try.
note that it is not disabling the integrated assembler because that did not
work..

[fnch, 2016-11-23 17:41:05]

Hi llonzano,
I applied the patch at https://codereview.chromium.org/2522973002/ on the top of
my tree. I then applied this CL (2457473003) on top of it. I built Chromium
manually on my ChromiumOS tree using the veyron_jerry overlay. This compiled
successfully. I only compiled the browser with Clang and tested it on a working
ChromiumOS filesystem (doing an sshfs mount). I tested the resulting binary by
running the SvgCubics benchmark (the one which shows the biggest improvement
after this CL is applied) and I can confirm that it ran with the expected
(improved) score, which is in line with what I get with gcc when running with
the system's Chromium. This is not a great deal of testing, but it seems to be
all successful.

I am also talking with some of our Clang guys concerning
https://llvm.org/bugs/show_bug.cgi?id=31058 and I just received an interesting
reply. They think that Clang is not strictly wrong in this case. The official
ARM syntax would require to pass three arguments to ldrexd ("LDREXD{<c>}{<q>}
<Rt>, <Rt2>, [<Rn>]" basically two 32-bit registers followed by the address) and
four arguments to strexd ("STREXD{<c>}{<q>} <Rd>, <Rt>, <Rt2>, [<Rn>]"). The
ability of passing a C uint64_t rather than the two corresponding 32-bit
registers seems to be a GAS extension. So we could even consider the alternative
of fixing
src/third_party/tcmalloc/chromium/src/base/atomicops-internal-arm-v6plus.h to
avoid the GAS-specific syntax. And what about src/base/atomicops.h? This seems
an alternative implementation of the same functionality which uses <atomic> (for
all compilers except MSVC). Is there any plan to replace the tcmalloc
implementation of atomicops.h with this?

[fnch, 2016-12-07 19:32:20]

matteo.franchin@arm.com changed reviewers:
+ llozano@chromium.org

[fnch, 2016-12-07 19:32:21]

The issue which caused this patch to be reverted (i.e. the breakage of the clang
build of ChromiumOS) has been fixed in
https://codereview.chromium.org/2522973002/, which landed last week and has not
caused troubles so far. I think we can consider pushing the tcmalloc fix again.

I rebased the patch. I also tested the patch by building ChromiumOS with clang
on my machine and it compiled fine. Luis, I would be grateful if you could
confirm that this patch can be relanded. Just to avoid causing any more
disturbance to the tree and its greenness :-)

[cavalcantii1, 2016-12-07 20:10:23]

Description was changed from

==========
tcmalloc: Use futex syscall in SpinLockDelay() for ARM.

SpinLockDelay() was avoiding using futex() when compiled for ARM.
The alternative implementation was sleeping for 2 ms. This caused
performance issues in Telemetry's SvgCubics benchmark. The score for
this benchmark more than doubles when this patch is applied (scores were
measured on veyron_jerry, veyron_minnie, daisy). Other benchmarks
(blink_perf.bindings/post-message, blink_perf.canvas/draw-static-webgl-
to-hw-accelerated-canvas-2d) benefit from this change to a lesser
extent. This patch also removes a reference to futex1 in a comment in
linux-syscall_support.h, as this is not a valid syscall name.

Note that a similar fix was pushed in the upstream version of tcmalloc
(issue-693, https://github.com/gperftools/gperftools/commit/7df7f14).

BUG=

Change-Id: I390ac51ed5e1b0ad021ac63eaf3bce81cdca8599
Committed: https://crrev.com/7f1336053e39737671a52ca7a265007e9024ee8a
Cr-Commit-Position: refs/heads/master@{#432628}
==========

to

==========
tcmalloc: Use futex syscall in SpinLockDelay() for ARM.

SpinLockDelay() was avoiding using futex() when compiled for ARM.
The alternative implementation was sleeping for 2 ms. This caused
performance issues in Telemetry's SvgCubics benchmark. The score for
this benchmark more than doubles when this patch is applied (scores were
measured on veyron_jerry, veyron_minnie, daisy). Other benchmarks
(blink_perf.bindings/post-message, blink_perf.canvas/draw-static-webgl-
to-hw-accelerated-canvas-2d) benefit from this change to a lesser
extent. This patch also removes a reference to futex1 in a comment in
linux-syscall_support.h, as this is not a valid syscall name.

Note that a similar fix was pushed in the upstream version of tcmalloc
(issue-693, https://github.com/gperftools/gperftools/commit/7df7f14).

BUG=

Change-Id: I390ac51ed5e1b0ad021ac63eaf3bce81cdca8599
Committed: https://crrev.com/7f1336053e39737671a52ca7a265007e9024ee8a
Cr-Commit-Position: refs/heads/master@{#432628}
==========

[cavalcantii1, 2016-12-07 20:10:32]

The CQ bit was checked by cavalcantii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 20:11:14]

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

[commit-bot: I haz the power, 2016-12-07 20:16:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 20:16:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[llozano, 2016-12-07 20:52:27]

On 2016/12/07 19:32:21, fnch wrote:
> The issue which caused this patch to be reverted (i.e. the breakage of the
clang
> build of ChromiumOS) has been fixed in
> https://codereview.chromium.org/2522973002/, which landed last week and has
not
> caused troubles so far. I think we can consider pushing the tcmalloc fix
again.
> 
> I rebased the patch. I also tested the patch by building ChromiumOS with clang
> on my machine and it compiled fine. Luis, I would be grateful if you could
> confirm that this patch can be relanded. Just to avoid causing any more
> disturbance to the tree and its greenness :-)

When I tested the fix by sbc, I tested it together with your change. Otherwise,
the problem does not reproduce (only reproduce with O0). 
So, I think you are ready to go.

[Will Harris, 2016-12-07 21:24:53]

still lgtm from me

[cavalcantii1, 2016-12-07 22:56:33]

Adding it to the CQ and let's see how it goes.

[cavalcantii1, 2016-12-07 22:56:50]

The CQ bit was checked by cavalcantii@chromium.org

[cavalcantii1, 2016-12-07 22:56:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, cavalcantii@chromium.org
Link to the patchset: https://codereview.chromium.org/2457473003/#ps100001
(title: "tcmalloc: Use futex syscall in SpinLockDelay() for ARM.")

[commit-bot: I haz the power, 2016-12-07 22:57:16]

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

[commit-bot: I haz the power, 2016-12-07 23:04:49]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481151410302800,
"parent_rev": "92ab682f5f758379493d7786881850727c8481bb", "commit_rev":
"1e00385c285b690238169e6bd9036bb686fd72ae"}

[commit-bot: I haz the power, 2016-12-07 23:05:16]

Description was changed from

==========
tcmalloc: Use futex syscall in SpinLockDelay() for ARM.

SpinLockDelay() was avoiding using futex() when compiled for ARM.
The alternative implementation was sleeping for 2 ms. This caused
performance issues in Telemetry's SvgCubics benchmark. The score for
this benchmark more than doubles when this patch is applied (scores were
measured on veyron_jerry, veyron_minnie, daisy). Other benchmarks
(blink_perf.bindings/post-message, blink_perf.canvas/draw-static-webgl-
to-hw-accelerated-canvas-2d) benefit from this change to a lesser
extent. This patch also removes a reference to futex1 in a comment in
linux-syscall_support.h, as this is not a valid syscall name.

Note that a similar fix was pushed in the upstream version of tcmalloc
(issue-693, https://github.com/gperftools/gperftools/commit/7df7f14).

BUG=

Change-Id: I390ac51ed5e1b0ad021ac63eaf3bce81cdca8599
Committed: https://crrev.com/7f1336053e39737671a52ca7a265007e9024ee8a
Cr-Commit-Position: refs/heads/master@{#432628}
==========

to

==========
tcmalloc: Use futex syscall in SpinLockDelay() for ARM.

SpinLockDelay() was avoiding using futex() when compiled for ARM.
The alternative implementation was sleeping for 2 ms. This caused
performance issues in Telemetry's SvgCubics benchmark. The score for
this benchmark more than doubles when this patch is applied (scores were
measured on veyron_jerry, veyron_minnie, daisy). Other benchmarks
(blink_perf.bindings/post-message, blink_perf.canvas/draw-static-webgl-
to-hw-accelerated-canvas-2d) benefit from this change to a lesser
extent. This patch also removes a reference to futex1 in a comment in
linux-syscall_support.h, as this is not a valid syscall name.

Note that a similar fix was pushed in the upstream version of tcmalloc
(issue-693, https://github.com/gperftools/gperftools/commit/7df7f14).

BUG=

Change-Id: I390ac51ed5e1b0ad021ac63eaf3bce81cdca8599
Committed: https://crrev.com/7f1336053e39737671a52ca7a265007e9024ee8a
Cr-Commit-Position: refs/heads/master@{#432628}
==========

[commit-bot: I haz the power, 2016-12-07 23:05:18]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-07 23:09:04]

Description was changed from

==========
tcmalloc: Use futex syscall in SpinLockDelay() for ARM.

SpinLockDelay() was avoiding using futex() when compiled for ARM.
The alternative implementation was sleeping for 2 ms. This caused
performance issues in Telemetry's SvgCubics benchmark. The score for
this benchmark more than doubles when this patch is applied (scores were
measured on veyron_jerry, veyron_minnie, daisy). Other benchmarks
(blink_perf.bindings/post-message, blink_perf.canvas/draw-static-webgl-
to-hw-accelerated-canvas-2d) benefit from this change to a lesser
extent. This patch also removes a reference to futex1 in a comment in
linux-syscall_support.h, as this is not a valid syscall name.

Note that a similar fix was pushed in the upstream version of tcmalloc
(issue-693, https://github.com/gperftools/gperftools/commit/7df7f14).

BUG=

Change-Id: I390ac51ed5e1b0ad021ac63eaf3bce81cdca8599
Committed: https://crrev.com/7f1336053e39737671a52ca7a265007e9024ee8a
Cr-Commit-Position: refs/heads/master@{#432628}
==========

to

==========
tcmalloc: Use futex syscall in SpinLockDelay() for ARM.

SpinLockDelay() was avoiding using futex() when compiled for ARM.
The alternative implementation was sleeping for 2 ms. This caused
performance issues in Telemetry's SvgCubics benchmark. The score for
this benchmark more than doubles when this patch is applied (scores were
measured on veyron_jerry, veyron_minnie, daisy). Other benchmarks
(blink_perf.bindings/post-message, blink_perf.canvas/draw-static-webgl-
to-hw-accelerated-canvas-2d) benefit from this change to a lesser
extent. This patch also removes a reference to futex1 in a comment in
linux-syscall_support.h, as this is not a valid syscall name.

Note that a similar fix was pushed in the upstream version of tcmalloc
(issue-693, https://github.com/gperftools/gperftools/commit/7df7f14).

BUG=

Change-Id: I390ac51ed5e1b0ad021ac63eaf3bce81cdca8599
Committed: https://crrev.com/7f1336053e39737671a52ca7a265007e9024ee8a
Committed: https://crrev.com/e787102a66a74471263456e204209ea6b29e2d53
Cr-Original-Commit-Position: refs/heads/master@{#432628}
Cr-Commit-Position: refs/heads/master@{#437099}
==========

[commit-bot: I haz the power, 2016-12-07 23:09:05]

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