Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1680943002:
add --gdb-index to linker (Closed)
Descriptionadds gdb-index option for linker
This cl adds a new configuration flag to gold linker: "gdb_index"
The flag is on by default if is_desktop_linux && is_debug && is_component_build
The previous attempt to add this flag to all the builds was reverted
because of disk-space requirements. This checkin makes it a default
only for linux debug builds.
Why do this?
gold linker has a flag to automatically generate gdb-index section
inside shared libraries when linking.
Webkit single-file compile/linking time:
16.31s without gdb-index
16.11s with gdb-index
gdb startup time
~51s without gdb-index
~19s with gdb-index
BUG=
Committed: https://crrev.com/f68b7c07a30baac088b8eda055ba52835c04692c
Cr-Commit-Position: refs/heads/master@{#378600}
Committed: https://crrev.com/fcb018ec85c2e56e44180bb3e2a822082826000d
Cr-Commit-Position: refs/heads/master@{#383324}
Patch Set 1 #Patch Set 2 : Added --gdb-index to common.gypi #Patch Set 3 : gdb_index as a flag #
Total comments: 2
Patch Set 4 : make flag default to false #Messages
Total messages: 41 (11 generated)
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
dpranke@chromium.org changed reviewers: + thakis@chromium.org
Whether or not to use -Wl,gdb-index has been a subject of discussion a few times over the years. There is some relevant discussion on crbug.com/374952, and most recently in this thread on chromium-dev: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/CnRm0EIBp0E/dis... Nico, Sunny, it looks like the last messages on that thread were from you two. Nico, what sort of testing of link time did you have in mind? Would testing full rebuilds in static and component builds of debug and release be sufficient for you? Or would you want to land the change see the impact on cycle time, if any? Or something else? Regardless, it seems relatively harmless to add a build arg to make this toggle-able even if it is off by default; any objection to that (not that patchset #1 does that, but it would be straightforward to add that)?
On 2016/02/10 00:02:30, Dirk Pranke wrote: > Whether or not to use -Wl,gdb-index has been a subject of discussion a few times > over the years. There is some relevant discussion on crbug.com/374952, and most > recently in this thread on chromium-dev: > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/CnRm0EIBp0E/dis... > > Nico, Sunny, it looks like the last messages on that thread were from you two. > > Nico, what sort of testing of link time did you have in mind? Would testing full > rebuilds in static and component builds of debug and release be sufficient for > you? Or would you want to land the change see the impact on cycle time, if any? > Or something else? > > Regardless, it seems relatively harmless to add a build arg to make this > toggle-able even if it is off by default; any objection to that (not that > patchset #1 does that, but it would be straightforward to add that)? What does single file link time mean here? Last time I enabled this flag I noticed that linking large modules like content was definitely slower. P.S. I would really like the gdb-add-index script to work with clang_debug_fission if that's at all possible.
On 2016/02/10 00:15:54, sunnyps wrote:
> What does single file link time mean here? Last time I enabled this flag I
> noticed that linking large modules like content was definitely slower.
>
> P.S. I would really like the gdb-add-index script to work with
> clang_debug_fission if that's at all possible.
Here is the complete test suite, what was tested, and run times.
--gdb-index flag cuts down my gdb loading time by 30s (60%), and slows
down linking by 1.5s. It is a big win in a compile/debug cycle.
# GDB-INDEX TIMING TESTS
I've tested these scenarios on my desktop rig (Linux, 48cores) with different
flags
1) clean and rebuild
> ninja -C out/Develop -t clean
> time ninja -C out/Develop content_shell
2) touch single webkit file and rebuild
> touch third_party/WebKit/Source/core/layout/LayoutTableCol.cpp
> time ninja -C out/Develop content_shell
3) debug start time
gdb> attach $pic
All builds have these flags
is_debug = true
is_component_build = true
Here are the test results
## no special flags
1) 10m50.905s
2) 14.539s
3) 55s
## linux_use_debug_fission=1
1) 10m55.494s
2) 16.727s
3) 54s
## --gdb-index, linux_use_debug_fission=1
1) 11m2.701s
2) 16.762s
3) 21s
## --gdb-index
1) 10m52.604s
2) 15.918s
3) 21s
These numbers look good to me. IIRC one problem of this was that it only works with newer gdbs and/or it made gold crash every now and then, but maybe I'm misremembering this? It's been a while, might be worth trying again. I don't have objections to this, given these numbers.
(but gyp and gn should match here)
On 2016/02/10 21:53:26, Nico wrote: > These numbers look good to me. IIRC one problem of this was that it only works > with newer gdbs and/or it made gold crash every now and then, but maybe I'm > misremembering this? It's been a while, might be worth trying again. I don't > have objections to this, given these numbers. I think you do remember correctly, but that was a year or two ago, and I think we're on new enough stuff now. I do worry a bit that the numbers might not be representative enough, because linking content_shell is only doing a few links (as opposed to building all or a building a bunch of the test binaries). But, it's easy enough to land this and watch the cycle times to see.
On 2016/02/10 21:53:44, Nico wrote: > (but gyp and gn should match here) I'm not actually sure if I care about this that much, given that this is a linux-only change and there's not many linux bots left that still build w/ gyp.
android, chromeos both are still on gyp by default, no?
On 2016/02/10 22:49:44, Nico wrote: > android, chromeos both are still on gyp by default, no? Android is not, but yes, ChromeOS is. To be clear, I have no objection to adding it to the GYP build, I just don't care so much :).
@atotic - I suggest we add the same flag to the equivalent place in //build/common.gypi: https://code.google.com/p/chromium/codesearch?q=Wl%2C--threads#chromium/src/b... then we can land the change, watch the cycle times on the bots for an hour or two to see if anything explodes (the first builds on the bot won't really count since they'll have to relink everything), and then send a PSA email out to chromium-dev@ indicating that we've made this change. If there really is little to no impact on build time, I don't see a need to add an easy way to turn this off, but let's see what happens. Sound good?
@dpranke I've added the flag to common.gypi. When should we land? On 2016/02/19 02:06:44, Dirk Pranke wrote: > @atotic - > > I suggest we add the same flag to the equivalent place in //build/common.gypi: > > https://code.google.com/p/chromium/codesearch?q=Wl%2C--threads#chromium/src/b... > > then we can land the change, watch the cycle times on the bots for an hour or > two to see if anything explodes (the first builds on the bot won't really count > since they'll have to relink everything), and then send a PSA email out to > chromium-dev@ indicating that we've made this change. > > If there really is little to no impact on build time, I don't see a need to add > an easy way to turn this off, but let's see what happens. > > Sound good?
lgtm. On 2016/02/22 22:58:09, atotic1 wrote: > @dpranke > > I've added the flag to common.gypi. When should we land? Normally I would say "whenever you like, just hit the commit checkbox", but I actually just landed another change that is going to cause the CQ linux bots to build and link a bunch more targets, and I need to monitor them to assess the impact on the bot cycle time, and landing this CL would seriously confuse the data. So, mind holding off for a few days?
On 2016/02/22 23:17:12, Dirk Pranke wrote: > lgtm. > > > On 2016/02/22 22:58:09, atotic1 wrote: > > @dpranke > > > > I've added the flag to common.gypi. When should we land? > > Normally I would say "whenever you like, just hit the commit checkbox", but > I actually just landed another change that is going to cause the CQ linux bots > to build and link a bunch more targets, and I need to monitor them to assess > the impact on the bot cycle time, and landing this CL would seriously confuse > the data. > > So, mind holding off for a few days? Will try on Wednesday, and announce it here.
On 2016/02/22 23:18:22, atotic1 wrote: > Will try on Wednesday, and announce it here. I was thinking more like Friday (or maybe Thursday), if that's okay?
The CQ bit was checked by atotic@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1680943002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1680943002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== add --gdb-index to linker gold linker has a flag to automatically generate gdb-index section inside shared libraries when linking. Webkit single-file compile/linking time: 16.31s without gdb-index 16.11s with gdb-index gdb startup time ~51s without gdb-index ~19s with gdb-index BUG= ========== to ========== add --gdb-index to linker gold linker has a flag to automatically generate gdb-index section inside shared libraries when linking. Webkit single-file compile/linking time: 16.31s without gdb-index 16.11s with gdb-index gdb startup time ~51s without gdb-index ~19s with gdb-index BUG= Committed: https://crrev.com/f68b7c07a30baac088b8eda055ba52835c04692c Cr-Commit-Position: refs/heads/master@{#378600} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f68b7c07a30baac088b8eda055ba52835c04692c Cr-Commit-Position: refs/heads/master@{#378600}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1762873002/ by stevenjb@chromium.org. The reason for reverting is: We suspect this may be causing an increase in binary size on some chromeos PFQ builders. Reverting to see if this is the cause. .
Message was sent while issue was closed.
mstensho@opera.com changed reviewers: + mstensho@opera.com
Message was sent while issue was closed.
I've noticed really slow startup times for gdb when run inside emacs recently, and a git bisect lead me to this commit. Before this commit, it took about 2 minutes. After this commit, it takes 15-20 minutes or something like that. I don't use shared libraries, FWIW.
Message was sent while issue was closed.
On 2016/03/04 15:59:38, mstensho wrote: > I've noticed really slow startup times for gdb when run inside emacs recently, > and a git bisect lead me to this commit. Before this commit, it took about 2 > minutes. After this commit, it takes 15-20 minutes or something like that. > > I don't use shared libraries, FWIW. Odd, given that that's the exact opposite of what I'd expect ...
Message was sent while issue was closed.
Description was changed from ========== add --gdb-index to linker gold linker has a flag to automatically generate gdb-index section inside shared libraries when linking. Webkit single-file compile/linking time: 16.31s without gdb-index 16.11s with gdb-index gdb startup time ~51s without gdb-index ~19s with gdb-index BUG= Committed: https://crrev.com/f68b7c07a30baac088b8eda055ba52835c04692c Cr-Commit-Position: refs/heads/master@{#378600} ========== to ========== adds gdb-index option for linker This cl adds a new configuration flag to gold linker: "gdb_index" The flag is on by default if is_desktop_linux && is_debug && is_component_build The previous attempt to add this flag to all the builds was reverted because of disk-space requirements. This checkin makes it a default only for linux debug builds. Why do this? gold linker has a flag to automatically generate gdb-index section inside shared libraries when linking. Webkit single-file compile/linking time: 16.31s without gdb-index 16.11s with gdb-index gdb startup time ~51s without gdb-index ~19s with gdb-index BUG= Committed: https://crrev.com/f68b7c07a30baac088b8eda055ba52835c04692c Cr-Commit-Position: refs/heads/master@{#378600} ==========
Message was sent while issue was closed.
Patchset #3 (id:40001) has been deleted
Message was sent while issue was closed.
We had to revert because of disk space requirements. I've made gdb_index a flag option I am unsure if it should ever be on by default. Currently it defaults to: is_desktop_linux && is_debug && is_component_build. This will still bite emacs gdb users. They could turn it off with gn args. I am not qualified to make this call, not sure what our dev mix is like.
Message was sent while issue was closed.
https://codereview.chromium.org/1680943002/diff/60001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/1680943002/diff/60001/build/config/compiler/B... build/config/compiler/BUILD.gn:81: gdb_index = is_desktop_linux && is_debug && is_component_build Let's just have this default to false for now and land it. We can change the value later if need be.
Message was sent while issue was closed.
Made flag default to false. https://codereview.chromium.org/1680943002/diff/60001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/1680943002/diff/60001/build/config/compiler/B... build/config/compiler/BUILD.gn:81: gdb_index = is_desktop_linux && is_debug && is_component_build On 2016/03/24 20:20:41, Dirk Pranke wrote: > Let's just have this default to false for now and land it. We can change the > value later if need be. Done. New patch uploaded.
Message was sent while issue was closed.
lgtm. In the future, you might consider creating a new CL, uploading the initial version of the patch, and then uploading the fix, so that you have a clean issue with a record of what changed from the revert. Uploading a fix to the same CL is fine too, but many people (including me) prefer the new-CL approach.
Message was sent while issue was closed.
Description was changed from ========== adds gdb-index option for linker This cl adds a new configuration flag to gold linker: "gdb_index" The flag is on by default if is_desktop_linux && is_debug && is_component_build The previous attempt to add this flag to all the builds was reverted because of disk-space requirements. This checkin makes it a default only for linux debug builds. Why do this? gold linker has a flag to automatically generate gdb-index section inside shared libraries when linking. Webkit single-file compile/linking time: 16.31s without gdb-index 16.11s with gdb-index gdb startup time ~51s without gdb-index ~19s with gdb-index BUG= Committed: https://crrev.com/f68b7c07a30baac088b8eda055ba52835c04692c Cr-Commit-Position: refs/heads/master@{#378600} ========== to ========== adds gdb-index option for linker This cl adds a new configuration flag to gold linker: "gdb_index" The flag is on by default if is_desktop_linux && is_debug && is_component_build The previous attempt to add this flag to all the builds was reverted because of disk-space requirements. This checkin makes it a default only for linux debug builds. Why do this? gold linker has a flag to automatically generate gdb-index section inside shared libraries when linking. Webkit single-file compile/linking time: 16.31s without gdb-index 16.11s with gdb-index gdb startup time ~51s without gdb-index ~19s with gdb-index BUG= Committed: https://crrev.com/f68b7c07a30baac088b8eda055ba52835c04692c Cr-Commit-Position: refs/heads/master@{#378600} ==========
but if you re-use the same CL, you'll need to re-open it (which I just did on this one).
The CQ bit was checked by atotic@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1680943002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1680943002/80001
Message was sent while issue was closed.
Description was changed from ========== adds gdb-index option for linker This cl adds a new configuration flag to gold linker: "gdb_index" The flag is on by default if is_desktop_linux && is_debug && is_component_build The previous attempt to add this flag to all the builds was reverted because of disk-space requirements. This checkin makes it a default only for linux debug builds. Why do this? gold linker has a flag to automatically generate gdb-index section inside shared libraries when linking. Webkit single-file compile/linking time: 16.31s without gdb-index 16.11s with gdb-index gdb startup time ~51s without gdb-index ~19s with gdb-index BUG= Committed: https://crrev.com/f68b7c07a30baac088b8eda055ba52835c04692c Cr-Commit-Position: refs/heads/master@{#378600} ========== to ========== adds gdb-index option for linker This cl adds a new configuration flag to gold linker: "gdb_index" The flag is on by default if is_desktop_linux && is_debug && is_component_build The previous attempt to add this flag to all the builds was reverted because of disk-space requirements. This checkin makes it a default only for linux debug builds. Why do this? gold linker has a flag to automatically generate gdb-index section inside shared libraries when linking. Webkit single-file compile/linking time: 16.31s without gdb-index 16.11s with gdb-index gdb startup time ~51s without gdb-index ~19s with gdb-index BUG= Committed: https://crrev.com/f68b7c07a30baac088b8eda055ba52835c04692c Cr-Commit-Position: refs/heads/master@{#378600} ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== adds gdb-index option for linker This cl adds a new configuration flag to gold linker: "gdb_index" The flag is on by default if is_desktop_linux && is_debug && is_component_build The previous attempt to add this flag to all the builds was reverted because of disk-space requirements. This checkin makes it a default only for linux debug builds. Why do this? gold linker has a flag to automatically generate gdb-index section inside shared libraries when linking. Webkit single-file compile/linking time: 16.31s without gdb-index 16.11s with gdb-index gdb startup time ~51s without gdb-index ~19s with gdb-index BUG= Committed: https://crrev.com/f68b7c07a30baac088b8eda055ba52835c04692c Cr-Commit-Position: refs/heads/master@{#378600} ========== to ========== adds gdb-index option for linker This cl adds a new configuration flag to gold linker: "gdb_index" The flag is on by default if is_desktop_linux && is_debug && is_component_build The previous attempt to add this flag to all the builds was reverted because of disk-space requirements. This checkin makes it a default only for linux debug builds. Why do this? gold linker has a flag to automatically generate gdb-index section inside shared libraries when linking. Webkit single-file compile/linking time: 16.31s without gdb-index 16.11s with gdb-index gdb startup time ~51s without gdb-index ~19s with gdb-index BUG= Committed: https://crrev.com/f68b7c07a30baac088b8eda055ba52835c04692c Cr-Commit-Position: refs/heads/master@{#378600} Committed: https://crrev.com/fcb018ec85c2e56e44180bb3e2a822082826000d Cr-Commit-Position: refs/heads/master@{#383324} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/fcb018ec85c2e56e44180bb3e2a822082826000d Cr-Commit-Position: refs/heads/master@{#383324} |
