Use 'local-exec' for threadlocal_heap_ in tcmalloc.

Use 'local-exec' TLS model if we are not using component build.
    
We can use a more restricted TLS model when we know the code will be in the executable, not in a shared library. This makes access to threadlocal_heap_ faster.

BUG=chromium:650546

[chihchung, 2016-09-26 12:25:06]

Description was changed from

==========
Use 'local-exec' for threadlocal_heap_ in tcmalloc.

BUG=
==========

to

==========
Use 'local-exec' TLS model if we are not using component build.
    
We can use a more restricted TLS model when we know the code will be in the
executable, not in a shared library. This makes access to threadlocal_heap_
faster.
==========

[chihchung, 2016-09-26 12:25:06]

chihchung@chromium.org changed reviewers:
+ wfh@chromium.org

[Will Harris, 2016-09-26 18:29:37]

BUG= ?

[chihchung, 2016-09-26 18:54:32]

Description was changed from

==========
Use 'local-exec' TLS model if we are not using component build.
    
We can use a more restricted TLS model when we know the code will be in the
executable, not in a shared library. This makes access to threadlocal_heap_
faster.
==========

to

==========
Use 'local-exec' TLS model if we are not using component build.
    
We can use a more restricted TLS model when we know the code will be in the
executable, not in a shared library. This makes access to threadlocal_heap_
faster.

BUG=chrome-os-partner:56243
==========

[chihchung, 2016-09-26 18:56:27]

Description was changed from

==========
Use 'local-exec' TLS model if we are not using component build.
    
We can use a more restricted TLS model when we know the code will be in the
executable, not in a shared library. This makes access to threadlocal_heap_
faster.

BUG=chrome-os-partner:56243
==========

to

==========
Use 'local-exec' TLS model if we are not using component build.
    
We can use a more restricted TLS model when we know the code will be in the
executable, not in a shared library. This makes access to threadlocal_heap_
faster.
==========

[chihchung, 2016-09-26 18:59:36]

On 2016/09/26 18:29:37, Will Harris wrote:
> BUG= ?

This is found when I tried to look at improving performance for ChromeOS on ARM,
but I think it also applies elsewhere. The original bug is
chrome-os-partner:56243 , but it doesn't seem to be very relevant. Do you want
me to use that bug, create a new one, or not BUG= is ok? Thanks.

[chihchung, 2016-09-26 19:20:22]

On 2016/09/26 18:59:36, chihchung wrote:
> On 2016/09/26 18:29:37, Will Harris wrote:
> > BUG= ?
> 
> This is found when I tried to look at improving performance for ChromeOS on
ARM,
> but I think it also applies elsewhere. The original bug is
> chrome-os-partner:56243 , but it doesn't seem to be very relevant. Do you want
> me to use that bug, create a new one, or not BUG= is ok? Thanks.

Another related bug is chromium:650137, but that is a toolchain correctness
issue.

[Will Harris, 2016-09-26 19:33:06]

I'm happy with either a new comment on chrome-os-partner:56243 saying you've
found an additional optimization, or a new bug, but I feel there really needs to
be a bug that explains this in more detail and also the reasoning behind it.

[chihchung, 2016-09-27 06:10:29]

Description was changed from

==========
Use 'local-exec' TLS model if we are not using component build.
    
We can use a more restricted TLS model when we know the code will be in the
executable, not in a shared library. This makes access to threadlocal_heap_
faster.
==========

to

==========
Use 'local-exec' TLS model if we are not using component build.
    
We can use a more restricted TLS model when we know the code will be in the
executable, not in a shared library. This makes access to threadlocal_heap_
faster.

BUG=chromium:650546
==========

[chihchung, 2016-09-27 06:11:41]

I created a new bug and update the message. Please take a look. Thanks!

[Will Harris, 2016-09-27 21:20:13]

lgtm I'd be interested if this impacts on the perf bots at all

[chihchung, 2016-09-27 23:41:55]

CL for src perf tryjob to run octane benchmark on all platform(s)

[chihchung, 2016-09-28 13:04:08]

libosmesa.so fails to build because it includes tcmalloc.

[chihchung, 2016-09-28 13:08:04]

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

[chihchung, 2016-09-28 13:08:05]

Hi Primiano,

I see you did lots of allocator reorg in 564618, so I am seeking you advise. My
question is, why does libosmesa.so needs to have its own copy of allocator and
not just using the one provided by the executable?

Thank,
Chih-Chung Chang

[Primiano Tucci (use gerrit), 2016-09-28 13:28:02]

On 2016/09/28 13:08:05, chihchung wrote:
> Hi Primiano,
> 
> I see you did lots of allocator reorg in 564618, so I am seeking you advise.
My
> question is, why does libosmesa.so needs to have its own copy of allocator and
> not just using the one provided by the executable?
> 
> Thank,
> Chih-Chung Chang

Hmm from https://codereview.chromium.org/1616793003:
-----
Some targets (clearkeycdmadapter, osmesa) now fell in the state where
they depend indirectly on base (which causes the removal of the default
clib) but via a shared_library boundary (which does not propagate the
replacement allocator / shim layer code).
Fixing those targets by adding a direct base dependency.
-----

I think this was a warkround about the windows shim previously was removing
references to libc. Probably this is not needed anymore after the new shim.
I agree with you that a .so should not have allocator symbols. I think we just
did that because we had just two snowflakes and that was the easy way to get
around the problem.

Can you try undoing the change in
https://codereview.chromium.org/1616793003/diff/20002/third_party/mesa/BUILD.gn
?

[chihchung, 2016-09-28 13:37:31]

Thanks for the answer. I'll try.

[chihchung, 2016-09-28 14:26:14]

CL for src perf tryjob to run octane benchmark on all platform(s)

[laszio, 2017-01-18 22:27:28]

On 2016/09/28 14:26:14, chihchung wrote:
> CL for src perf tryjob to run octane benchmark on all platform(s)

Hi Chihchung,

May I know the status of this CL? It seems that this brings some performance
benefits to kevin; I compared the profiles of kevin-gcc-image and
kevin-llvm-image and the most different, top functions come from tcmalloc.

[chihchung, 2017-01-19 02:23:47]

On 2017/01/18 22:27:28, laszio wrote:
> On 2016/09/28 14:26:14, chihchung wrote:
> > CL for src perf tryjob to run octane benchmark on all platform(s)
> 
> Hi Chihchung,
> 
> May I know the status of this CL? It seems that this brings some performance
> benefits to kevin; I compared the profiles of kevin-gcc-image and
> kevin-llvm-image and the most different, top functions come from tcmalloc.

Sorry I was busy with something else. I'll look at it today.

[chihchung, 2017-01-19 06:17:05]

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

[commit-bot: I haz the power, 2017-01-19 06:17:33]

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, 2017-01-19 06:29:26]

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

[commit-bot: I haz the power, 2017-01-19 06:29:27]

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

[chihchung, 2017-01-19 07:51:04]

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

[commit-bot: I haz the power, 2017-01-19 07:51:20]

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, 2017-01-19 07:55:21]

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

[commit-bot: I haz the power, 2017-01-19 07:55:22]

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

[chihchung, 2017-01-19 08:13:06]

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

[commit-bot: I haz the power, 2017-01-19 08:13:15]

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, 2017-01-19 08:23:55]

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

[commit-bot: I haz the power, 2017-01-19 08:23:56]

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

[chihchung, 2017-01-19 08:28:17]

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

[commit-bot: I haz the power, 2017-01-19 08:28:29]

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, 2017-01-19 08:40:03]

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

[commit-bot: I haz the power, 2017-01-19 08:40:03]

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

[chihchung, 2017-01-19 08:51:47]

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

[commit-bot: I haz the power, 2017-01-19 08:52:01]

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, 2017-01-19 09:03:23]

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

[commit-bot: I haz the power, 2017-01-19 09:03:24]

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

[chihchung, 2017-01-19 09:45:20]

Currently these targets failed to build for local-exec (because they are shared
libraries):

libcast_graphics_1.0.so
libclearkeycdm.so

I think they don't intend to override the malloc implementation with tcmalloc,
but because they depend on //base, they actually do that.

primiano@, any idea how to proceed? Removing dependencies of //base does not
work as they actually depend on some things in that.

[Primiano Tucci (use gerrit), 2017-01-19 10:45:28]

On 2017/01/19 09:45:20, chihchung wrote:
> Currently these targets failed to build for local-exec (because they are
shared
> libraries):
> 
> libcast_graphics_1.0.so
> libclearkeycdm.so
> 
> I think they don't intend to override the malloc implementation with tcmalloc,
> but because they depend on //base, they actually do that.
> 
> primiano@, any idea how to proceed? Removing dependencies of //base does not
> work as they actually depend on some things in that.

I wonder if https://codereview.chromium.org/2429773002/ would solve this, I
think I did the surgery there to remove the base dep but then forgot about that
CL.
can you confirm if it that works?

[chihchung, 2017-01-19 11:04:16]

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

[commit-bot: I haz the power, 2017-01-19 11:04:32]

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, 2017-01-19 11:15:21]

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

[commit-bot: I haz the power, 2017-01-19 11:15:22]

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

[chihchung, 2017-01-19 11:28:04]

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

[commit-bot: I haz the power, 2017-01-19 11:28:16]

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

[chihchung, 2017-01-19 11:29:28]

On 2017/01/19 10:45:28, Primiano Tucci wrote:
> On 2017/01/19 09:45:20, chihchung wrote:
> > Currently these targets failed to build for local-exec (because they are
> shared
> > libraries):
> > 
> > libcast_graphics_1.0.so
> > libclearkeycdm.so
> > 
> > I think they don't intend to override the malloc implementation with
tcmalloc,
> > but because they depend on //base, they actually do that.
> > 
> > primiano@, any idea how to proceed? Removing dependencies of //base does not
> > work as they actually depend on some things in that.
> 
> I wonder if https://codereview.chromium.org/2429773002/ would solve this, I
> think I did the surgery there to remove the base dep but then forgot about
that
> CL.
> can you confirm if it that works?

Thanks for help. I tried it but it is the same. It seems that change removes
dependencies for clearkeycdmadapter but not clearkeycdm.

[commit-bot: I haz the power, 2017-01-19 11:40:51]

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

[commit-bot: I haz the power, 2017-01-19 11:40:52]

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

[Primiano Tucci (use gerrit), 2017-01-19 12:53:08]

On 2017/01/19 11:29:28, chihchung wrote:
> On 2017/01/19 10:45:28, Primiano Tucci wrote:
> > On 2017/01/19 09:45:20, chihchung wrote:
> > > Currently these targets failed to build for local-exec (because they are
> > shared
> > > libraries):
> > > 
> > > libcast_graphics_1.0.so
> > > libclearkeycdm.so
> > > 
> > > I think they don't intend to override the malloc implementation with
> tcmalloc,
> > > but because they depend on //base, they actually do that.
> > > 
> > > primiano@, any idea how to proceed? Removing dependencies of //base does
not
> > > work as they actually depend on some things in that.
> > 
> > I wonder if https://codereview.chromium.org/2429773002/ would solve this, I
> > think I did the surgery there to remove the base dep but then forgot about
> that
> > CL.
> > can you confirm if it that works?
> 
> Thanks for help. I tried it but it is the same. It seems that change removes
> dependencies for clearkeycdmadapter but not clearkeycdm.

Oh I see.
well the problem is that in the current architecture you cannot depend on base
and NOT get tcmalloc.
Why that is creating problems? It is already the case for all the translation
unit.
Mind making a quick summary of the problem, so I can see if we can solve this
problem differently?

[chihchung, 2017-01-19 13:59:19]

> Oh I see.
> well the problem is that in the current architecture you cannot depend on base
> and NOT get tcmalloc.
> Why that is creating problems? It is already the case for all the translation
> unit.
> Mind making a quick summary of the problem, so I can see if we can solve this
> problem differently?

The goal is making tcmalloc use the 'local-exec' TLS model. That is faster and
workaround a gcc bug. However that requires the tcmalloc code (itself, not the
caller) to be in an executable, not in a shared library, otherwise it fails at
linking stage. But the above .so targets include the tcmalloc code, so they fail
to build with the change.

[Primiano Tucci (use gerrit), 2017-01-19 14:27:20]

On 2017/01/19 13:59:19, chihchung wrote:
> > Oh I see.
> > well the problem is that in the current architecture you cannot depend on
base
> > and NOT get tcmalloc.
> > Why that is creating problems? It is already the case for all the
translation
> > unit.
> > Mind making a quick summary of the problem, so I can see if we can solve
this
> > problem differently?
> 
> The goal is making tcmalloc use the 'local-exec' TLS model. That is faster and
> workaround a gcc bug. However that requires the tcmalloc code (itself, not the
> caller) to be in an executable, not in a shared library, otherwise it fails at
> linking stage. But the above .so targets include the tcmalloc code, so they
fail
> to build with the change.

Okay, yes, the fact that tcmalloc end up being in .so(s) is undesirable. 
However I am still not sure how we end up in these cases, I though I was fixing
all them in that CL I linked.
The current rationale is:
- base is what gives tcmalloc
- in a component build, tcmalloc ends up in libbase.so, and all the other
targets end up depending on libbase.so. So there is only one allocator defined
in libbase.so that everybody ends up using
- in a static build the allocator is in libbase.a, everything gets linked
together, so tcmalloc is in the linked executable.

So, let me se if I read this correctly: is the problem here the fact that even
in static (NON-component) builds we have some .so targets that depend on base
and inherit tcmalloc?
If this is the case this is going to be quite tricky to solve.

[chihchung, 2017-01-19 15:03:59]

> So, let me se if I read this correctly: is the problem here the fact that even
> in static (NON-component) builds we have some .so targets that depend on base
> and inherit tcmalloc?
> If this is the case this is going to be quite tricky to solve.

Yes, the change is wrapped in an "#if !defined(COMPONENT_BUILD)", but there are
still .so fail to build because they include tcmalloc code.

[Primiano Tucci (use gerrit), 2017-01-19 17:11:26]

On 2017/01/19 15:03:59, chihchung wrote:
> > So, let me se if I read this correctly: is the problem here the fact that
even
> > in static (NON-component) builds we have some .so targets that depend on
base
> > and inherit tcmalloc?
> > If this is the case this is going to be quite tricky to solve.
> 
> Yes, the change is wrapped in an "#if !defined(COMPONENT_BUILD)", but there
are
> still .so fail to build because they include tcmalloc code.

Ok let me be clear, if what you want to achieve is avoiding that tcmalloc gets
linked in a .so which is built in the tree even in static build I don't see any
solution other than:
A) refactor the target that generates the .so in a way that makes it NOT
directly depend on base (I think there is some precedent in
media/cdm/cdm_wrapper.h but I really don't know the design of that code)
B) refactor base and the allocator, introduce a base-without-allocator target,
change the BUILD.gn files for the target.so to depend on base-without-allocator

B is quite long, has a lot of subtleties because base itself has intrinsic
dependencies to the allocator (see crbug.com/642879) and might be controversial
for base owners, so should first be discussed with them (switching the TLS model
on one platform might not be a sufficient motivation for the extra work and
extra codebase complexity). 
Unfortunately I don't see other solutions right now. But happy to reason more on
this if you have other proposals.

[chihchung, 2017-01-20 02:40:35]

On 2017/01/19 17:11:26, Primiano Tucci wrote:
> On 2017/01/19 15:03:59, chihchung wrote:
> > > So, let me se if I read this correctly: is the problem here the fact that
> even
> > > in static (NON-component) builds we have some .so targets that depend on
> base
> > > and inherit tcmalloc?
> > > If this is the case this is going to be quite tricky to solve.
> > 
> > Yes, the change is wrapped in an "#if !defined(COMPONENT_BUILD)", but there
> are
> > still .so fail to build because they include tcmalloc code.
> 
> Ok let me be clear, if what you want to achieve is avoiding that tcmalloc gets
> linked in a .so which is built in the tree even in static build I don't see
any
> solution other than:
> A) refactor the target that generates the .so in a way that makes it NOT
> directly depend on base (I think there is some precedent in
> media/cdm/cdm_wrapper.h but I really don't know the design of that code)
> B) refactor base and the allocator, introduce a base-without-allocator target,
> change the BUILD.gn files for the target.so to depend on
base-without-allocator
> 
> B is quite long, has a lot of subtleties because base itself has intrinsic
> dependencies to the allocator (see crbug.com/642879) and might be
controversial
> for base owners, so should first be discussed with them (switching the TLS
model
> on one platform might not be a sufficient motivation for the extra work and
> extra codebase complexity). 
> Unfortunately I don't see other solutions right now. But happy to reason more
on
> this if you have other proposals.

(Sorry I didn't read the long crbug.com/642879 fully...)
I think for the current issue, the other CL (making the TLS model "default")
should be enough.
In the long term base-without-allocator seems to be the way to go.

[Primiano Tucci (use gerrit), 2017-02-07 11:06:51]

Can this be closed now that https://codereview.chromium.org/2640263003/ haz
landed?