Do we still need "-t msvc"?

[Shezan Baig]

Hi folks,

I took my first dive into the ninja code yesterday :) and started by looking at the "-t msvc" stuff.  From what I can tell, what this does is simply load an env-file and invoke cl.  There's code in there to optionally parse the includes, but we don't use it anymore, in favor of "deps=msvc" in the main ninja process.  So I was thinking the env-file stuff can probably be generalized into Subprocess, instead of being msvc-specific, and we can eventually remove "-t msvc".

I have a work-in-progress patch here [1].  It shaves off about 6K from the ninja binary (using LTCG), while also teaching ninja how to load custom env-files for any Subprocess (not just msvc).  Note that this currently only works on Windows, but I can implement it for other platforms too.

Does this sound like something that can be merged upstream?  (Assuming things like gyp etc are first updated to move off "-t msvc").  Or is there something about "-t msvc" that is vital that I am missing?

Thanks!

-shez-

[1] https://github.com/bulldy80/ninja/compare/remove_t_msvc

[Scott Graham]

Yeah, it's only hanging around to pass environments in an overly complicated way. :)

I support getting rid of -t msvc, but I think gyp might need some work first. (I doubt it affects other generators, but hard to know I guess.)

That patch changes behaviour slightly. CreateProcess will search for the named binary in the PATH of the parent environment, not the passed environment block. The reinvocation of ninja with -t msvc injects the envblock into its own environment (see PushPathIntoEnvironment) in addition to passing it to the child. I did some work in gyp on making sure that tools are always specified with absolute paths, but I don't think it's complete.

(We did also discuss passing the the environment in different ways last time around, e.g. embedding as an escaped string into the .ninja file to avoid extra IO, but envfile= seems fine to me.)

--
You received this message because you are subscribed to the Google Groups "ninja-build" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ninja-build...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Shezan Baig]

Yeah, I did see PushPathToEnvironment, but didn't port it over because I found that it didn't actually work :)

(Or rather, it only works if PATH is the first environment variable in the env block, because it is using 'c_str()' instead of 'data()', and since each env var is null-terminated, this means it is only ever looking at the first env var).

I figured the reason gyp specified absolute paths was to work around that issue :)  In any case, I think it is better for the generator to provide the absolute path so we only look it up at generation time, rather than making ninja do it at build time.

[Scott Graham]

On Sat, Jul 19, 2014 at 12:15 PM, Shezan Baig <shezb...@gmail.com> wrote:

Yeah, I did see PushPathToEnvironment, but didn't port it over because I found that it didn't actually work :)

(Or rather, it only works if PATH is the first environment variable in the env block, because it is using 'c_str()' instead of 'data()', and since each env var is null-terminated, this means it is only ever looking at the first env var).

Oh! Great, maybe gyp's fine then.

 

I figured the reason gyp specified absolute paths was to work around that issue :)

Yeah. I did fix the main invocations for cc/cxx. One other case was the clang c99 conversion, but that's gone now. One other case I was thinking of was when the user subs in "gomacc cl.exe" or something as the compiler, something needs to make sure it's absoluted, but I agree that's the generators problem.

 

In any case, I think it is better for the generator to provide the absolute path so we only look it up at generation time, rather than making ninja do it at build time.

Agreed.

 

(I guess my only request then for your patch would be to not remove -t msvc for some time. Last time we changed the ninja arguments backwards-incompatibly it was pretty painful for some months as the ninja binary isn't versioned with gyp, so every time someone tried to build old chrome revisions they got messed up.)

[Shezan Baig]

On Sat, Jul 19, 2014 at 3:32 PM, Scott Graham <sco...@chromium.org> wrote:

On Sat, Jul 19, 2014 at 12:15 PM, Shezan Baig <shezb...@gmail.com> wrote: 

In any case, I think it is better for the generator to provide the absolute path so we only look it up at generation time, rather than making ninja do it at build time.

Agreed.

 

(I guess my only request then for your patch would be to not remove -t msvc for some time. Last time we changed the ninja arguments backwards-incompatibly it was pretty painful for some months as the ninja binary isn't versioned with gyp, so every time someone tried to build old chrome revisions they got messed up.)

Sounds good :)  I was planning on that actually (that's why I have the removal in a separate commit).

Thanks!
-shez-



 

[Nico Weber]

On Sat, Jul 19, 2014 at 11:41 AM, Shezan Baig <shezb...@gmail.com> wrote:

Hi folks,

I took my first dive into the ninja code yesterday :) and started by looking at the "-t msvc" stuff.  From what I can tell, what this does is simply load an env-file and invoke cl.  There's code in there to optionally parse the includes, but we don't use it anymore,

"we" being gyp I suppose? There could be other generators that use it. (I don't know of any, though.)

 

in favor of "deps=msvc" in the main ninja process.

About deps=msvc: I kind of think that having this might have been a mistake. I think it's better to assume that compilers support writing make-style depfiles and have small wrappers around the ones that don't (like cl.exe) – like ninja -t msvc -o, but these wrappers shouldn't be part of ninja. See the discussion on https://github.com/martine/ninja/pull/721 for the reasoning.

So if we're making backward-incompatible changes, I'd would:

* Remove deps=msvc

* Default to deps=gcc if depfile is set (this one isn't backward-incompatible; maybe we can do this for 1.6?)

 

So I was thinking the env-file stuff can probably be generalized into Subprocess, instead of being msvc-specific, and we can eventually remove "-t msvc".

I have a work-in-progress patch here [1].  It shaves off about 6K from the ninja binary (using LTCG), while also teaching ninja how to load custom env-files for any Subprocess (not just msvc).  Note that this currently only works on Windows, but I can implement it for other platforms too.

We've talked about adding env support to ninja a few times and never did it, which suggests that maybe it isn't really needed. Adding it needs careful thought about semantics: Do targets get rebuild if the envfile is touched? Yes, probably? Should they then also be rebuilt if the env vars that are set when ninja is started change? Both "yes" and "no" seem like suboptimal answers. If env handling is punted to generators like it currently mostly is, this seems to have more predictable behavior.

(If you have data that shows that having an extra process to set up the environment has a measurable cost, that'd be a good argument for supporting env stuff directly.)

I do agree with the larger point that it'd be nice to remove -t msvc. But it'd probably mean that generators would have to grow something similar and then use that instead of -t msvc.

Nico

[Shezan Baig]

Hmm, no, I don't really have any data on this.  When I was playing with my patch earlier, I wasn't able to find any real performance difference with not having the extra process.  I was looking at it more from a "fewer moving parts is better for the whole system" point-of-view :)

But I agree with your comments about deps=msvc though, I think having a common format would be better.  And this obviously necessitates having an external tool for msvc, so we may as well put the env stuff there as well.

At this point, I'm ready to abandon my branch, but I think working through that has given me a good insight into the code overall, so I'll probably start looking at some of the issues on github to see if I can tackle any of them (at least the simple ones :) ).

Thanks!
-shez-

[Richard O'Grady]

We used to run scripts to convert our various compilers' deps files from their "GCC-esque" format to something ninja would accept.  It was not that great. Certainly launching 8000+ extra processes on Cygwin is not totally zero impact, but more importantly it caused various headaches with our distributed build system. To fix that I've ended up adding a few more deps styles to my ninja fork, to convert the incoming deps to standard gcc format. I'd say it's < 100 LOC and to me it feels much cleaner. Though I do agree it's not ideal to have ninja explode to accept any kind of format.  Just my 2c.

[Neil Mitchell]

The cost of any process (even something trivial) in Windows can be up
to 0.3s, particularly on corporate machines with aggressive virus
scanners. I have managed to optimise some operations from over a
minute to 3s just by eliminating some process creation.

[Scott Graham]

That was my initial thought too, but we should be able to come up with some concrete numbers based on Shez's patch to decide now.

[Shezan Baig]

Yeah, feel free to try the patch to see if you can get any improvements.

In my tests, I was using a small-ish project (about 1300 cl invocations, taking about 4.5 mins for a clean build).  I wasn't seeing any conclusive improvements, the build times were fluctuating by 10-20 seconds, even though I had nothing else running on the machine (virus scanners etc were turned off).  This was regardless of whether I had the patch applied or not.  So this indicated to me that the process creation was not a significant part of the entire build process.  Things did look a lot cleaner in Process Explorer though, which I seem to like for some reason :)

You might notice a difference if you're  building chromium, I didn't try with that.  Please let me know the results if you try it.  Also, I was using msys, not cygwin, if that makes a difference (I'm not sure though).