Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2848113002:
clang: Link lld against tcmalloc. (Closed)
|
Created:
3 years, 7 months ago by pcc1 Modified:
3 years, 5 months ago CC:
chromium-reviews, eugenis+clang_chromium.org, vmpstr+watch_chromium.org, Lei Zhang, dsinclair, yunlian, glider+clang_chromium.org, Nico, ukai+watch_chromium.org, Reid Kleckner, dmikurube+clang_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionclang: Link lld against tcmalloc.
We may want to link the rest of the toolchain against tcmalloc, but for
now this just eliminates a difference between lld and gold, which should
help fix a perf regression when switching to lld.
BUG=607968
R=thakis@chromium.org,krasin@chromium.org
Review-Url: https://codereview.chromium.org/2848113002
Cr-Commit-Position: refs/heads/master@{#471422}
Committed: https://chromium.googlesource.com/chromium/src/+/36be29e9bff22ab7cb6acce2ac66270a805117f2
Patch Set 1 #
Total comments: 2
Patch Set 2 : Copy the .so into the clang package instead of depending on the one in binutils, and fix the test s… #Patch Set 3 : Refresh #
Messages
Total messages: 29 (14 generated)
Oh, and I will upload a package once the issue mentioned in crbug.com/714769 is fixed.
On 2017/04/28 23:20:18, pcc1 wrote: > Oh, and I will upload a package once the issue mentioned in crbug.com/714769 is > fixed. Yes, I suppose we'll have to wait until Monday to try pushing this.
hans@chromium.org changed reviewers: + hans@chromium.org
https://codereview.chromium.org/2848113002/diff/1/tools/clang/scripts/update.py File tools/clang/scripts/update.py (right): https://codereview.chromium.org/2848113002/diff/1/tools/clang/scripts/update.... tools/clang/scripts/update.py:570: '-Wl,-rpath,\\$ORIGIN/../../../binutils/Linux_x64/Release/lib'] Does package.py need an update to include the .so?
https://codereview.chromium.org/2848113002/diff/1/tools/clang/scripts/update.py File tools/clang/scripts/update.py (right): https://codereview.chromium.org/2848113002/diff/1/tools/clang/scripts/update.... tools/clang/scripts/update.py:570: '-Wl,-rpath,\\$ORIGIN/../../../binutils/Linux_x64/Release/lib'] On 2017/04/28 23:23:26, hans wrote: > Does package.py need an update to include the .so? The .so is part of binutils, so wouldn't it come with the binutils package?
On 2017/04/28 23:27:16, pcc1 wrote: > https://codereview.chromium.org/2848113002/diff/1/tools/clang/scripts/update.py > File tools/clang/scripts/update.py (right): > > https://codereview.chromium.org/2848113002/diff/1/tools/clang/scripts/update.... > tools/clang/scripts/update.py:570: > '-Wl,-rpath,\\$ORIGIN/../../../binutils/Linux_x64/Release/lib'] > On 2017/04/28 23:23:26, hans wrote: > > Does package.py need an update to include the .so? > > The .so is part of binutils, so wouldn't it come with the binutils package? Oh, right. I didn't realize that's how it worked. I'm not sure how I feel about this dependency though. There are people relying on update.py to get Clang for non-Chromium things too (e.g. http://llvm.org/docs/LibFuzzer.html#versions). I don't know if any of those rely on lld, but it seems a bit unfortunate if the binaries in the clang package can't "stand on their own" (besides the usual glibc etc.) I don't have any great ideas though. The tcmalloc .so seems to be 1.2 MB (what are they doing in there?) which is not negligeable. But if it gives a decent speed-up, maybe the right thing is to ship it as part of the clang package? Especially if we could benefit from it for clang too.
On 2017/04/28 23:38:23, hans wrote: > On 2017/04/28 23:27:16, pcc1 wrote: > > > https://codereview.chromium.org/2848113002/diff/1/tools/clang/scripts/update.py > > File tools/clang/scripts/update.py (right): > > > > > https://codereview.chromium.org/2848113002/diff/1/tools/clang/scripts/update.... > > tools/clang/scripts/update.py:570: > > '-Wl,-rpath,\\$ORIGIN/../../../binutils/Linux_x64/Release/lib'] > > On 2017/04/28 23:23:26, hans wrote: > > > Does package.py need an update to include the .so? > > > > The .so is part of binutils, so wouldn't it come with the binutils package? > > Oh, right. I didn't realize that's how it worked. > > I'm not sure how I feel about this dependency though. There are people relying > on update.py to get Clang for non-Chromium things too (e.g. > http://llvm.org/docs/LibFuzzer.html#versions). I don't know if any of those rely > on lld, but it seems a bit unfortunate if the binaries in the clang package > can't "stand on their own" (besides the usual glibc etc.) > > I don't have any great ideas though. The tcmalloc .so seems to be 1.2 MB (what > are they doing in there?) which is not negligeable. But if it gives a decent > speed-up, maybe the right thing is to ship it as part of the clang package? > Especially if we could benefit from it for clang too. I think I agree. Once we stop needing the gold plugin for LTO, there will be no need to ship tcmalloc as part of binutils, so we can start building it as part of the clang package. But that change will be simpler to make if we're already shipping it, and it would also unblock depending on it from clang. So for now I'll change this script to copy the .so from the binutils package.
How much of a speedup are we talking here? On Apr 28, 2017 8:00 PM, <pcc@chromium.org> wrote: > On 2017/04/28 23:38:23, hans wrote: > > On 2017/04/28 23:27:16, pcc1 wrote: > > > > > > https://codereview.chromium.org/2848113002/diff/1/tools/ > clang/scripts/update.py > > > File tools/clang/scripts/update.py (right): > > > > > > > > > https://codereview.chromium.org/2848113002/diff/1/tools/ > clang/scripts/update.py#newcode570 > > > tools/clang/scripts/update.py:570: > > > '-Wl,-rpath,\\$ORIGIN/../../../binutils/Linux_x64/Release/lib'] > > > On 2017/04/28 23:23:26, hans wrote: > > > > Does package.py need an update to include the .so? > > > > > > The .so is part of binutils, so wouldn't it come with the binutils > package? > > > > Oh, right. I didn't realize that's how it worked. > > > > I'm not sure how I feel about this dependency though. There are people > relying > > on update.py to get Clang for non-Chromium things too (e.g. > > http://llvm.org/docs/LibFuzzer.html#versions). I don't know if any of > those > rely > > on lld, but it seems a bit unfortunate if the binaries in the clang > package > > can't "stand on their own" (besides the usual glibc etc.) > > > > I don't have any great ideas though. The tcmalloc .so seems to be 1.2 MB > (what > > are they doing in there?) which is not negligeable. But if it gives a > decent > > speed-up, maybe the right thing is to ship it as part of the clang > package? > > Especially if we could benefit from it for clang too. > > I think I agree. Once we stop needing the gold plugin for LTO, there will > be no > need to ship tcmalloc as part of binutils, so we can start building it as > part > of the clang package. But that change will be simpler to make if we're > already > shipping it, and it would also unblock depending on it from clang. > > So for now I'll change this script to copy the .so from the binutils > package. > > https://codereview.chromium.org/2848113002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I measured about 10% (median of 5 runs) linking base_unittests. Haven't measured any larger targets, but I'd expect it to be in the same ballpark. Peter On Fri, Apr 28, 2017 at 5:49 PM, Nico Weber <thakis@chromium.org> wrote: > How much of a speedup are we talking here? > > On Apr 28, 2017 8:00 PM, <pcc@chromium.org> wrote: > >> On 2017/04/28 23:38:23, hans wrote: >> > On 2017/04/28 23:27:16, pcc1 wrote: >> > > >> > >> https://codereview.chromium.org/2848113002/diff/1/tools/clan >> g/scripts/update.py >> > > File tools/clang/scripts/update.py (right): >> > > >> > > >> > >> https://codereview.chromium.org/2848113002/diff/1/tools/clan >> g/scripts/update.py#newcode570 >> > > tools/clang/scripts/update.py:570: >> > > '-Wl,-rpath,\\$ORIGIN/../../../binutils/Linux_x64/Release/lib'] >> > > On 2017/04/28 23:23:26, hans wrote: >> > > > Does package.py need an update to include the .so? >> > > >> > > The .so is part of binutils, so wouldn't it come with the binutils >> package? >> > >> > Oh, right. I didn't realize that's how it worked. >> > >> > I'm not sure how I feel about this dependency though. There are people >> relying >> > on update.py to get Clang for non-Chromium things too (e.g. >> > http://llvm.org/docs/LibFuzzer.html#versions). I don't know if any of >> those >> rely >> > on lld, but it seems a bit unfortunate if the binaries in the clang >> package >> > can't "stand on their own" (besides the usual glibc etc.) >> > >> > I don't have any great ideas though. The tcmalloc .so seems to be 1.2 >> MB (what >> > are they doing in there?) which is not negligeable. But if it gives a >> decent >> > speed-up, maybe the right thing is to ship it as part of the clang >> package? >> > Especially if we could benefit from it for clang too. >> >> I think I agree. Once we stop needing the gold plugin for LTO, there will >> be no >> need to ship tcmalloc as part of binutils, so we can start building it as >> part >> of the clang package. But that change will be simpler to make if we're >> already >> shipping it, and it would also unblock depending on it from clang. >> >> So for now I'll change this script to copy the .so from the binutils >> package. >> >> https://codereview.chromium.org/2848113002/ >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ah it was in a comment, sorry, I had only looked at the cl description. That's kind of close to the "is this worth the complexity" bar imho :-/ On Apr 28, 2017 8:56 PM, "Peter Collingbourne" <pcc@google.com> wrote: I measured about 10% (median of 5 runs) linking base_unittests. Haven't measured any larger targets, but I'd expect it to be in the same ballpark. Peter On Fri, Apr 28, 2017 at 5:49 PM, Nico Weber <thakis@chromium.org> wrote: > How much of a speedup are we talking here? > > On Apr 28, 2017 8:00 PM, <pcc@chromium.org> wrote: > >> On 2017/04/28 23:38:23, hans wrote: >> > On 2017/04/28 23:27:16, pcc1 wrote: >> > > >> > >> https://codereview.chromium.org/2848113002/diff/1/tools/clan >> g/scripts/update.py >> > > File tools/clang/scripts/update.py (right): >> > > >> > > >> > >> https://codereview.chromium.org/2848113002/diff/1/tools/clan >> g/scripts/update.py#newcode570 >> > > tools/clang/scripts/update.py:570: >> > > '-Wl,-rpath,\\$ORIGIN/../../../binutils/Linux_x64/Release/lib'] >> > > On 2017/04/28 23:23:26, hans wrote: >> > > > Does package.py need an update to include the .so? >> > > >> > > The .so is part of binutils, so wouldn't it come with the binutils >> package? >> > >> > Oh, right. I didn't realize that's how it worked. >> > >> > I'm not sure how I feel about this dependency though. There are people >> relying >> > on update.py to get Clang for non-Chromium things too (e.g. >> > http://llvm.org/docs/LibFuzzer.html#versions). I don't know if any of >> those >> rely >> > on lld, but it seems a bit unfortunate if the binaries in the clang >> package >> > can't "stand on their own" (besides the usual glibc etc.) >> > >> > I don't have any great ideas though. The tcmalloc .so seems to be 1.2 >> MB (what >> > are they doing in there?) which is not negligeable. But if it gives a >> decent >> > speed-up, maybe the right thing is to ship it as part of the clang >> package? >> > Especially if we could benefit from it for clang too. >> >> I think I agree. Once we stop needing the gold plugin for LTO, there will >> be no >> need to ship tcmalloc as part of binutils, so we can start building it as >> part >> of the clang package. But that change will be simpler to make if we're >> already >> shipping it, and it would also unblock depending on it from clang. >> >> So for now I'll change this script to copy the .so from the binutils >> package. >> >> https://codereview.chromium.org/2848113002/ >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I think it's worth it for full LTO. Arguably less so for ThinLTO, but as we recently discovered it seems likely that ThinLTO won't be ready for a few weeks. I'd like to continue moving to lld+ThinLTO by changing one variable at a time. The first variable I'd like to change is the linker, but not the allocator. Happy to re-evaluate once ThinLTO launches though. On 2017/04/29 01:33:30, Nico wrote: > Ah it was in a comment, sorry, I had only looked at the cl description. > > That's kind of close to the "is this worth the complexity" bar imho :-/ > > On Apr 28, 2017 8:56 PM, "Peter Collingbourne" <mailto:pcc@google.com> wrote: > > I measured about 10% (median of 5 runs) linking base_unittests. Haven't > measured any larger targets, but I'd expect it to be in the same ballpark. > > Peter > > On Fri, Apr 28, 2017 at 5:49 PM, Nico Weber <mailto:thakis@chromium.org> wrote: > > > How much of a speedup are we talking here? > > > > On Apr 28, 2017 8:00 PM, <mailto:pcc@chromium.org> wrote: > > > >> On 2017/04/28 23:38:23, hans wrote: > >> > On 2017/04/28 23:27:16, pcc1 wrote: > >> > > > >> > > >> https://codereview.chromium.org/2848113002/diff/1/tools/clan > >> g/scripts/update.py > >> > > File tools/clang/scripts/update.py (right): > >> > > > >> > > > >> > > >> https://codereview.chromium.org/2848113002/diff/1/tools/clan > >> g/scripts/update.py#newcode570 > >> > > tools/clang/scripts/update.py:570: > >> > > '-Wl,-rpath,\\$ORIGIN/../../../binutils/Linux_x64/Release/lib'] > >> > > On 2017/04/28 23:23:26, hans wrote: > >> > > > Does package.py need an update to include the .so? > >> > > > >> > > The .so is part of binutils, so wouldn't it come with the binutils > >> package? > >> > > >> > Oh, right. I didn't realize that's how it worked. > >> > > >> > I'm not sure how I feel about this dependency though. There are people > >> relying > >> > on update.py to get Clang for non-Chromium things too (e.g. > >> > http://llvm.org/docs/LibFuzzer.html#versions). I don't know if any of > >> those > >> rely > >> > on lld, but it seems a bit unfortunate if the binaries in the clang > >> package > >> > can't "stand on their own" (besides the usual glibc etc.) > >> > > >> > I don't have any great ideas though. The tcmalloc .so seems to be 1.2 > >> MB (what > >> > are they doing in there?) which is not negligeable. But if it gives a > >> decent > >> > speed-up, maybe the right thing is to ship it as part of the clang > >> package? > >> > Especially if we could benefit from it for clang too. > >> > >> I think I agree. Once we stop needing the gold plugin for LTO, there will > >> be no > >> need to ship tcmalloc as part of binutils, so we can start building it as > >> part > >> of the clang package. But that change will be simpler to make if we're > >> already > >> shipping it, and it would also unblock depending on it from clang. > >> > >> So for now I'll change this script to copy the .so from the binutils > >> package. > >> > >> https://codereview.chromium.org/2848113002/ > >> > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by pcc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) 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...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pcc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by pcc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The two red bots are due to crbug.com/721492 which is unrelated. lgtm
The CQ bit was checked by pcc@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494610204131030,
"parent_rev": "fcf91a8eaacdeaedb0de1eba39ff4c24fab34323", "commit_rev":
"36be29e9bff22ab7cb6acce2ac66270a805117f2"}
Message was sent while issue was closed.
Description was changed from ========== clang: Link lld against tcmalloc. We may want to link the rest of the toolchain against tcmalloc, but for now this just eliminates a difference between lld and gold, which should help fix a perf regression when switching to lld. BUG=607968 R=thakis@chromium.org,krasin@chromium.org ========== to ========== clang: Link lld against tcmalloc. We may want to link the rest of the toolchain against tcmalloc, but for now this just eliminates a difference between lld and gold, which should help fix a perf regression when switching to lld. BUG=607968 R=thakis@chromium.org,krasin@chromium.org Review-Url: https://codereview.chromium.org/2848113002 Cr-Commit-Position: refs/heads/master@{#471422} Committed: https://chromium.googlesource.com/chromium/src/+/36be29e9bff22ab7cb6acce2ac66... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/36be29e9bff22ab7cb6acce2ac66...
Message was sent while issue was closed.
On 2017/04/29 02:14:20, pcc1 wrote: > I think it's worth it for full LTO. Arguably less so for ThinLTO, but as we > recently discovered it seems likely that ThinLTO won't be ready for a few weeks. > > I'd like to continue moving to lld+ThinLTO by changing one variable at a time. > The first variable I'd like to change is the linker, but not the allocator. > Happy to re-evaluate once ThinLTO launches though. I measured this again with ThinLTO. With tcmalloc (median of 6): 42.29s With glibc malloc: 44.61s So it looks like the cost is closer to 5% now. I think I'd be happy with glibc malloc for now, and re-evaluating for the toolchain as a whole at a later time. I'm working on a CL that will stop building the gold plugin, and at the same time I'll switch lld back to glibc malloc. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
