Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2849583005:
Disable linker map for Clang toolchains. (Closed)
DescriptionDisable linker map for Clang toolchains.
Right now, we are transitioning to LLD, and LLD has a different
map format (so, the analysis tool will need to be updated), as well as
suboptimal implementation that adds 60 minutes to the Chrome link time.
This will be reenabled when both of the blockers above are resolved.
BUG=716209, 607968
Review-Url: https://codereview.chromium.org/2849583005
Cr-Commit-Position: refs/heads/master@{#468113}
Committed: https://chromium.googlesource.com/chromium/src/+/317d9edab7ba87a5c05564ca8c66a12911c46872
Patch Set 1 #Patch Set 2 : !use_lld #Messages
Total messages: 36 (15 generated)
The CQ bit was checked by krasin@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...
krasin@chromium.org changed reviewers: + agrieve@chromium.org
Hi Andrew, can you please take a look? I am not sure it's the right way to disable your feature; please, suggest a better one, if you think the current way will affect Android (I honestly don't know)
On 2017/04/28 16:21:16, krasin1 wrote: > Hi Andrew, > > can you please take a look? I am not sure it's the right way to disable your > feature; please, suggest a better one, if you think the current way will affect > Android (I honestly don't know) yep, this is fine. lgtm.
krasin@chromium.org changed reviewers: + dpranke@google.com
Hi Dirk, I need your OWNERS approval for this tiny change. :) This is to unlock https://codereview.chromium.org/2842683002/ that was delayed by this unexpected linker slowdown. That happens because LLD uses an O(n^2) algorithm for generating maps; that will soon be fixed, disabling maps in the mean time.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Why is this related to clang? Shouldn't https://cs.chromium.org/chromium/src/build/toolchain/gcc_toolchain.gni?type=c... just have an `&& !use_lld` at the end?
On 2017/04/28 18:03:32, Nico wrote: > Why is this related to clang? Shouldn't > https://cs.chromium.org/chromium/src/build/toolchain/gcc_toolchain.gni?type=c... > just have an `&& !use_lld` at the end? Because: 1) I want to move us to LLD everywhere where we use Gold now 2) LLD has a different format of output file. If we allow both formats, that will need to be supported in the analysis tool. 3) I want to enable this option again, as the work Andrew is doing (size analysis based on these maps) is an important one.
On Fri, Apr 28, 2017 at 2:08 PM, <krasin@chromium.org> wrote: > On 2017/04/28 18:03:32, Nico wrote: > > Why is this related to clang? Shouldn't > > > https://cs.chromium.org/chromium/src/build/toolchain/ > gcc_toolchain.gni?type=cs&q=enable_linker_map+package:%5Echromium$&l=251 > > just have an `&& !use_lld` at the end? > > Because: > > 1) I want to move us to LLD everywhere where we use Gold now > 2) LLD has a different format of output file. If we allow both formats, > that > will need to be supported in the analysis tool. > 3) I want to enable this option again, as the work Andrew is doing (size > analysis based on these maps) is an important one. > Sure, but the interesting bit is use_lld, not is_clang, no? -- 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.
On Fri, Apr 28, 2017 at 2:08 PM, Nico Weber <thakis@chromium.org> wrote: > On Fri, Apr 28, 2017 at 2:08 PM, <krasin@chromium.org> wrote: > >> On 2017/04/28 18:03:32, Nico wrote: >> > Why is this related to clang? Shouldn't >> > >> https://cs.chromium.org/chromium/src/build/toolchain/gcc_ >> toolchain.gni?type=cs&q=enable_linker_map+package:%5Echromium$&l=251 >> > just have an `&& !use_lld` at the end? >> >> Because: >> >> 1) I want to move us to LLD everywhere where we use Gold now >> 2) LLD has a different format of output file. If we allow both formats, >> that >> will need to be supported in the analysis tool. >> 3) I want to enable this option again, as the work Andrew is doing (size >> analysis based on these maps) is an important one. >> > > Sure, but the interesting bit is use_lld, not is_clang, no? > For example, if we were to switch the android bots to clang (+ gold) then we'd want to keep the map file. -- 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.
Why don't to switch Android bots to Clang+LLD? I mean, I would rather have enable_linker_map = use_lld, after the next Clang roll, so we only compute maps if LLD is used.
On 2017/04/28 18:09:21, Nico wrote: > On Fri, Apr 28, 2017 at 2:08 PM, Nico Weber <mailto:thakis@chromium.org> wrote: > > > On Fri, Apr 28, 2017 at 2:08 PM, <mailto:krasin@chromium.org> wrote: > > > >> On 2017/04/28 18:03:32, Nico wrote: > >> > Why is this related to clang? Shouldn't > >> > > >> https://cs.chromium.org/chromium/src/build/toolchain/gcc_ > >> toolchain.gni?type=cs&q=enable_linker_map+package:%5Echromium$&l=251 > >> > just have an `&& !use_lld` at the end? > >> > >> Because: > >> > >> 1) I want to move us to LLD everywhere where we use Gold now > >> 2) LLD has a different format of output file. If we allow both formats, > >> that > >> will need to be supported in the analysis tool. > >> 3) I want to enable this option again, as the work Andrew is doing (size > >> analysis based on these maps) is an important one. > >> > > > > Sure, but the interesting bit is use_lld, not is_clang, no? > > > > For example, if we were to switch the android bots to clang (+ gold) then > we'd want to keep the map file. > > -- > 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. I think the interesting this is just linux vs android. If we switched Android to clang + lld, I'd still want linker maps to be enabled.
On 2017/04/28 18:13:04, krasin1 wrote: > Why don't to switch Android bots to Clang+LLD? > > I mean, I would rather have enable_linker_map = use_lld, after the next Clang > roll, so we only compute maps if LLD is used. While LLD is about the same performance in case of LTO (as it's mostly the same code running in LLVM Gold plugin; and it takes the majority of the time), for regular builds LLD is usually 2x-3x faster than Gold.
On Fri, Apr 28, 2017 at 2:13 PM, <krasin@chromium.org> wrote: > Why don't to switch Android bots to Clang+LLD? > We'll probably do that one day, but switching fewer things at once is better imo. My understanding here is that this change is needed to work around a performance bug in LLD, and that this is unrelated to the compiler. Am I misunderstanding something? > > I mean, I would rather have enable_linker_map = use_lld, after the next > Clang > roll, so we only compute maps if LLD is used. > > https://codereview.chromium.org/2849583005/ > -- 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 the interesting this is just linux vs android. If we switched Android to > clang + lld, I'd still want linker maps to be enabled. Right. And LLD has already got this fixed. We'll get this fix into the Chromium land after the next Clang roll. So, my proposal: disable maps on Linux today; enable maps on Linux for LLD only after the roll happened; when switching Android to Clang, switch it to Clang+LLD and compute maps using LLD there. Does it make sense?
> We'll probably do that one day, but switching fewer things at once is > better imo. Correct. I also feel this way. > > My understanding here is that this change is needed to work around a > performance bug in LLD, and that this is unrelated to the compiler. Am I > misunderstanding something? Also correct. That's why we can switch to LLD ahead of switching to Clang, assuming Android already has the Clang toolchain available on the file system. > > > > > > I mean, I would rather have enable_linker_map = use_lld, after the next > > Clang > > roll, so we only compute maps if LLD is used. > > > > https://codereview.chromium.org/2849583005/ > > > > -- > 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.
On Fri, Apr 28, 2017 at 2:18 PM, <krasin@chromium.org> wrote: > > > I think the interesting this is just linux vs android. If we switched > Android > to > > clang + lld, I'd still want linker maps to be enabled. > > Right. And LLD has already got this fixed. We'll get this fix into the > Chromium > land after the next Clang roll. > > So, my proposal: disable maps on Linux today; enable maps on Linux for LLD > only > after the roll happened; when switching Android to Clang, switch it to > Clang+LLD > and compute maps using LLD there. > > Does it make sense? > Yes, but why don't do 1 with a conditional on use_lld since that's what you really want to check? (And then once the fixed toolchain is rolled in, remove that check again.) > > https://codereview.chromium.org/2849583005/ > -- 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by krasin@chromium.org to run a CQ dry run
Nico, I have updated the patch as you suggested. Since I have just a few days left, it's certainly your call how to handle this.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
patch set 2 lgtm It's possible that patch set 1 is good too, I just don't yet understand the thinking behind it :-/
On 2017/04/28 18:55:04, Nico wrote: > patch set 2 lgtm > > It's possible that patch set 1 is good too, I just don't yet understand the > thinking behind it :-/ no worries; your suggestion is just as good, and I also buy the argument that the change looks less confusing. I still recommend to try switching to LLD even before switching compilation to Clang.
On 2017/04/28 18:56:41, krasin1 wrote: > On 2017/04/28 18:55:04, Nico wrote: > > patch set 2 lgtm > > > > It's possible that patch set 1 is good too, I just don't yet understand the > > thinking behind it :-/ > > no worries; your suggestion is just as good, and I also buy the argument that > the change looks less confusing. > > I still recommend to try switching to LLD even before switching compilation to > Clang. .... on Android.
The CQ bit was unchecked by krasin@chromium.org
The CQ bit was checked by krasin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2849583005/#ps20001 (title: "!use_lld")
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": 20001, "attempt_start_ts": 1493406612623900,
"parent_rev": "8e5e44bc2a23a1599de02595e445257571f4f76f", "commit_rev":
"317d9edab7ba87a5c05564ca8c66a12911c46872"}
Message was sent while issue was closed.
Description was changed from ========== Disable linker map for Clang toolchains. Right now, we are transitioning to LLD, and LLD has a different map format (so, the analysis tool will need to be updated), as well as suboptimal implementation that adds 60 minutes to the Chrome link time. This will be reenabled when both of the blockers above are resolved. BUG=716209,607968 ========== to ========== Disable linker map for Clang toolchains. Right now, we are transitioning to LLD, and LLD has a different map format (so, the analysis tool will need to be updated), as well as suboptimal implementation that adds 60 minutes to the Chrome link time. This will be reenabled when both of the blockers above are resolved. BUG=716209,607968 Review-Url: https://codereview.chromium.org/2849583005 Cr-Commit-Position: refs/heads/master@{#468113} Committed: https://chromium.googlesource.com/chromium/src/+/317d9edab7ba87a5c05564ca8c66... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/317d9edab7ba87a5c05564ca8c66...
Message was sent while issue was closed.
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
Message was sent while issue was closed.
lgtm |
