1341504 - Incremental linking is effectively disabled on Windows

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

Per https://msdn.microsoft.com/en-us/library/4khtbfyf.aspx :

LINK performs a full link if any of the following options are specified:

    Link Incrementally is not selected (/INCREMENTAL:NO)

    /OPT:REF is selected

    /OPT:ICF is selected

    /OPT:LBR is selected

    /ORDER is selected

Per config.mk, we're always using /OPT:REF on win32, and /OPT:REF,ICF on win64.

Comment 1

[(Away)]

> Per config.mk, we're always using /OPT:REF on win32, and /OPT:REF,ICF on win64.

"Always" on opt builds, yes. Debug builds are ok -- there's an ifndef MOZ_DEBUG a bit further up.

Comment 2

[Honza Bambas (:mayhemer)]

(In reply to David Major [:dmajor] from comment #1)
> > Per config.mk, we're always using /OPT:REF on win32, and /OPT:REF,ICF on win64.
> 
> "Always" on opt builds, yes. Debug builds are ok -- there's an ifndef
> MOZ_DEBUG a bit further up.

Can confirm that, yes.  I presume we can't do anything about opt builds?

Comment 3

[(not currently active) Ted Mielczarek]

We could disable these options for non-automation builds to make local builds faster, at the expense of making them less like automation. We've already got the `--enable-release` switch that controls the `DEVELOPER_OPTIONS` variable in configure, we could just use that.

Comment 4

[Mike Hommey [:glandium]]

BTW, as a data point, Linux builds already do that with ICF (disabling on local builds, enabling on --enable-release).

Comment 5

[(Away)]

For my work I often care about ICF results, but I recognize that I am in the minority here. (I suspect I may be in the minority just for building opt at all; most people I talk to seem to stick to dbg.)

Comment 6

[Masatoshi Kimura [:emk]]

I'm building with MOZ_DEBUG_SYMBOLS enabled (because otherwise debugger cannot find symbols) but disabling DEBUG macros (because otherwise built binaries are toooooo slow and virtually unusable.) Does it count "opt builds"?

Comment 7

[Mike Hommey [:glandium]]

(In reply to Masatoshi Kimura [:emk] from comment #6)
> I'm building with MOZ_DEBUG_SYMBOLS enabled (because otherwise debugger
> cannot find symbols) but disabling DEBUG macros (because otherwise built
> binaries are toooooo slow and virtually unusable.) Does it count "opt
> builds"?

Depends what your mozconfig looks like to achieve it. That being said, what you're describing is also the default.

Comment 8

[Mike Hommey [:glandium]]

(and is an opt build)

Comment 9

[(Away)]

I've just filed bug 1346069 for a case where incremental linking ends up being much worse than full linking. Something to keep in mind.

Comment 11

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1341504 - Don't disable incremental linking when optimizations are turned off.

Incremental linking is automatically disabled when -OPT:REF or -OPT:ICF are used.
Seeing that we're disabling optimizations anyway, may as well not use those linker flags as they considerably slow down linking.

Review commit: https://reviewboard.mozilla.org/r/175634/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175634/

Comment 12

[(Away)]

Comment on attachment 8903868 [details]
Bug 1341504 - Don't disable incremental linking when optimizations are turned off.

https://reviewboard.mozilla.org/r/175634/#review181570

::: config/config.mk:183
(Diff revision 1)
> +
> +ifdef MOZ_OPTIMIZE
>  ifdef HAVE_64BIT_BUILD
> -OS_LDFLAGS = -DEBUG -OPT:REF,ICF
> +OS_LDFLAGS += -OPT:REF,ICF
>  else
> -OS_LDFLAGS = -DEBUG -OPT:REF
> +OS_LDFLAGS += -OPT:REF

If you don't mind a drive-by... we could remove the bitness ifdef and just use -OPT:REF,ICF everywhere (on 32-bit builds, -OPT:REF and -OPT:REF,ICF ought to be identical).

I've been meaning to do this for a long time but can't be bothered to land on its own, so I've been waiting for a related patch to sneak it into. :-)

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8903868 [details]
Bug 1341504 - Don't disable incremental linking when optimizations are turned off.

https://reviewboard.mozilla.org/r/175634/#review181570

> If you don't mind a drive-by... we could remove the bitness ifdef and just use -OPT:REF,ICF everywhere (on 32-bit builds, -OPT:REF and -OPT:REF,ICF ought to be identical).
> 
> I've been meaning to do this for a long time but can't be bothered to land on its own, so I've been waiting for a related patch to sneak it into. :-)

sure i can add that.

arent you concerned that the overhead of parsing of few more characters may take all the benefits away ?  :)

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8903868 [details]
Bug 1341504 - Don't disable incremental linking when optimizations are turned off.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175634/diff/1-2/

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8903868 [details]
Bug 1341504 - Don't disable incremental linking when optimizations are turned off.

https://reviewboard.mozilla.org/r/175634/#review183630

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/817a2c423412
Don't disable incremental linking when optimizations are turned off. r=glandium

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/817a2c423412

Comment 18

[Eric Rahm [:erahm]]

What happened to the discussion about doing this of opt (the default) developer builds as well?