Add use_allocator instead of linux_use_tcmalloc to switch the allocator.

Add use_allocator instead of linux_use_tcmalloc to switch the allocator.

This change is to add a new build option 'use_allocator' which will
replace 'linux_use_tcmalloc' in the future. It doesn't change the
behavior immediately. The migration plan is as follows:

1) (this change)
... Add 'use_allocator' and set its default to "see_use_tcmalloc".
... Change allocator conditions to check use_allocator firstly.
... Use linux_use_tcmalloc if use_allocator=="see_use_tcmalloc".
... NO IMPACT without specifying use_allocator explicitly.
2) Change Blink to accept use_allocator. http://crrev.com/177053003/
3) Change gyp to accept use_allocator. http://crrev.com/178643004/
4) PSA the transition period to chromium-dev@.
5) (after the PSA-ed transition period)
... Make 'use_allocator' to "tcmalloc" or "none" (it depends) by default.
... Remove all linux_use_tcmalloc.
... Assert in gyp_chromium to check if linux_use_tcmalloc is not specified.

At the point of this change (1), linux_use_tcmalloc is still used by default
because 'use_allocator%': "see_use_tcmalloc".

As written in http://crbug.com/345554, linux_use_tcmalloc would
be confusing to have more options about allocators. We plan to:
A) enable gperftools' heap-profiler with non-tcmalloc allocator,
B) add a new memory allocator instead of tcmalloc.

BUG=345554, 339604, 341349
R=agl@chromium.org, brettw@chromium.org, dgarrett@chromium.org, jam@chromium.org, jamesr@chromium.org, joi@chromium.org, miket@chromium.org, nick@chromium.org, rsleevi@chromium.org, scherkus@chromium.org, sergeyu@chromium.org, shess@chromium.org, sievers@chromium.org, sky@chromium.org, vitalybuka@chromium.org, willchan@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255129

[Dai Mikurube (NOT FULLTIME), 2014-02-24 09:01:01]

Hi willchan@ and thakis@,

To enable a new allocator and a new allocator build option more easily, I
suggested to replace the build option linux_use_tcmalloc at
http://crbug.com/339604.

Here, I just tried implementing one option of gyp changes. What do you think
about it?

If we need a deeper discussion, let's discuss it at http://crbug.com/345554.

[Dai Mikurube (NOT FULLTIME), 2014-02-25 03:45:47]

(+primiano and scarybeasts)

[Dai Mikurube (NOT FULLTIME), 2014-02-25 09:08:57]

Ah, it may not work. I'll check it later...

[Dai Mikurube (NOT FULLTIME), 2014-02-25 09:16:59]

Oops, it was a mistake in my local build script. No problems in this patch. :)

[Dai Mikurube (NOT FULLTIME), 2014-02-28 23:49:57]

Adding many OWNERS of many gyp files... Could you please take an OWNER look on
your own gyp file(s)?

See the change description and http://crbug.com/345554 for the details. The main
owner of linux_use_tcmalloc, willchan@, is ooo for a while, but he's mostly
positive on this kind of change. http://crbug.com/345554#c8

You see the new condition expr is so long. But, note that it's just during the
transition period, and linux_use_tcmalloc is eventually removed after the
transition period. The condition will be finally just like
'use_allocator!="none"'.

[miket_OOO, 2014-03-01 00:02:01]

On 2014/02/28 23:49:57, Dai Mikurube wrote:
> Adding many OWNERS of many gyp files... Could you please take an OWNER look on
> your own gyp file(s)?
> 
> See the change description and http://crbug.com/345554 for the details. The
main
> owner of linux_use_tcmalloc, willchan@, is ooo for a while, but he's mostly
> positive on this kind of change. http://crbug.com/345554#c8
> 
> You see the new condition expr is so long. But, note that it's just during the
> transition period, and linux_use_tcmalloc is eventually removed after the
> transition period. The condition will be finally just like
> 'use_allocator!="none"'.

message_center.gyp LGTM

[dgarrett-goog, 2014-03-01 00:08:53]

courgette.gyp LGTM

[willchan no longer on Chromium, 2014-03-01 00:10:37]

Plan SGTM and net/ && base/ LGTM

[Dai Mikurube (NOT FULLTIME), 2014-03-01 00:12:42]

dgarrett:

Thanks, but sorry that we'd need your @chromium.org comment as an owner.

[jamesr, 2014-03-01 00:12:46]

lgtm

[brettw, 2014-03-01 00:35:00]

lgtm

[darin (slow to review), 2014-03-01 00:38:55]

https://codereview.chromium.org/177353002/diff/160001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/177353002/diff/160001/ash/ash.gyp#newcode1029
ash/ash.gyp:1029: # TODO(dmikurube): Kill linux_use_tcmalloc.
http://crbug.com/345554
maybe this GYP block would be better as a .gypi that other can include? there is
a lot of duplication of this snippet across the codebase.

[dgarrett, 2014-03-01 00:46:34]

On 2014/03/01 00:08:53, dgarrett-goog wrote:
> courgette.gyp LGTM

My mistake:

courgette.gyp LGTM

[Sergey Ulanov, 2014-03-01 00:47:23]

lgtm

[Dai Mikurube (NOT FULLTIME), 2014-03-01 01:01:18]

Thanks, Darin.

https://codereview.chromium.org/177353002/diff/160001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/177353002/diff/160001/ash/ash.gyp#newcode1029
ash/ash.gyp:1029: # TODO(dmikurube): Kill linux_use_tcmalloc.
http://crbug.com/345554
On 2014/03/01 00:38:56, darin wrote:
> maybe this GYP block would be better as a .gypi that other can include? there
is
> a lot of duplication of this snippet across the codebase.

Sure. Maybe. But actually, it's not trivial since they have some differences per
platform and/or other conditions. I and primiano@ plan a gyp refactoring later
(discussed in the bug), but we need use_allocator earlier.

[scherkus (not reviewing), 2014-03-01 01:04:47]

media lgtm

[no sievers, 2014-03-01 01:20:31]

gpu lgtm

[Ryan Sleevi, 2014-03-01 01:47:33]

net/ LGTM

[darin (slow to review), 2014-03-01 05:05:11]

On 2014/03/01 01:01:18, Dai Mikurube wrote:
> Thanks, Darin.
> 
> https://codereview.chromium.org/177353002/diff/160001/ash/ash.gyp
> File ash/ash.gyp (right):
> 
> https://codereview.chromium.org/177353002/diff/160001/ash/ash.gyp#newcode1029
> ash/ash.gyp:1029: # TODO(dmikurube): Kill linux_use_tcmalloc.
> http://crbug.com/345554
> On 2014/03/01 00:38:56, darin wrote:
> > maybe this GYP block would be better as a .gypi that other can include?
there
> is
> > a lot of duplication of this snippet across the codebase.
> 
> Sure. Maybe. But actually, it's not trivial since they have some differences
per
> platform and/or other conditions. I and primiano@ plan a gyp refactoring later
> (discussed in the bug), but we need use_allocator earlier.

OK, I had only spot-checked a few, and they looked similar. No problem.

If you still need a review on portions of this CL, just let me know.

-Darin

[Jói, 2014-03-03 12:54:30]

//components LGTM

[agl, 2014-03-03 13:50:10]

lgtm

[jam, 2014-03-03 16:14:43]

content lgtm

[Dai Mikurube (NOT FULLTIME), 2014-03-03 22:50:08]

Adding some more OWNERS, vitalybuka@ for cloud_print and printing, shess@ for
sql and atwilson@ for sync.

Could you take a look for your own gyp(s)?


Darin,

Thanks. I think we can go ahead as-is since only similar gyps are remaining.

[Scott Hess - ex-Googler, 2014-03-03 22:52:18]

lgtm for sql/sql.gyp

[Vitaly Buka (NO REVIEWS), 2014-03-03 22:54:58]

lgtm

[Dai Mikurube (NOT FULLTIME), 2014-03-04 00:10:22]

Thanks, everyone! :)

Adding nick@ for sync/. (atwilson@ may be ooo or on a trip.) Could you take an
OWNERS look for sync/sync_tests.gyp?

[ncarter (slow), 2014-03-04 20:12:38]

lgtm

[Dai Mikurube (NOT FULLTIME), 2014-03-04 20:17:10]

Thanks everyone! It's now all lgtm'ed.

I'll be committing...

[Dai Mikurube (NOT FULLTIME), 2014-03-04 20:17:26]

The CQ bit was checked by dmikurube@chromium.org

[commit-bot: I haz the power, 2014-03-04 20:20:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmikurube@chromium.org/177353002/200001

[commit-bot: I haz the power, 2014-03-04 20:42:10]

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

[commit-bot: I haz the power, 2014-03-04 20:42:11]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Dai Mikurube (NOT FULLTIME), 2014-03-04 20:44:37]

Hmm, the Butler reported different reviewers to be reviewed...

ben@, sky@, could you rubber-stamp on ui/ gyp files?

[sky, 2014-03-04 22:19:06]

+mark

https://codereview.chromium.org/177353002/diff/200001/ui/keyboard/keyboard.gyp
File ui/keyboard/keyboard.gyp (right):

https://codereview.chromium.org/177353002/diff/200001/ui/keyboard/keyboard.gy...
ui/keyboard/keyboard.gyp:104: ['OS=="linux" and ((use_allocator!="none" and
use_allocator!="see_use_tcmalloc") or (use_allocator=="see_use_tcmalloc" and
linux_use_tcmalloc==1))', {
Oy, isn't there a better way to express this long if once and not duplicate it
in a ton of files?

[jamesr, 2014-03-04 22:20:17]

Not in gyp.   If we had a better build system yes, but we don't.

[sky, 2014-03-04 22:25:20]

*SIGH* LGTM then

[Mark Mentovai, 2014-03-04 22:26:36]

Thanks, James, that was totally constructive and worth the time of all 29 of us.

Scott, yes, you can set a variable conditionally. If it needs to be global, you
could use common.gypi.

  'variables': {
    'conditions': [
      ['some_long_conditon==1 and rather_not_duplicate==1', {
        'my_new_variable': 1,
      }, {  # else
        'my_new_variable': 0,
      }],
    ],
  }

[Dai Mikurube (NOT FULLTIME), 2014-03-04 22:27:52]

Thanks, sky@ and jamesr@.

https://codereview.chromium.org/177353002/diff/200001/ui/keyboard/keyboard.gyp
File ui/keyboard/keyboard.gyp (right):

https://codereview.chromium.org/177353002/diff/200001/ui/keyboard/keyboard.gy...
ui/keyboard/keyboard.gyp:104: ['OS=="linux" and ((use_allocator!="none" and
use_allocator!="see_use_tcmalloc") or (use_allocator=="see_use_tcmalloc" and
linux_use_tcmalloc==1))', {
On 2014/03/04 22:19:07, sky wrote:
> Oy, isn't there a better way to express this long if once and not duplicate it
> in a ton of files?

Sad news that we don't have that in gyp. :(  But :

1) The "long if" will be shortened into a single "use_allocator" statement after
the transition period.
2) The duplication in a ton of files will be refactored in the future plan. It's
not very trivial. Please see the bug.

[jamesr, 2014-03-04 22:30:49]

On 2014/03/04 22:26:36, Mark Mentovai wrote:
> Thanks, James, that was totally constructive and worth the time of all 29 of
us.

Well it's true.

> Scott, yes, you can set a variable conditionally. If it needs to be global,
you
> could use common.gypi.
> 
>   'variables': {
>     'conditions': [
>       ['some_long_conditon==1 and rather_not_duplicate==1', {

That would be possible if all the variables that the conditional depended on
were fully evaluated at the time this clause in common.gypi was expanded, which
I believe is not the case in this scenario.  The expansion order of variables in
gyp is really constraining here (see the 5 levels of nesting of 'variables' in
common.gypi for another side effect of this).

Ideally none of this would be needed and all targets of type 'executable' would
have this expression set but that's completely impossible to express in gyp.

[Dai Mikurube (NOT FULLTIME), 2014-03-04 22:49:25]

I think so, too. I tried a similar way locally to be more simple, but it failed.

I'm going ahead with this, and I'll remove linux_use_tcmalloc later after the
transition period. If something is critically wrong, please stop CQ.

[Dai Mikurube (NOT FULLTIME), 2014-03-04 22:49:37]

The CQ bit was checked by dmikurube@chromium.org

[commit-bot: I haz the power, 2014-03-04 22:50:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmikurube@chromium.org/177353002/200001

[Mark Mentovai, 2014-03-04 23:00:52]

No, you can’t simplify this to the point that you can automatically inject
this dependency into all executables. But the long duplicated condition
that’s replacing linux_use_tcmalloc everywhere can certainly be simplified
from the long form that it’s now using to a short single-variable form.

Is it correct to inject this dependency only into executables? What about
shared libraries and loadable modules?


On Tue, Mar 4, 2014 at 5:30 PM, <jamesr@chromium.org> wrote:

> On 2014/03/04 22:26:36, Mark Mentovai wrote:
>
>> Thanks, James, that was totally constructive and worth the time of all 29
>> of
>>
> us.
>
> Well it's true.
>
>
>  Scott, yes, you can set a variable conditionally. If it needs to be
>> global,
>>
> you
>
>> could use common.gypi.
>>
>
>     'variables': {
>>      'conditions': [
>>        ['some_long_conditon==1 and rather_not_duplicate==1', {
>>
>
> That would be possible if all the variables that the conditional depended
> on
> were fully evaluated at the time this clause in common.gypi was expanded,
> which
> I believe is not the case in this scenario.  The expansion order of
> variables in
> gyp is really constraining here (see the 5 levels of nesting of
> 'variables' in
> common.gypi for another side effect of this).
>
> Ideally none of this would be needed and all targets of type 'executable'
> would
> have this expression set but that's completely impossible to express in
> gyp.
>
> https://codereview.chromium.org/177353002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jamesr, 2014-03-04 23:33:45]

On Tue, Mar 4, 2014 at 3:00 PM, Mark Mentovai <mark@chromium.org> wrote:

> No, you can’t simplify this to the point that you can automatically inject
> this dependency into all executables. But the long duplicated condition
> that’s replacing linux_use_tcmalloc everywhere can certainly be simplified
> from the long form that it’s now using to a short single-variable form.
>
> Is it correct to inject this dependency only into executables? What about
> shared libraries and loadable modules?
>

We're getting off topic now, so feel free to skip the rest of this reply if
you aren't interested in this discussion.  It doesn't impact this patch
(IMO)

The answer to your question "is it correct to ..." is yes.  The decision of
what allocator to use is a decision that only an executable can decide (see
comment in allocator.gyp).  A shared library or loadable module can't use a
different allocator from other libraries or the executable it is loaded
into since our APIs depend on passing heap-allocated pointers around
between libraries and free() doing the same thing in all of them.  What
this condition is really trying to express is "if you are an executable
using tcmalloc as your allocator, declare a dependency on this target" but
as you've seen there is not a very concise way to express that right now.
 This patch contains several different long
nearly-duplicated-but-not-quite-the-same conditions to express this.  I
trust Dai to clean this up soon since he's had to deal with plenty of
fallout of different targets getting this subtly wrong.

My ideal solution would be a way to express somewhere that all executable
targets within chromium on configurations that use tcmalloc (and possibly
other allocators in the future, depending on how they are set up) have a
dependency on base/allocator.gyp:allocator since that's the policy we want
in chromium.  The executables have to declare a dependency themselves,
since allocator is a static library that has to be linked into the
executable and not into some other shared library.  I think I tried at one
point making targets depend on allocator.gyp unconditionally and mutating
the type of the target, but ran into variable expansion ordering issues
that were not surmountable.  Dai's plan in the code review description will
get us to the next-best place IMO.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-03-05 01:09:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmikurube@chromium.org/177353002/200001

[commit-bot: I haz the power, 2014-03-05 06:30:13]

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

[commit-bot: I haz the power, 2014-03-05 06:30:14]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[Mark Mentovai, 2014-03-05 14:35:12]

James Robinson wrote:

> We're getting off topic now, so feel free to skip the rest of this reply
> if you aren't interested in this discussion.  It doesn't impact this patch
> (IMO)
>
> The answer to your question "is it correct to ..." is yes.  The decision
> of what allocator to use is a decision that only an executable can decide
> (see comment in allocator.gyp).  A shared library or loadable module can't
> use a different allocator from other libraries or the executable it is
> loaded into since our APIs depend on passing heap-allocated pointers around
> between libraries and free() doing the same thing in all of them.  What
> this condition is really trying to express is "if you are an executable
> using tcmalloc as your allocator, declare a dependency on this target" but
> as you've seen there is not a very concise way to express that right now.
>  This patch contains several different long
> nearly-duplicated-but-not-quite-the-same conditions to express this.  I
> trust Dai to clean this up soon since he's had to deal with plenty of
> fallout of different targets getting this subtly wrong.
>
> My ideal solution would be a way to express somewhere that all executable
> targets within chromium on configurations that use tcmalloc (and possibly
> other allocators in the future, depending on how they are set up) have a
> dependency on base/allocator.gyp:allocator since that's the policy we want
> in chromium.  The executables have to declare a dependency themselves,
> since allocator is a static library that has to be linked into the
> executable and not into some other shared library.  I think I tried at one
> point making targets depend on allocator.gyp unconditionally and mutating
> the type of the target, but ran into variable expansion ordering issues
> that were not surmountable.  Dai's plan in the code review description will
> get us to the next-best place IMO.
>

We’ve come across this situation before for asan, and there’s been some
work on making the “make all targets depend on it” approach work. See
https://code.google.com/p/gyp/issues/detail?id=380 (a background bug on
dependencies inside “target_conditions”) and
https://code.google.com/p/gyp/issues/detail?id=409&q=glider (about putting
dependencies into “target_defaults”).

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dai Mikurube (NOT FULLTIME), 2014-03-05 20:07:37]

Committed patchset #14 manually as r255129 (presubmit successful).

[Dai Mikurube (NOT FULLTIME), 2014-04-24 08:12:55]

+mseaborn

[Dai Mikurube (NOT FULLTIME), 2014-04-24 08:14:18]

oops, sorry, I wrongly commented on an old patchset...