Fold most logic from //base/allocator into //base.

Fold //base/allocator:unified_allocator_shim into //base:base.

This CL is a refactor and has no intended behavior change.

There is no reason to have a separate source set for
//base/allocator:unified_allocator_shim, which introduces dependency cycles.

BUG=665567
Review-Url: https://codereview.chromium.org/2651043008
Cr-Commit-Position: refs/heads/master@{#446800}
Committed: https://chromium.googlesource.com/chromium/src/+/83d8f196d15bc31bfbce50ecc435016ba2a96bcf

[erikchen, 2017-01-25 23:45:26]

Description was changed from

==========
Move most configs/source_sets from //base/allocator into //base.

There was no need to have a separate soure set, and it was causing dependency
issues, since it's very hard for Chrome code to not depend on //base.

BUG=665567
==========

to

==========
Move most configs/source_sets from //base/allocator into //base.

There was no need to have separate source sets, and it was causing dependency
issues, since it's very hard for Chrome code to not depend on //base.

BUG=665567
==========

[erikchen, 2017-01-26 00:03:49]

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

[commit-bot: I haz the power, 2017-01-26 00:04:27]

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

[erikchen, 2017-01-26 00:06:43]

Description was changed from

==========
Move most configs/source_sets from //base/allocator into //base.

There was no need to have separate source sets, and it was causing dependency
issues, since it's very hard for Chrome code to not depend on //base.

BUG=665567
==========

to

==========
Fold most logic from //base/allocator into //base.

This CL is a refactor and has no intended behavior change beyond elimination of
the source set //base/allocator:unified_allocator_shim and folding the sources
directly into //base:base.

//base/allocator was defining a source set included by base, which causes cyclic
dependency issues. The only library I left in //base/allocator was tcmalloc,
which is actually independent from //base.

BUG=665567
==========

[erikchen, 2017-01-26 00:06:53]

Description was changed from

==========
Fold most logic from //base/allocator into //base.

This CL is a refactor and has no intended behavior change beyond elimination of
the source set //base/allocator:unified_allocator_shim and folding the sources
directly into //base:base.

//base/allocator was defining a source set included by base, which causes cyclic
dependency issues. The only library I left in //base/allocator was tcmalloc,
which is actually independent from //base.

BUG=665567
==========

to

==========
Fold most logic from //base/allocator into //base.

This CL is a refactor and has no intended behavior change beyond elimination of
the source set //base/allocator:unified_allocator_shim and folding the sources
directly into //base:base.

//base/allocator was defining a source set included by base, which causes cyclic
dependency issues. The only library I left in //base/allocator was tcmalloc,
which is actually independent from //base.

BUG=665567
==========

[commit-bot: I haz the power, 2017-01-26 00:09:39]

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

[commit-bot: I haz the power, 2017-01-26 00:09:40]

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

[erikchen, 2017-01-26 00:36:06]

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

[commit-bot: I haz the power, 2017-01-26 00:37:24]

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-26 00:45:12]

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

[commit-bot: I haz the power, 2017-01-26 00:45:12]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[erikchen, 2017-01-26 00:53:45]

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

[commit-bot: I haz the power, 2017-01-26 00:55:10]

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

[erikchen, 2017-01-26 01:49:57]

erikchen@chromium.org changed reviewers:
+ mark@chromium.org, primiano@chromium.org

[erikchen, 2017-01-26 01:49:58]

primiano, mark: Please review.

[Primiano Tucci (use gerrit), 2017-01-26 02:03:44]

https://codereview.chromium.org/2651043008/diff/80001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2651043008/diff/80001/base/BUILD.gn#newcode125
base/BUILD.gn:125: # The Windows-only allocator shim is only enabled for Release
static builds, and
Hmm wouldn't it make sense that this allocator-related stuff still lived in
//base/allocator/BUILD.gn?
correct me if I am wrong but the  actual problem you found here is the
allocator_shim being a source_set, which would cause cylic dependencies.
All these configs and groups could still live in /allocator and be reffered by
base (as it is already the case today, if I read the code correctly).
I think the only thing that you might need to move is the actual sources +=
assignments.

P.S. all this provided that the pattern of 
target(A) depending source_set(B) and B having references to things in A
(without explicitly depending on it) is undesirable.
Should we check with gn-dev whether this is actually a problem or we have other
instances of this? Which would make all this a non-problem

[erikchen, 2017-01-26 02:09:12]

https://codereview.chromium.org/2651043008/diff/80001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2651043008/diff/80001/base/BUILD.gn#newcode125
base/BUILD.gn:125: # The Windows-only allocator shim is only enabled for Release
static builds, and
On 2017/01/26 02:03:44, Primiano Tucci wrote:
> Hmm wouldn't it make sense that this allocator-related stuff still lived in
> //base/allocator/BUILD.gn?
> correct me if I am wrong but the  actual problem you found here is the
> allocator_shim being a source_set, which would cause cylic dependencies.
> All these configs and groups could still live in /allocator and be reffered by
> base (as it is already the case today, if I read the code correctly).
> I think the only thing that you might need to move is the actual sources +=
> assignments.
win_use_allocator_shim is used by both this logic and the actual sources logic.
We could replicate the logic to create win_use_allocator_shim, or pull it into a
gni file, but that seems like overkill.

> 
> P.S. all this provided that the pattern of 
> target(A) depending source_set(B) and B having references to things in A
> (without explicitly depending on it) is undesirable.
> Should we check with gn-dev whether this is actually a problem or we have
other
> instances of this? Which would make all this a non-problem
The documentation in base says that base should not depend on source_sets. So
let's say we turn it into a static library. Now we have references to undefined
symbols.

[commit-bot: I haz the power, 2017-01-26 04:18:13]

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

[commit-bot: I haz the power, 2017-01-26 04:18:14]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[erikchen, 2017-01-26 06:30:12]

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

[commit-bot: I haz the power, 2017-01-26 06:30:34]

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

[erikchen, 2017-01-26 06:31:13]

On 2017/01/26 02:09:12, erikchen wrote:
> https://codereview.chromium.org/2651043008/diff/80001/base/BUILD.gn
> File base/BUILD.gn (right):
> 
> https://codereview.chromium.org/2651043008/diff/80001/base/BUILD.gn#newcode125
> base/BUILD.gn:125: # The Windows-only allocator shim is only enabled for
Release
> static builds, and
> On 2017/01/26 02:03:44, Primiano Tucci wrote:
> > Hmm wouldn't it make sense that this allocator-related stuff still lived in
> > //base/allocator/BUILD.gn?
> > correct me if I am wrong but the  actual problem you found here is the
> > allocator_shim being a source_set, which would cause cylic dependencies.
> > All these configs and groups could still live in /allocator and be reffered
by
> > base (as it is already the case today, if I read the code correctly).
> > I think the only thing that you might need to move is the actual sources +=
> > assignments.
> win_use_allocator_shim is used by both this logic and the actual sources
logic.
> We could replicate the logic to create win_use_allocator_shim, or pull it into
a
> gni file, but that seems like overkill.
> 
> > 
> > P.S. all this provided that the pattern of 
> > target(A) depending source_set(B) and B having references to things in A
> > (without explicitly depending on it) is undesirable.
> > Should we check with gn-dev whether this is actually a problem or we have
> other
> > instances of this? Which would make all this a non-problem
> The documentation in base says that base should not depend on source_sets. So
> let's say we turn it into a static library. Now we have references to
undefined
> symbols.

See thread primiano@ started on gn-dev for more information:
https://groups.google.com/a/chromium.org/forum/#!topic/gn-dev/W-YFaAhU73M

I really see no reason to have circular dependencies when they can be avoided.

[commit-bot: I haz the power, 2017-01-26 08:16:50]

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

[commit-bot: I haz the power, 2017-01-26 08:16:51]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[erikchen, 2017-01-26 18:31:17]

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

[commit-bot: I haz the power, 2017-01-26 18:32:04]

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

[Primiano Tucci (use gerrit), 2017-01-26 18:47:09]

https://codereview.chromium.org/2651043008/diff/80001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2651043008/diff/80001/base/BUILD.gn#newcode125
base/BUILD.gn:125: # The Windows-only allocator shim is only enabled for Release
static builds, and
On 2017/01/26 02:09:12, erikchen wrote:
> On 2017/01/26 02:03:44, Primiano Tucci wrote:
> > Hmm wouldn't it make sense that this allocator-related stuff still lived in
> > //base/allocator/BUILD.gn?
> > correct me if I am wrong but the  actual problem you found here is the
> > allocator_shim being a source_set, which would cause cylic dependencies.
> > All these configs and groups could still live in /allocator and be reffered
by
> > base (as it is already the case today, if I read the code correctly).
> > I think the only thing that you might need to move is the actual sources +=
> > assignments.
> win_use_allocator_shim is used by both this logic and the actual sources
logic.
> We could replicate the logic to create win_use_allocator_shim, or pull it into
a
> gni file, but that seems like overkill.
> 
> > 
> > P.S. all this provided that the pattern of 
> > target(A) depending source_set(B) and B having references to things in A
> > (without explicitly depending on it) is undesirable.
> > Should we check with gn-dev whether this is actually a problem or we have
> other
> > instances of this? Which would make all this a non-problem
> The documentation in base says that base should not depend on source_sets. So
> let's say we turn it into a static library. Now we have references to
undefined
> symbols.

What I am saying is that all the windows logic (and I think also the android
below) can stay (and IMHO shoud stay) in base/allocator/BUILD.gn
This is my line of reasoning, tell me if I am missing something:

The following targets depend on win_use_allocator_shim:
 1. source_set("base_allocator_shim")
 2. config(base_allocator_shim_define) 
 3. group(base_allocator)

1. is a pure leaf target and does not have any dependencies back to base. So
could live in allocator/BUILD.gn
2. is a pure config target, could live in allocator/BUILD.gn
3. depends in turn on :
   tcmalloc: which is a pure leaf target and can live (in fact you are correctly
leaving it there) in allocator/
   base_allocator_shim: see point 1 above.

So, if I am reasoning correctly all this hunk (lines 125 - 191) can stay in
allocator/BUILD.gn without requiring any gni or any logic duplication.

[erikchen, 2017-01-26 19:20:15]

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

[commit-bot: I haz the power, 2017-01-26 19:20:53]

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-26 20:16:31]

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

[commit-bot: I haz the power, 2017-01-26 20:16:32]

Dry run: This issue passed the CQ dry run.

[erikchen, 2017-01-26 20:51:35]

Description was changed from

==========
Fold most logic from //base/allocator into //base.

This CL is a refactor and has no intended behavior change beyond elimination of
the source set //base/allocator:unified_allocator_shim and folding the sources
directly into //base:base.

//base/allocator was defining a source set included by base, which causes cyclic
dependency issues. The only library I left in //base/allocator was tcmalloc,
which is actually independent from //base.

BUG=665567
==========

to

==========
Fold //base/allocator:unified_allocator_shim into //base:base.

This CL is a refactor and has no intended behavior change.

There is no reason to have a separate source set for
//base/allocator:unified_allocator_shim, which introduces dependency cycles.

BUG=665567
==========

[erikchen, 2017-01-26 20:51:52]

https://codereview.chromium.org/2651043008/diff/80001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2651043008/diff/80001/base/BUILD.gn#newcode125
base/BUILD.gn:125: # The Windows-only allocator shim is only enabled for Release
static builds, and
On 2017/01/26 18:47:09, Primiano Tucci wrote:
> On 2017/01/26 02:09:12, erikchen wrote:
> > On 2017/01/26 02:03:44, Primiano Tucci wrote:
> > > Hmm wouldn't it make sense that this allocator-related stuff still lived
in
> > > //base/allocator/BUILD.gn?
> > > correct me if I am wrong but the  actual problem you found here is the
> > > allocator_shim being a source_set, which would cause cylic dependencies.
> > > All these configs and groups could still live in /allocator and be
reffered
> by
> > > base (as it is already the case today, if I read the code correctly).
> > > I think the only thing that you might need to move is the actual sources
+=
> > > assignments.
> > win_use_allocator_shim is used by both this logic and the actual sources
> logic.
> > We could replicate the logic to create win_use_allocator_shim, or pull it
into
> a
> > gni file, but that seems like overkill.
> > 
> > > 
> > > P.S. all this provided that the pattern of 
> > > target(A) depending source_set(B) and B having references to things in A
> > > (without explicitly depending on it) is undesirable.
> > > Should we check with gn-dev whether this is actually a problem or we have
> > other
> > > instances of this? Which would make all this a non-problem
> > The documentation in base says that base should not depend on source_sets.
So
> > let's say we turn it into a static library. Now we have references to
> undefined
> > symbols.
> 
> What I am saying is that all the windows logic (and I think also the android
> below) can stay (and IMHO shoud stay) in base/allocator/BUILD.gn
> This is my line of reasoning, tell me if I am missing something:
> 
> The following targets depend on win_use_allocator_shim:
>  1. source_set("base_allocator_shim")
>  2. config(base_allocator_shim_define) 
>  3. group(base_allocator)
> 
> 1. is a pure leaf target and does not have any dependencies back to base. So
> could live in allocator/BUILD.gn
> 2. is a pure config target, could live in allocator/BUILD.gn
> 3. depends in turn on :
>    tcmalloc: which is a pure leaf target and can live (in fact you are
correctly
> leaving it there) in allocator/
>    base_allocator_shim: see point 1 above.
> 
> So, if I am reasoning correctly all this hunk (lines 125 - 191) can stay in
> allocator/BUILD.gn without requiring any gni or any logic duplication.

Done.

[This was the first thing I tried, but for some reason I thought
win_use_allocator_shim would have to be duplicated.]

[Mark Mentovai, 2017-01-26 23:25:52]

LGTM

[Primiano Tucci (use gerrit), 2017-01-27 19:28:38]

LGTM thanks

[erikchen, 2017-01-27 19:30:18]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2017-01-27 19:31:14]

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-27 22:37:58]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485545418799640,
"parent_rev": "dbfc3b670a3437eb85257c704d69b7fbf733620f", "commit_rev":
"83d8f196d15bc31bfbce50ecc435016ba2a96bcf"}

[commit-bot: I haz the power, 2017-01-27 22:38:27]

Description was changed from

==========
Fold //base/allocator:unified_allocator_shim into //base:base.

This CL is a refactor and has no intended behavior change.

There is no reason to have a separate source set for
//base/allocator:unified_allocator_shim, which introduces dependency cycles.

BUG=665567
==========

to

==========
Fold //base/allocator:unified_allocator_shim into //base:base.

This CL is a refactor and has no intended behavior change.

There is no reason to have a separate source set for
//base/allocator:unified_allocator_shim, which introduces dependency cycles.

BUG=665567

Review-Url: https://codereview.chromium.org/2651043008
Cr-Commit-Position: refs/heads/master@{#446800}
Committed:
https://chromium.googlesource.com/chromium/src/+/83d8f196d15bc31bfbce50ecc435...
==========

[commit-bot: I haz the power, 2017-01-27 22:38:28]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/83d8f196d15bc31bfbce50ecc435...