enable incremental linking by default when using ninja on Windows

enable incremental linking by default when using ninja on Windows

BUG=125748
TEST=ninja windows links


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151930

[Nico, 2012-05-03 22:41:38]

As far as I understand from previous patches, incremental linking requires all
.obj files instead of a .lib file being passed on the link line.

At least on the toolchains I know (i.e. not windows :-P), this has slightly
different semantics: passing a .lib means that .obj files in there are loaded on
demand, and .obj files that only contain unreferenced symbols aren't loaded.
Passing all .obj files on the other hand means all .obj files get loaded, which
can have different results e.g. if one of the .obj files that would otherwise
not be loaded references a symbol that doesn't exist.

Past experience suggests that this is how the msvc linker works, too. Do you
know if that's true? If so, that means incremental linking can fail when regular
linking works, correct?


(mostly unrelated ps: My local winja build fails to link initial\chrome.dll atm,
with or without this flag (non-components build)...do you see this too? do you
want me to file a bug? only mostly unrelated because the linker error is
different with and without the incremental linking flag.)

[scottmg, 2012-05-03 23:03:03]

On Thu, May 3, 2012 at 3:41 PM,  <thakis@chromium.org> wrote:
> As far as I understand from previous patches, incremental linking requires
> all
> .obj files instead of a .lib file being passed on the link line.
>
> At least on the toolchains I know (i.e. not windows :-P), this has slightly
> different semantics: passing a .lib means that .obj files in there are
> loaded on
> demand, and .obj files that only contain unreferenced symbols aren't loaded.
> Passing all .obj files on the other hand means all .obj files get loaded,
> which
> can have different results e.g. if one of the .obj files that would
> otherwise
> not be loaded references a symbol that doesn't exist.
>
> Past experience suggests that this is how the msvc linker works, too. Do you
> know if that's true? If so, that means incremental linking can fail when
> regular
> linking works, correct?

Yes, that's all accurate.

I think it's still sensible to have it on by default since a) it's
Debug only, and b) if it breaks, realistically it has to get fixed
anyway for anyone to be halfway productive on Windows.

At least for the cases I changed originally, they were unintended
static dependencies that were "happening" to work, so refactoring was
generally a net improvement. That might not always be true I guess.

Setting incremental_chrome_dll=0 explicitly is still available as an
escape mechanism of course.

> (mostly unrelated ps: My local winja build fails to link initial\chrome.dll
> atm,
> with or without this flag (non-components build)...do you see this too? do
> you
> want me to file a bug? only mostly unrelated because the linker error is
> different with and without the incremental linking flag.)

Hmm, I haven't been testing non-components, so it's probably just
borked. I'll have a look.

>
> https://chromiumcodereview.appspot.com/10366002/

[Nico, 2012-05-03 23:24:37]

> Yes, that's all accurate.
> 
> I think it's still sensible to have it on by default since a) it's
> Debug only, and b) if it breaks, realistically it has to get fixed
> anyway for anyone to be halfway productive on Windows.
> 
> At least for the cases I changed originally, they were unintended
> static dependencies that were "happening" to work, so refactoring was
> generally a net improvement. That might not always be true I guess.
> 
> Setting incremental_chrome_dll=0 explicitly is still available as an
> escape mechanism of course.

I'm mostly worried about configuration matrix blowup. Since it breaks msvc, we
can't turn on incremental for everyone, so this is a strict increase in
complexity.

I'm personally happy with this change, and I tried to do something similar on
mac (fake thin archives) that sped up incremental builds by some significant
number (20s -> 12s or something like that), and the mac tech lead shot it down
due to this complexity increase. (That was admittedly before we had component
builds, and the component build shares many of the stuff exposed by that, so
it's less of a risk today then it was back then.)

Long story short, maybe some windows build person should approve this (not sure
who. One of darin / jam / nsylvain probably). I think it's a good change.

[scottmg, 2012-08-15 21:06:12]

Hey Nico, John was harassing me to do this by default to avoid extra config
settings for ninja-using Windows devs.

Seems reasonable since it only affects the unsupported-for-now-ninja config
anyway?

(Plus, Evan's been working hard at rewriting winja.. Bot-ready soon, hopefully!)

[Nico, 2012-08-15 21:10:23]

On 2012/08/15 21:06:12, scottmg wrote:
> Hey Nico, John was harassing me to do this by default to avoid extra config
> settings for ninja-using Windows devs.

John was also harassing me on gtalk about this :-) He seemed to lose interest
after I pointed out that this changes linking semantics.

(As I said in my last comment, I think this is a good change, but someone
windowsy (darin / jam / nsylvain) should Igtm for now.)

> Seems reasonable since it only affects the unsupported-for-now-ninja config
> anyway?
> 
> (Plus, Evan's been working hard at rewriting winja.. Bot-ready soon,
hopefully!)

[scottmg, 2012-08-15 21:12:37]

On 2012/08/15 21:10:23, Nico wrote:
> On 2012/08/15 21:06:12, scottmg wrote:
> > Hey Nico, John was harassing me to do this by default to avoid extra config
> > settings for ninja-using Windows devs.
> 
> John was also harassing me on gtalk about this :-) He seemed to lose interest
> after I pointed out that this changes linking semantics.

Maybe we (you) could re-pursue thin archives so the semantics are different
everywhere! ;-)

[jam, 2012-08-15 21:28:17]

lgtm

I might not have been clear; I'm not familiar with Mac to know how much of a
hack/maintenance burden it is to do this. On Windows, incremental linking is on
by default in VS so I don't think there's nothing dangerous here. Also, given
that most devs are using this, it would be supporting yet another config if devs
use it and bots (eventually when they switch to ninja) don't.

[commit-bot: I haz the power, 2012-08-16 16:44:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/10366002/8002

[commit-bot: I haz the power, 2012-08-16 18:57:12]

Change committed as 151930