Remove {linux|android}_use_tcmalloc and switch to use_allocator in Chromium.

Remove {linux|android}_use_tcmalloc and switch to use_allocator in Chromium.

If this change breaks some bots, please try restarting the 
bot before reverting it. http://crrev.com/264460 may not be 
effective yet until restarting.

See the bug and http://crrev.com/255129 for the details.

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

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

Hi gyp file OWNERS,

Could you take a look at this patchset?

It's a final clean-up of http://crrev.com/177353002 to remove *_use_tcmalloc and
complicated gyp expressions introduced there.

* A patchset for Blink (http://crrev.com/259543003/) should be landed before
this patchset.

* Buildbots and trybots have already been switched. (If you find something still
using *_use_tcmalloc, please let me know that!)

[Dai Mikurube (NOT FULLTIME), 2014-04-24 08:13:38]

+mseaborn

[Jói, 2014-04-24 08:18:02]

-joi, +blundell for components/OWNERS

[sky, 2014-04-24 16:08:34]

LGTM

[Vitaly Buka (NO REVIEWS), 2014-04-24 16:11:26]

LGTM printing/ cloud_print/

[jam, 2014-04-24 16:16:56]

chrome, content, ipc lgtm

[Mark Seaborn, 2014-04-24 16:36:21]

On 2014/04/24 08:13:38, Dai Mikurube wrote:
> +mseaborn

LGTM for components/nacl/.

[scherkus (not reviewing), 2014-04-24 17:08:17]

media lgtm

[enne (OOO), 2014-04-24 17:14:18]

lgtm cc webkit/renderer/compositor_bindings

[Lambros, 2014-04-24 18:20:17]

remoting/ LGTM

[miket_OOO, 2014-04-24 20:34:30]

ui/message_center LGTM

[willchan no longer on Chromium, 2014-04-24 20:38:10]

lgtm

[Scott Hess - ex-Googler, 2014-04-24 21:57:32]

Rubber-stamp LGTM, with "I don't know what I'm doing" dog as the relevant image.

[Ryan Sleevi, 2014-04-25 00:29:47]

I'm not happy with this, but in part it's a pre-existing problem.

The use of use_allocator is inconsistent throughout the GYP files, and I cannot
help but feel it will lead to subtle bugs - if it hasn't already.

Variations of the dependency:
  OS==linux && component==shared_library (ash/ash.gyp,
components/components_tests.gyp - which seems wrong, the allocator should never
be linked in the shared library)
  OS==linux (base/base.gyp)
  os_posix == 1 && OS != "mac" && OS != "android" && OS != "ios"
(cc/cc_tests.gyp - and the SAME FILE later on does it for OS=="linux" instead)
  unconditionally (chrome/chrome_android.gypi , chromeos/chromeos.gyp)
  os_posix == 1 && OS != "mac" && OS != "android" (chrome/chrome_exe.gypi)
  os_posix == 1 && OS != "mac" (components/nacl.gyp - ironically nested within
an "OS==linux" condition)
  (OS=="linux" and os_posix==1 and use_aura==1) or OS=="android"
(content/content_app.gypi . Amusing with the os_posix==1 - afraid of a non-POSIX
Linux?)
  OS=="linux" or OS=="android" (content/content_shell.gypi)
  os_posix == 1 and OS != "android" (content/content_tests.gypi)


I cannot help but feel that there is a need for cleanup, and I don't feel
comfortable L.G.T.Ming this without that cleanup or a commitment that the next
CL is going to be that cleanup.

[Dai Mikurube (NOT FULLTIME), 2014-04-25 01:42:59]

+primiano

Hi rsleevi,

Yeah, we're aware of that problem. http://crbug.com/345554 discusses some more
cleanup of this situation -- we finally plan to move and merge these conditions
into allocator.gyp. But before that, we needed to switch into use_allocator.

[Primiano Tucci (use gerrit), 2014-04-25 16:41:28]

On 2014/04/25 00:29:47, Ryan Sleevi wrote:
> I'm not happy with this, but in part it's a pre-existing problem.
> 
> The use of use_allocator is inconsistent throughout the GYP files, and I
cannot
> help but feel it will lead to subtle bugs - if it hasn't already.

Ryan, I completely agree with you. I also pointed out in the past (e.g.,
crrev.com/10824043, "[chromium-dev] Question on allocator") that the current
situation of the allocator in our codebase is just a nightmare. We should have
never ended up in this situation.
There is ton of cruft, as the examples you pointed out, like checking that Linux
is still posix (^__^), or checking twice that OS==linux.

However, my impression is that all the work that Dai is doing:
1. Is not making the situation any worse
2. As you can see from this CL, is reducing the complexity of the
allocator-related gyp lines (which are clearly not Dai's fault), making it
easier in the (hopefully very near) future to clean them up.

In essence, my point is that this CL is not the final step in which we want to
end up, but is a necessary step for doing any clean up. I'd be up for NOT doing
any cleaning up in this CL, as cross-platform gyp changes are always tricky. I
would, instead, prefer to gradually clean up that mess soon after this cl lands
and all the bots are happy. In this sense:

- I agree that nothing except final targets should depend on the allocator (that
shared_library is pretty scary there)
- I would much much prefer that all the targets did just include allocator.gyp
unconditionally, and then all the switching logic was centralized inside the
allocator target.

The latter, however, is going to take some time as, as usual, the conditions
under which non-standard allocators are currently used are non trivial and
converging to the right set of refactoring is going to take a while (and a lot
of reverts).

[Ryan Sleevi, 2014-04-25 18:52:07]

On 2014/04/25 16:41:28, Primiano Tucci wrote:
> On 2014/04/25 00:29:47, Ryan Sleevi wrote:
> > I'm not happy with this, but in part it's a pre-existing problem.
> > 
> > The use of use_allocator is inconsistent throughout the GYP files, and I
> cannot
> > help but feel it will lead to subtle bugs - if it hasn't already.
> 
> Ryan, I completely agree with you. I also pointed out in the past (e.g.,
> crrev.com/10824043, "[chromium-dev] Question on allocator") that the current
> situation of the allocator in our codebase is just a nightmare. We should have
> never ended up in this situation.
> There is ton of cruft, as the examples you pointed out, like checking that
Linux
> is still posix (^__^), or checking twice that OS==linux.
> 
> However, my impression is that all the work that Dai is doing:
> 1. Is not making the situation any worse
> 2. As you can see from this CL, is reducing the complexity of the
> allocator-related gyp lines (which are clearly not Dai's fault), making it
> easier in the (hopefully very near) future to clean them up.
> 

Agreed. My concern here is that is that if the clean-up was prioritized at the
same level, it could have led to a solution that still accomplished the goals
(get everyone on use_allocator) while also leaving the code better. As it
stands, this code just changes the existing mess - which, sure, it may be an
inherited mess. If cleaning up the mess was as important, we might have seen a
different approach.

One such approach would have been replacing all these crazy conditionals with
just a common "includes: [ 'base/allocator/allocator.gypi' ]" for every target:
'executable', and then left all the craziness within that .gypi file. For the
reviewers on this thread, it would have been the same degree of work, it would
have accomplished your "new feature" goal (that is, support for
"use_allocator"), while also leaving the code better than you left it.

That this wasn't pursued is unfortunate, but that ship has sailed. My review
comments are making sure to highlight this, and making sure that there is a
strong sense of ownership attached to this code, and that "cleaning up this
mess" (and the scariness of shared_library) is treated as important as landing
the use_allocator - that is, that this doesn't sit on a shelf for 6 months with
a Pri-3 Type-Cleanup bug. In general, if you see something weird while writing a
new feature, it's better for Chrome overall if you fix the weirdness *first*,
before your new feature ("use_allocator") - as if you don't, the cleanup will
languish because there will *always* be new features.

I'm not convinced the situation is as dire as you suggest - I think it likely
can be done with another patchset of similar scope to this (or could have been
done w/in this patch set), so I want to make sure it's being looked at.

With that "caveat author" rant, LGTM, but cleanup needed. And I will hound y'all
for it :)

[ncarter (slow), 2014-04-25 19:49:05]

sync/ lgtm

[blundell, 2014-04-28 07:44:35]

//components rubberstamp LGTM

[Primiano Tucci (use gerrit), 2014-04-28 10:01:55]

> One such approach would have been replacing all these crazy conditionals with
> just a common "includes: [ 'base/allocator/allocator.gypi' ]" for every
target:
> 'executable', and then left all the craziness within that .gypi file.

I know what you mean. I think this is what I proposed in crrev.com/345554 #7 and
#10.
However if you read the replies there, folks have suggested as that is not
really trivial as it might seem due to the situation on Windows, which
apparently has slightly different requirements / ways to enable the allocator.

I think that all boils down to the fact that most of the folks involved here
have more knowledge on the POSIX side and less clues about the trickery involved
on Windows, that's why this kind of more gradual change seemed more feasible.

The other problem I recall about squashing first all the craziness inside
allocator.gyp was handling multi-projects dependencies. There are at least other
3 projects which are affected by this change that I'm aware of (WebKit/Blink,
build and build_internal) and from what I recall, during the discussions, nobody
could come up with a plan to do handle that gracefully.

I'm not saying this is the best plan ever, but I think this was the better we
got so far.
I appreciate the understanding and share all the reserves about the need of
future cleanup.

[Dai Mikurube (NOT FULLTIME), 2014-04-30 15:37:43]

Thanks for your comments, Ryan and Primiano.

Gentle ping. Could  you take a look?
dgarrett: courgette/
sievers: gpu/
brettw: ppapi/ and url/

[dgarrett, 2014-04-30 16:47:26]

courgette/   LGTM

[no sievers, 2014-04-30 17:40:25]

On 2014/04/30 15:37:43, Dai Mikurube (ooo till May 6) wrote:
> Thanks for your comments, Ryan and Primiano.
> 
> Gentle ping. Could  you take a look?
> dgarrett: courgette/
> sievers: gpu/

gpu lgtm

> brettw: ppapi/ and url/

[Dai Mikurube (NOT FULLTIME), 2014-05-01 15:41:08]

+abarth, yzshen

Could you take a look at ppapi/ and url/ ?  brettw may be busy to look at them.


Thanks, dgarrett and sievers.

[Dai Mikurube (NOT FULLTIME), 2014-05-01 15:42:19]

+abarth, yzshen

Could you take a look at ppapi/ and url/ ?  brettw may be busy to look at them.


Thanks, dgarrett and sievers.

[abarth-chromium, 2014-05-01 17:52:00]

//url LGTM

[yzshen1, 2014-05-01 18:04:33]

ppapi LGTM

[Dai Mikurube (NOT FULLTIME), 2014-05-07 01:54:11]

The CQ bit was checked by dmikurube@chromium.org

[Dai Mikurube (NOT FULLTIME), 2014-05-07 01:54:30]

Thanks, all! Will be committing...

[commit-bot: I haz the power, 2014-05-07 01:57:11]

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

[commit-bot: I haz the power, 2014-05-07 18:46:03]

Change committed as 268876