Don't apply msvc link opts for non-opt build

[upsuper]

/OPT:REF,ICF sometimes takes lots of time. It makes no sense to apply them when doing debug build. MSVC's linker by default disables these optimizations when /DEBUG is specified, unless they are explicitly passed.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[upsuper]

As a comparison, for the build script of libbindgen, with /OPT:REF,ICF, it takes 1m40s to link on my machine, but without that, 0.55s.

[retep998]

gc_sections for gnu linkers doesn't check whether optimizations are enabled before passing its equivalent flags. This would mean that we'd be ignoring sess.opts.cg.link_dead_code when we're using the msvc linker with optimizations disabled.

If we are going to disable /OPT:REF,ICF, we really ought to do incremental linking at the same time. #37543

[upsuper]

I agree that we should do incremental linking as well. But I think that change could be more complicated than simply turning off the link-time optimization for debug build. And this trivial change shows a great win on link time, especially for crates who have lots of generated code.

As I've mentioned above, switching from /OPT:REF,ICF to nothing reduces link time for build script of libbindgen from ~100s to ~0.55s. Also it reduces the time of linking the whole rust-bindgen binary from ~185s to ~1.1s. It is a ~170x improvement.

So I think we should take it as soon as possible without letting further potential improvement block it.

[ollie27]

/INCREMENTAL is enabled by default unless /OPT is also passed so this patch may be enough to enable incremental linking.

[retep998]

@ollie27 Cargo deletes the old binary before invoking rustc, so at the moment there is no chance of the result of previous linking being usable for incremental linking. Making incremental linking actually possible should be as simple as telling cargo to not delete that sort of stuff, although it'll be a while before anyone actually teaches cargo when to do that.

[ollie27]

Well maybe we should explicitly pass /INCREMENTAL:NO until we fix Cargo so it doesn't generate unnecessary .ilk files etc.

[cpeterso]

@Manishearth or @pnkfelix, can you review @upsuper's linker change here? This is a big productivity issue. This PR reduces his link times from three minutes (!!) to one second by disabling MSVC LTO for debug builds.

[upsuper]

Making incremental linking actually possible should be as simple as telling cargo to not delete that sort of stuff, although it'll be a while before anyone actually teaches cargo when to do that.

Incremental linking may need more investigation to see whether that works well with Rust's binaries. I did see an issue that incremental linked binary (linking a Rust library into Gecko) sometimes has weird runtime error inside Rust code.

[alexcrichton]

@upsuper

Thanks for the PR! Sorry about taking a bit to get around to this. I'd be curious to play around a bit here with the various options before we just turn this off entirely. We get significant size wins sometimes with --gc-sections-equivalent options, so it's beneficial to have this turned on all the time if we can.

IIRC REF and ICF are separate optimizations. I don't recall ever measuring either independently, so I'd be curious about what the effect is by using only one of those rather than both. Could you measure the compile times there?

Also, could you take a look at the traditional "hello world" and see what the difference in the size of the executable is? Does it jump by megabytes or does it remain the same?

In any case I think we definitely need to change something here because a 100s link time is crazy huge. Just want to explore the possibility space ahead of time!

[upsuper]

I tried building the build script of libbindgen. This is the data:

OPT	Link time	Binary size
REF,ICF	85.51s	5,648 KB
REF,NOICF	0.63s	7,467 KB
ICF	143.89s	7,790 KB
(None)	0.63s	12,795 KB

And for binary of rust-bindgen:

OPT	Link time	Binary size
REF,ICF	147.81s	9,283 KB
REF,NOICF	0.98s	12,854 KB
ICF	156.14s	12,748 KB
(None)	1.00s	21,513 KB

I didn't run them many times, but it seems to me the time consumption is roughly stable, and somehow faster than my measurement before. Probably because that laptop is not doing anything else this time.

So it seems the binary size is indeed much larger without the optimizations (~2.3x), but the reduction of time consumption should well justify that difference.

[upsuper]

Oh, /OPT:REF indicates /OPT:ICF as well. Let me test again with /OPT:REF,NOICF.

[upsuper]

Updated the data with /OPT:REF,NOICF. So it seems /OPT:REF,NOICF might be a good compromise between size and time consumption, which is as fast as no optimization, but still reduces the size to some extent.

[upsuper]

Updated the patch to use /OPT:REF,NOICF for non-opt build.

[alexcrichton]

Thanks for the investigation @upsuper! Definitely makes sense to me to leave REF on and turn ICF off.

@bors: r+

[bors]

📌 Commit 3fcbdfe has been approved by alexcrichton

[bors]

⌛ Testing commit 3fcbdfe with merge abc1ef6...

[bors]

💔 Test failed - auto-win-msvc-64-opt-rustbuild

[upsuper]

@alexcrichton Could you review the test change?

[alexcrichton]

@bors: r+

[bors]

📌 Commit 257f643 has been approved by alexcrichton

[bors]

⌛ Testing commit 257f643 with merge 1842efb...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild
Approved by: alexcrichton
Pushing 1842efb to master...