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

Issue 2770933009: linux: Improve gdb startup time for debug builds from over 4 minutes to 35s. (Closed)

Created:
3 years, 9 months ago by Nico
Modified:
3 years, 8 months ago
Reviewers:
Dirk Pranke, piman, mithro
CC:
chromium-reviews, Lei Zhang, awong, brettw, sky, Ken Russell (switch to Gerrit), Jeffrey Yasskin, Jakob Kummerow, piman
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

linux: Improve gdb startup time for debug builds from over 4 minutes to 35s. This change contains two parts, for full-symbol (symbol_level=2, default) and reduced-symbol (symbol_level=1) debug builds. It has no effect on release builds. 1.) For symbol_level=2, pass -Wl,--gdb-index to the linker. This lets the linker write an index that lets it load binaries much faster. gdb startup time for target chrome from goes from 60s to 10s and time from `run` in gdb to the program actually starting from 270s to 45s. In return, this slows down linking a bit, but for target chrome in a debug build, it increases link time from 37s to 42s, which is better than making people who want to use gdb wait several minutes every time they start gdb. There's some history here: We used to pass -Wl,--gdb-index long ago, and then removed it in https://codereview.chromium.org/335903002/, with the recommendation that people who want to use gdb could run build/gdb-add-index. But running `gdb-add-index chrome` takes 73s nowadays, a lot more than the 5s that gold needs. (Back then, gdb-add-index was faster, and gold, due to us not yet defaulting to component builds in debug builds, was slower. Also, people were on an older default Ubuntu and used an older gdb version.) People who don't use gdb should use symbol_level=1 for their builds anyhow (and bots do too), so this small regression in link time shouldn't affect them. Remove the explicit gdb_index gn arg now that this has a good default. 2) For symbol_level=1, make this mode actually work again after the gn swtich. In symbol_level=1 builds, gn would pass `-g1 -gsplit-dwarf` to clang (*). Surprisingly, -gsplit-dwarf implies -g2 with clang, so the -g1 gets overriden by -g2 immediately. Before this, symbol_level=1 in debug builds would produce full debug info. Since all bots set symbol_level=1, this might help with build speed on debug bots. For people who set this locally, it'll also speed up gdb startup time for target chrome from 39s to 13s and time from `run` in gdb to the program actually starting from 255s to 35s. *: clang always writes stack debug info to both .o and .dwo files, and lets the linker link them into the executable, so -g1 -gsplit-dwarf would make no sense. This was used as justification to make -gsplit-dwarf imply -g2, as it otherwise wouldn't have an effect. BUG=374952 R=piman@chromium.org, tansell@chromium.org Review-Url: https://codereview.chromium.org/2770933009 Cr-Original-Commit-Position: refs/heads/master@{#459790} Committed: https://chromium.googlesource.com/chromium/src/+/7b26c51270cebd5de672a155b34f30bc8282c35a Review-Url: https://codereview.chromium.org/2770933009 . Cr-Commit-Position: refs/heads/master@{#459824} Committed: https://chromium.googlesource.com/chromium/src/+/b9f7581b93ae8096afed29d415791803caf2e2e3

Patch Set 1 #

Patch Set 2 : errrrr #

Patch Set 3 : ios/mac, and remove flag #

Patch Set 4 : rebase #

Patch Set 5 : reland: no 32-bit #

Unified diffs Side-by-side diffs Delta from patch set Stats (+13 lines, -12 lines) Patch
M build/config/compiler/BUILD.gn View 1 2 3 4 5 chunks +13 lines, -12 lines 0 comments Download

Messages

Total messages: 39 (26 generated)
Nico
3 years, 9 months ago (2017-03-24 23:23:17 UTC) #8
piman
drive-by lgtm ship it.
3 years, 9 months ago (2017-03-25 00:23:01 UTC) #14
mithro
On 2017/03/25 00:23:01, piman wrote: > drive-by lgtm ship it. LGTM -- all this sounds ...
3 years, 9 months ago (2017-03-25 00:37:25 UTC) #15
Dirk Pranke
I'm out, so just skimmed this, but there is already a gdb_index GN arg. You ...
3 years, 9 months ago (2017-03-25 00:48:42 UTC) #16
Nico
On 2017/03/25 00:48:42, Dirk Pranke (slow until 28th) wrote: > I'm out, so just skimmed ...
3 years, 8 months ago (2017-03-27 14:58:59 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2770933009/60001
3 years, 8 months ago (2017-03-27 15:21:24 UTC) #28
commit-bot: I haz the power
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/7b26c51270cebd5de672a155b34f30bc8282c35a
3 years, 8 months ago (2017-03-27 16:07:11 UTC) #31
jwd
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2776193002/ by jwd@chromium.org. ...
3 years, 8 months ago (2017-03-27 16:29:33 UTC) #32
findit-for-me
Findit confirmed this CL at revision 459790 as the culprit for failures in the build ...
3 years, 8 months ago (2017-03-27 16:34:15 UTC) #33
Nico
Committed patchset #5 (id:80001) manually as b9f7581b93ae8096afed29d415791803caf2e2e3 (presubmit successful).
3 years, 8 months ago (2017-03-27 17:58:37 UTC) #36
sreten.kovacevic
I have a question about this patch. You removed GN arg gdb_index, but wouldn't it ...
3 years, 8 months ago (2017-04-06 09:56:16 UTC) #37
Nico
https://codereview.chromium.org/2791403004/ fixes the problem you're having, right? On Apr 6, 2017 5:56 AM, <sreten.kovacevic@imgtec.com> wrote: ...
3 years, 8 months ago (2017-04-06 12:06:59 UTC) #38
sreten.kovacevic
3 years, 8 months ago (2017-04-07 09:30:04 UTC) #39
Message was sent while issue was closed.
On 2017/04/06 12:06:59, Nico wrote:
> https://codereview.chromium.org/2791403004/ fixes the problem you're
> having, right?
> 
> On Apr 6, 2017 5:56 AM, <mailto:sreten.kovacevic@imgtec.com> wrote:
> 
> > I have a question about this patch. You removed GN arg gdb_index, but
> > wouldn't
> > it be much nicer solution to set it to true, add it in conditions and that
> > way
> > make it possible to disable this option in our build.
> >
> > For example, when compiling v8 on MIPS64 gold linker is not used and
> > --gdb-index
> > option is not supported, so we are forced to use symbol_level=1, which is
> > not
> > something we usually want. Also, this way it would be more obvious what is
> > happening with build, no need to explain what is purpose of specific GN
> > argument.
> >
> > https://codereview.chromium.org/2770933009/
> >
> 
> -- 
> 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.

yeah, didn't see the patch, thank you.

Powered by Google App Engine
This is Rietveld 408576698