[PATCH] D20136: Get default -fms-compatibility-version from cl.exe's version

Wed May 11 14:17:04 PDT 2016

amccarth added inline comments.

================
Comment at: lib/Driver/MSVCToolChain.cpp:478
@@ +477,3 @@
+
+  const DWORD VersionSize = ::GetFileVersionInfoSizeW(ClExeWide.c_str(),
+                                                      nullptr);
----------------
amccarth wrote:
> thakis wrote:
> > amccarth wrote:
> > > amccarth wrote:
> > > > thakis wrote:
> > > > > amccarth wrote:
> > > > > > Yes, it looks in the executable (which I tried to emphasize with the method name).
> > > > > > 
> > > > > > I don't think this is very expensive given that Explorer often makes zillions of such calls, but I'm open to other suggestions.
> > > > > > 
> > > > > > I know that you can't use a library that's newer than the compiler (because it may use new language features), but I don't know if that applies in the other direction or how we would safely and reliably map directory names to library versions and therefore to compiler versions.
> > > > > I agree that figuring out the right value for fmsc-version automatically somehow is definitely something we should do.
> > > > > 
> > > > > I forgot that `getVisualStudioBinariesFolder` already works by looking for cl.exe in PATH, so cl.exe's metadata is already warmed up in the disk cache. However, GetFileVersionInfoW() probably opens cl.exe itself and does some PE parsing to get at the version, and that probably is in cold cache territory. (https://msdn.microsoft.com/en-us/library/windows/desktop/ms647003(v=vs.85).aspx suggests that this function might open several files).
> > > > > 
> > > > > `getVisualStudioBinariesFolder` checks:
> > > > > 
> > > > > 1. getenv("VCINSTALLDIR")
> > > > > 2. cl.exe in getenv("PATH")
> > > > > 3. registry (via getVisualStudioInstallDir)
> > > > > 
> > > > > The common cases are 1 and 3. For 1, for default installs, the version number is part of the directory name (for default installs, what most people have). For 3, the version number is in the registry key we query. So in most cases we shouldn't have to look at cl.exe itself. And for the cases where we would have to look, maybe it's ok to require an explicit fmsc-version flag.
> > > > The version number in the directory name and the registry is the version number of Visual Studio not of the compiler.  Yes, we could do a mapping (VS 14 comes bundled with CL 19), assuming Microsoft continues to keep VS releases and compiler releases in sync, and it means this code will forever need updates to the mapping data.
> > > > 
> > > > The mapping would give just the major version number, which might be all that matters now, but if there's ever a CL 19.1 that has different compatibility requirements (and is maybe released out-of-band with Visual Studio), we'd be stuck.
> > > > 
> > > > Getting the actual version from the compiler seems the most accurate and future-proof way to check.  If that's too expensive, then maybe we should abandon the idea of detecting the default for compatibility.
> > > I'll do some research to figure out the actual costs.  I suspect that walking the PATH for the executable may be far more expensive, but I'll get some numbers and report back.
> > Compilers being released independently of VC versions and fractional compat numbers sounds like things we can worry about when they happen (probably not soon, right?).
> > 
> > We already walk PATH, so that wouldn't be an additional cost.
> > 
> > Be sure to measure cold disk cache perf impact (which is tricky on Windows since there's as far as I know no way to tell the OS to drop its caches). As far as I know file metadata is stored with the directory node on NTFS, so stating files doesn't warm up file content accesses.
> > Compilers being released independently of VC versions and fractional compat numbers sounds like things we can worry about when they happen (probably not soon, right?).
> 
> It already happens.  Herb Sutter talks about it in one of his blogs:  "Soon after VC++11 ships we have announced we will do out-of-band releases for additional C++11 conformance which will naturally also include more C11 features that are in the C subset of C++11."  In this case, it's just the build number (of major.minor.build) that's updating, but it's for increasing conformance, which is exactly a compatibility issue.
> 
> > We already walk PATH, so that wouldn't be an additional cost.
> 
> I suspect we may be walking it more than once, which can be expensive even if the cache is all warmed up.  This is one of the things I'm checking.  If it's a problem, I'll propose a patch to cache the result from the first walk.
> 
> > stating files doesn't warm up file content accesses.
> 
> That is correct.
My machine in Windows 7 Enterprise, with cl.exe on a spinning HD (not SSD), and with Bit9 in "monitor" mode, which drags down all i/o operations.

First, I used procmon to make sure there were no surprising file ops happening as a result of my change.  As expected, it creates the file handle, maps the file, and performs three read non-contiguous operations, loading a total of 5 pages (1 page = 4096 bytes).  I'm guessing that the three reads correspond to the file header, a resource index, and the actual version resource.  In the cold cache case, the file operations themselves (as reported by procmon) totaled 17 ms.

Subsequent calls from the same process did not even repeat the reads.  It seems the system might cache the resource information once loaded.

Procmon also highlighted that the path search is doing twice as many checks as necessary.  For each segment it checks for both cl.exe and cl.exe.exe.  The path searching (according to procmon) took a little more than 1 ms per walk, and, with this change, it happens three times, for a total of almost 4 ms.  This could be eliminated by caching the result in the MSVCToolChain object so subsequent calls are essentially free and/or reduced by eliminating the spurious checks for cl.exe.exe.

With release builds of Clang (with and without my change) and procmon turned off, I built a trivial C++ program multiple times.  These are effectively warm-cache results, which I think are appropriate, since you're either building one-off interactively (in which case 20 ms isn't a big deal) or you're building a lot, in which case the cache will likely be warm for all but the first invocation.

W/o this change:  1.426 s average (1.421, 1.431, 1.458, 1.408, 1.411)
With this change:  1.442 s average (1.467, 1.442, 1.417, 1.433, 1.453)

So the delta is 16 ms, some of which is the extra path walking, and is on the order of the run-to-run variance.

http://reviews.llvm.org/D20136