Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1016)

Issue 1584893002: Allow base to depend on allocator (Closed)

Created:
4 years, 11 months ago by Primiano Tucci (use gerrit)
Modified:
4 years, 11 months ago
Reviewers:
Will Harris, Nico, brettw
CC:
chromium-reviews, wfh+watch_chromium.org, Dai Mikurube (NOT FULLTIME), vmpstr+watch_chromium.org, Ruud van Asseldonk
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Allow base to depend on allocator A smaller, yet key, step to move From: a situation where mainly executables (but not really) depend on allocator, and base needs dependencies (to tcmalloc) to be injected from content (which violates the ODR in component buids). To: a situation where only base depends on allocator and the other targets get recursively the required linked flags. In essence this CL is a more gradual approach to the bigger unreviewable crrev.com/1528013002. How is the transition handled? ------------------------------ After this CL, the situation will be as follows: From a build time perspective base will also depend on allocator. This will not change anything substantial in static builds and introduce yet another (temporary) ODR violation in Linux component builds. The big change introduced by this CL is the fact that all the executable targets that depend on base (virtualy all) will also get another indirect dependency to allocator. In other words, after this CL executable targets will depend on allocator for two reasons: - Because they have an explicit dependency to it (the one I am going to get rid of in the immediate future). - Because this new transitive path I am introducing in base. Rationale of this approach -------------------------- This allows to restrict the critical changes in a smaller CL easier to review, at the cost of the temporary double dependency on base. The good things are: - If something will break, this CL will be very easy to revert. - The next cleanups will be straightforward. - We have now smoke tests (crrev.com/1577883002) that will help us realize if something goes wrong. Next steps ---------- In the next CLs I will: - Remove the content -> base injection layer, and let base directly use the tcmalloc functions it needs. - Remove all the traces of USE_TCMALLOC outside of base. - Start cleaning up the hundreds use_allocator conditionals in the gyp files in a way which is easier to review and produce zero ninja diffs (see crrev.com/1583973002 as an example) Ninja diffs caused by this change --------------------------------- ### Win, static build, GN: https://paste.ee/p/hvcRp The missing targets (mostly tests) that previously were not depending on allocator, now get that by virtue of the transitive dependency. ### Win, static build, GYP: https://paste.ee/p/AGuKR As above. Just GYP seems to emit the ninja files in a different, inlined, format. ### linux static build, GYP: https://paste.ee/p/kmD7U As above. Plus the new targets also get the -Wl,-u (keep symbol) args as expected by allocator.gyp for the tcmalloc heap profiler. ### linux shared build, GYP: https://paste.ee/p/FHHNR Nothing relevant. Just I moved the dependency to allocator from base_unittests to base, and that is the only thing that reflects in the ninja files. BUG=564618 Committed: https://crrev.com/58e5f8b23a68e6a87d89e87c6d9e6bef2c265ecd Cr-Commit-Position: refs/heads/master@{#370405}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Pure rebase, no changes #

Patch Set 3 : Move is_nacl condition to allocator group #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+30 lines, -46 lines) Patch
M base/BUILD.gn View 1 2 3 chunks +4 lines, -1 line 0 comments Download
M base/allocator/BUILD.gn View 1 2 2 chunks +12 lines, -12 lines 0 comments Download
M base/allocator/allocator.gyp View 4 chunks +12 lines, -15 lines 0 comments Download
M base/base.gyp View 1 3 chunks +2 lines, -18 lines 2 comments Download

Dependent Patchsets:

Messages

Total messages: 23 (10 generated)
Primiano Tucci (use gerrit)
wfh/thakis another bit of patience. If this flies, the remaining bits will be just boring ...
4 years, 11 months ago (2016-01-13 20:02:37 UTC) #4
Will Harris
On 2016/01/13 20:02:37, Primiano Tucci wrote: > wfh/thakis another bit of patience. > If this ...
4 years, 11 months ago (2016-01-19 18:07:41 UTC) #6
Primiano Tucci (use gerrit)
+brettw for the GN changes.
4 years, 11 months ago (2016-01-19 18:13:22 UTC) #8
brettw
lgtm https://codereview.chromium.org/1584893002/diff/1/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1584893002/diff/1/base/BUILD.gn#newcode960 base/BUILD.gn:960: if (!is_nacl) { I think the allocator should ...
4 years, 11 months ago (2016-01-19 20:20:33 UTC) #9
Primiano Tucci (use gerrit)
https://codereview.chromium.org/1584893002/diff/1/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1584893002/diff/1/base/BUILD.gn#newcode960 base/BUILD.gn:960: if (!is_nacl) { On 2016/01/19 20:20:33, brettw wrote: > ...
4 years, 11 months ago (2016-01-20 14:11:43 UTC) #11
Nico
lgtm https://codereview.chromium.org/1584893002/diff/60001/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/1584893002/diff/60001/base/base.gyp#newcode24 base/base.gyp:24: 'allocator/allocator.gyp:allocator', allocator is now on some platforms a ...
4 years, 11 months ago (2016-01-20 14:47:39 UTC) #12
Primiano Tucci (use gerrit)
https://codereview.chromium.org/1584893002/diff/60001/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/1584893002/diff/60001/base/base.gyp#newcode24 base/base.gyp:24: 'allocator/allocator.gyp:allocator', On 2016/01/20 14:47:39, Nico wrote: > allocator is ...
4 years, 11 months ago (2016-01-20 14:50:30 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1584893002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1584893002/60001
4 years, 11 months ago (2016-01-20 14:50:57 UTC) #16
commit-bot: I haz the power
Committed patchset #3 (id:60001)
4 years, 11 months ago (2016-01-20 15:58:39 UTC) #18
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/58e5f8b23a68e6a87d89e87c6d9e6bef2c265ecd Cr-Commit-Position: refs/heads/master@{#370405}
4 years, 11 months ago (2016-01-20 15:59:18 UTC) #20
robert.bradford
A revert of this CL (patchset #3 id:60001) has been created in https://codereview.chromium.org/1610673002/ by robert.bradford@intel.com. ...
4 years, 11 months ago (2016-01-20 16:31:44 UTC) #21
Primiano Tucci (use gerrit)
That was quick! Nico: I guess that the reason of the failure >clang: error: no ...
4 years, 11 months ago (2016-01-20 16:33:43 UTC) #22
Primiano Tucci (use gerrit)
4 years, 11 months ago (2016-01-20 18:16:42 UTC) #23
Message was sent while issue was closed.
For the records, this is relanding in :

https://codereview.chromium.org/1605023004

Powered by Google App Engine
This is Rietveld 408576698