1188823 - Use /DEBUG:FASTLINK for local developer builds using Visual C++ 2015

Status: NEW

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

Per https://randomascii.wordpress.com/2015/07/27/programming-is-puzzles/ , Visual C++ 2015 introduced a new (but undocumented) linker flag: /DEBUG:FASTLINK. This apparently does something similar to -gsplit-dwarf, or what Apple's linker does: not link debug all debug info from object files into the resulting PDB. The PDB files are still useful for local debugging, but not for distribution, so we wouldn't want to use this for builds in automation.

Apparently it results in a significant linking speedup.

Comment 1

[Salah]

Hi, I would like to work on this bug. Can you provide me with a few steps on how to start?
Thanks

Comment 2

[AJ Kerrigan [:ajkerrigan]]

I'm going to take a crack at this one. I also wanted to include this link from the Visual C++ team blog: https://blogs.msdn.microsoft.com/vcblog/2015/10/16/debugfastlink-for-vs2015-update-1/

It has some useful info about the /DEBUG:FASTLINK flag that wasn't available when this bug was filed.

Comment 3

[AJ Kerrigan [:ajkerrigan]]

Attachment: Use /DEBUG:FASTLINK for local dev builds with VC++ 2015

I took a look at this and had some questions, but to get the conversation
started I figured I'd make the simplest change that does the job in my
VS2015 environment.

I'd like to kick this around a little with you, either here or via IRC
(whichever's easier). My main questions are:

- Is there anything critical that this patch does not account for?

- I see that logic is being moved out of configure.in files. Should
  the DEBUG flags be determined elsewhere (config.mk, compiler-opts.m4, etc)
  or is that outside the scope of this bug?

- Is the combination of DEVELOPER_OPTIONS and _MSC_VER >= 1900 the most
  effective way to identify a local developer build using Visual C++ 2015
  or higher, or is there another preferred way?

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 8724878 [details] [diff] [review]
Use /DEBUG:FASTLINK for local dev builds with VC++ 2015

Review of attachment 8724878 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch, and for the link to the blog post! We are trying to move things out of old-configure.in into the new Python configure setup, but that's a little tricky (bug 1250301), so I don't think you can easily do it in this patch unfortunately. You could ask glandium on #build if he has an estimated timeline for that work if you would like to try to wait and implement this patch on top of it. We're actively trying to move things out of config.mk, so configure-or-its-replacement is probably the best spot for this data.

The check you have is the right check, $DEVELOPER_OPTIONS is the thing to use.

I can't think of anything you've missed offhand, I suppose local testing would be the best thing here, just sanity check that doing a local build and debugging the resulting binary doesn't show anything obviously broken? It would also be neat to get timings on an incremental rebuild to see what kind of speedup we get. Something like `touch toolkit/xre/nsAppRunner.cpp; time ./mach build binaries` after a full build with and without this patch would be interesting to know! (Probably run a few times in a row to get an average.)

::: js/src/old-configure.in
@@ +1642,5 @@
>          CFLAGS="$CFLAGS -we4553"
>          CXXFLAGS="$CXXFLAGS -we4553"
>          LIBS="$LIBS kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib psapi.lib"
> +        # Speed up linking for local developer builds using Visual C++ 2015 or higher
> +        # See bug 1188823

You can leave off the "see bug N" bit. I know you probably saw this elsewhere in the file, but this is why we have hg blame. :)

Comment 5

[AJ Kerrigan [:ajkerrigan]]

I made these changes a while back and tried testing them, but even though I see the flag being used there's no difference in timing or PDB size. So something is preventing it from working properly, and I'm not sure what that is.

I'd like to get back and dig into it some more, but my free time for this sort of fiddling is going to be nil for a while. I'm unassigning and leaving this update in case anyone else wants to pick up where I left off in the meantime.

Comment 6

[(Away)]

(In reply to AJ Kerrigan [:ajkerrigan] from comment #5)
> I made these changes a while back and tried testing them, but even though I
> see the flag being used there's no difference in timing or PDB size. So
> something is preventing it from working properly, and I'm not sure what that
> is.

With a vanilla mozconfig, your linker command for xul.dll will include:
-DEBUG -DEBUGTYPE:CV -DEBUG -OPT:REF

The first part comes from here, as you've found: https://dxr.mozilla.org/mozilla-central/rev/f9f3cc95d7282f1fd83f66dd74acbcdbfe821915/old-configure.in#1173

The second part comes from: https://dxr.mozilla.org/mozilla-central/rev/f9f3cc95d7282f1fd83f66dd74acbcdbfe821915/config/config.mk#172

If we add FASTLINK only to the first switch then the feature doesn't get turned on because the second -DEBUG wins. We should probably just remove the second -DEBUG. When I tried that, I saw some nice wins:

With -OPT:REF, full link: 29s -> 14s
Without -OPT:REF, full link: 26s -> 10s
Without -OPT:REF, no-op incremental link: 0.6s -> 0.5s

(These are from |link.exe -time| and don't include mach overhead. Also note that -OPT:REF disables incremental linking.)

Comment 7

[(Away)]

I'd like to propose some cleanups to help get this patch working. Ted, what do you think of these? (I'm happy to do these in a separate bug if you like)

(1) Remove -DEBUGTYPE:CV. It's a default setting and we don't need to set it explicitly.

(2) Remove the -DEBUG from https://dxr.mozilla.org/mozilla-central/rev/f9f3cc95d7282f1fd83f66dd74acbcdbfe821915/config/config.mk#172

(3) As long as we're touching (2), we can just take the 64-bit codepath on all platforms. "OPT:REF,ICF" is identical to "OPT:REF" on 32-bit builds.

(4) As long as we're touching (2), we shouldn't tie OPT:REF, which is a perf optimization, to the presence of MOZ_DEBUG_SYMBOLS.

(5) (Maybe controversial) If we're already going to venture into DEVELOPER_OPTIONS territory, how about disabling OPT:REF on all developer builds, including non-debug ones? The plus side is it unlocks incremental linking on non-debug builds; the downside is it adds another deviation from official builds.

Comment 8

[(Away)]

So, apparently FASTLINK breaks the ability to examine symbols in WinDbg ("x xul!*" comes up completely empty). This is kind of a deal-breaker IMO. :-(

Comment 9

[Gregory Szorc [:gps]]

(In reply to David Major [:dmajor] from comment #8)
> So, apparently FASTLINK breaks the ability to examine symbols in WinDbg ("x
> xul!*" comes up completely empty). This is kind of a deal-breaker IMO. :-(

Search the internets for mspdbcmf.exe (hint: it is part of VS2015u2). I reckon we could integrate that into `mach run --debug`.

Comment 10

[(Away)]

Setting aside the fact that it feels like a clunky extra step to go through just to debug my own build, mspdbcmf.exe takes 55 seconds or nearly 4x the time that FASTLINK saved me.

Comment 11

[Gregory Szorc [:gps]]

(In reply to David Major [:dmajor] from comment #10)
> Setting aside the fact that it feels like a clunky extra step to go through
> just to debug my own build,

That's what tools are for :)

> mspdbcmf.exe takes 55 seconds or nearly 4x the time that FASTLINK saved me.

bleh :/ Microsoft has been blogging about FASTLINK a lot lately, including in the VS "15" posts. e.g. https://blogs.msdn.microsoft.com/vcblog/2016/10/05/faster-c-build-cycle-in-vs-15-with-debugfastlink/

I wonder if they've improved times at all. If not, it might be worth filing a Connect bug to get our use case on their radar.

Comment 12

[(not currently active) Ted Mielczarek]

(In reply to David Major [:dmajor] from comment #8)
> So, apparently FASTLINK breaks the ability to examine symbols in WinDbg ("x
> xul!*" comes up completely empty). This is kind of a deal-breaker IMO. :-(

That's a bummer. :-( I would definitely file it in Connect, it'd be great to be able to use this by default but breaking useful debug functionality isn't good.

FWIW, all your suggestions in comment 7 sound fine. I would actually just move all the flag selection to configure, there's no need to select any of that at build time in rules.mk.

Comment 13

[Honza Bambas (:mayhemer)]

I just merged and tried this patch locally on a debug build (vs2015, x86) and I haven't any trouble debugging the code.  Stacks are unwinded, local vars resolved, everything works.  And it saves almost 2 minutes of build time!

Comment 14

[(Away)]

(In reply to Honza Bambas (:mayhemer) from comment #13)
> I just merged and tried this patch locally on a debug build (vs2015, x86)
> and I haven't any trouble debugging the code.  Stacks are unwinded, local
> vars resolved, everything works.  And it saves almost 2 minutes of build
> time!

But can you enumerate and search symbols, e.g. "x xul!*webgl2*specs" in WinDbg? (I don't know the equivalent in Visual Studio.)

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to David Major [:dmajor] from comment #14)
> (In reply to Honza Bambas (:mayhemer) from comment #13)
> > I just merged and tried this patch locally on a debug build (vs2015, x86)
> > and I haven't any trouble debugging the code.  Stacks are unwinded, local
> > vars resolved, everything works.  And it saves almost 2 minutes of build
> > time!
> 
> But can you enumerate and search symbols, e.g. "x xul!*webgl2*specs" in
> WinDbg? (I don't know the equivalent in Visual Studio.)

Empty.  I don't know an eq in VS too..

Is the enumeration breaking something we seriously need?  Because I've never enumerated symbols in my life :)

For now I have this as a root patch to speed up my building.  If this is made optional for those who can benefit, I would be super happy.

Comment 16

[(Away)]

I search symbols by wildcard all the time. :) Mostly because I can't keep track of what namespace everything is in -- and templates and "`anonymous namespace'::" are especially difficult for WinDbg to parse. Also "bm" to set wildcard breakpoints when it complains about overloaded function names being ambiguous.

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to David Major [:dmajor] from comment #16)
> I search symbols by wildcard all the time. :) Mostly because I can't keep
> track of what namespace everything is in -- and templates and "`anonymous
> namespace'::" are especially difficult for WinDbg to parse. Also "bm" to set
> wildcard breakpoints when it complains about overloaded function names being
> ambiguous.

Interesting feature.  Honestly, I never use WinDbg my self, except some very extreme cases.  VS does all I need for me.

But it can't put breaks points according symbol matching.  Isn't there an extension for it?

Comment 19

[Honza Bambas (:mayhemer)]

OK, could we please move forward here?  Applying this locally after every even small repository update means to rebuild completely, since configure has changed with it.

I believe making this a configure option would be a perfect solution.  Some of us (I believe a lot of us!) would benefit from turning it on and others can easily turn it off when it breaks stuff.

Anybody against?

Comment 20

[(Away)]

I could live with this as long as I can opt in and out of /DEBUG:FASTLINK orthogonally from whatever happens in bug 1341504. In other words, please don't tie _both_ of them to $DEVELOPER_OPTIONS. So a configure option for this bug would work for me.

Comment 21

[Honza Bambas (:mayhemer)]

FYI:  :((

https://connect.microsoft.com/VisualStudio/feedback/details/2584625/crash-while-navigating-in-call-stack-window

Reproducible under some conditions regularly under VS2015 IDE attached as a debugger.  For me it crashes instantly when a break-point is hit...

Going to check how VS2017 works.

Comment 22

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #21)
> Going to check how VS2017 works.

The RC released today still crashes :((((

Comment 23

[(not currently active) Ted Mielczarek]

(In reply to Honza Bambas (:mayhemer) from comment #21)
> https://connect.microsoft.com/VisualStudio/feedback/details/2584625/crash-
> while-navigating-in-call-stack-window

This link doesn't work for me, is there a better link?

Comment 24

[Honza Bambas (:mayhemer)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #23)
> (In reply to Honza Bambas (:mayhemer) from comment #21)
> > https://connect.microsoft.com/VisualStudio/feedback/details/2584625/crash-
> > while-navigating-in-call-stack-window
> 
> This link doesn't work for me, is there a better link?

Can't find it anymore... :/  The report said that the particular crash was happening when debugging a code linked with /debug:fastlink.  Removing that switch helped to fix it.

I was testing with VS2017 fastlink builds.  They can't be debugged in VS2015 IDE (not surprisingly) - there is a freeze at the start of the debug session.  And they crash (but in a bit different way) in VS2017.  I was only testing with fastlink builds, so hard to say if it's not a different issue of the 2017 IDE.