Intent to implement: Remove windows.h from Chrome precompiled headers

[Brett Wilson]

Our precompiled headers were set up many years ago when Chrome was Windows-only. We routinely used Windows headers.

Now, relatively few source files include the windows headers, so the benefit is reduced, Furthermore, the addition of windows.h makes the precompiled headers unnecessarily larger and slower for cases where windows headers are not used.

I plan on removing the 4 includes for Windows headers and just leave the C/C++ standard library stuff. The file is build/precompile.h if you're interested. This will not affect the Blink precompiled headers which are separate.

There are some advantages to removing it. There will potentially be fewer places where we'll have #define conflicts. For example, we have to #undef SendMessage in a few places, and people sometimes run into problems with min/max. This happens because the precompiled header is force-included into every source file that uses precompiled headers, even if it doesn't depend on Windows.

This change will allow us to use precompiled headers in more places. Currently there are a number of third-party libraries that have typedefs or #defines for things like "UINT32" and "boolean" that conflict with windows.h. These third party libraries, and, more importantly, any code that includes them, can't be compiled with precompiled headers.

In the future, if we really want Windows PCH, we can make a different Windows-including header and use it. But given the timings below this doesn't seem necessary.

Timings (on a Z620, full symbols, non-component, debug build) and some build sizes:

GN 64-bit current file:   41:57

GN 64-bit no windows.h:  42:04

GYP 32-bit current file:  40:30, 28,647 MB

GYP 32-bit no windows.h:  41:44, 28,030 MB

I only did one build of each configuration so the error bars are pretty high. The GYP build has a slight time regression which I don't consider large enough to worry about, and a slight improvement in disk usage. The GN build doesn't have any precompiled headers set up for Blink yet which makes it slower, but it also lacks NaCl which mostly balances out.

Please let me know if you have any concerns or objections.

Brett

[Bruce]

When doing the build performance tests did you make use of WIN32_LEAN_AND_MEAN or similar defines to trim the number of header files pulled in? Using that could help reduce the regression. It doesn't look like this #define has been used in the past.

No concerns or objections. I found it disturbing when I realized that <Windows.h> was included in some non-Windows C++ files.

[Brett Wilson]

WIN32_LEAN_AND_MEAN is defined globally on Windows builds in common.gypi.

Brett

[Nico Weber]

Sounds fine to me.

Did you check incremental build times when hacking on chrome ui files? It'd expect the impact to be most noticeable there.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Marijn Kruisselbrink]

On Fri, Jul 24, 2015 at 2:00 PM, Brett Wilson <bre...@chromium.org> wrote:

Our precompiled headers were set up many years ago when Chrome was Windows-only. We routinely used Windows headers.

Now, relatively few source files include the windows headers, so the benefit is reduced, Furthermore, the addition of windows.h makes the precompiled headers unnecessarily larger and slower for cases where windows headers are not used.

It probably doesn't matter, but I'm curious how you determined "relatively few"? Some pretty low level base/ header files indirectly still include windows.h, so some very non-scientific grepping shows me that at least almost half of the .cc files in content/ and chrome/ still include windows.h. Specifically atomics on windows still rely on windows.h, and atomics are used indirectly by things like scoped_refptr, base::Bind etc.

Are there any plans for these low-level headers to not actually include windows.h? I'd love to be able to remove some of the #undef SendMessage and #undef PostMessage, but instead with now only roughly half of the codebase including windows.h I find myself having to add about a dozen more of these undefines...

I plan on removing the 4 includes for Windows headers and just leave the C/C++ standard library stuff. The file is build/precompile.h if you're interested. This will not affect the Blink precompiled headers which are separate.

There are some advantages to removing it. There will potentially be fewer places where we'll have #define conflicts. For example, we have to #undef SendMessage in a few places, and people sometimes run into problems with min/max. This happens because the precompiled header is force-included into every source file that uses precompiled headers, even if it doesn't depend on Windows.

This change will allow us to use precompiled headers in more places. Currently there are a number of third-party libraries that have typedefs or #defines for things like "UINT32" and "boolean" that conflict with windows.h. These third party libraries, and, more importantly, any code that includes them, can't be compiled with precompiled headers.

In the future, if we really want Windows PCH, we can make a different Windows-including header and use it. But given the timings below this doesn't seem necessary.

Timings (on a Z620, full symbols, non-component, debug build) and some build sizes:

GN 64-bit current file:   41:57

GN 64-bit no windows.h:  42:04

GYP 32-bit current file:  40:30, 28,647 MB

GYP 32-bit no windows.h:  41:44, 28,030 MB

I only did one build of each configuration so the error bars are pretty high. The GYP build has a slight time regression which I don't consider large enough to worry about, and a slight improvement in disk usage. The GN build doesn't have any precompiled headers set up for Blink yet which makes it slower, but it also lacks NaCl which mostly balances out.

Please let me know if you have any concerns or objections.

Brett

--

[Brett Wilson]

On Fri, Aug 7, 2015 at 4:35 PM, Marijn Kruisselbrink <m...@chromium.org> wrote:

On Fri, Jul 24, 2015 at 2:00 PM, Brett Wilson <bre...@chromium.org> wrote:

Our precompiled headers were set up many years ago when Chrome was Windows-only. We routinely used Windows headers.

Now, relatively few source files include the windows headers, so the benefit is reduced, Furthermore, the addition of windows.h makes the precompiled headers unnecessarily larger and slower for cases where windows headers are not used.

It probably doesn't matter, but I'm curious how you determined "relatively few"? Some pretty low level base/ header files indirectly still include windows.h, so some very non-scientific grepping shows me that at least almost half of the .cc files in content/ and chrome/ still include windows.h. Specifically atomics on windows still rely on windows.h, and atomics are used indirectly by things like scoped_refptr, base::Bind etc.

I just poked around. My standard may be off because we used to use it in a LOT more places.

 

Are there any plans for these low-level headers to not actually include windows.h? I'd love to be able to remove some of the #undef SendMessage and #undef PostMessage, but instead with now only roughly half of the codebase including windows.h I find myself having to add about a dozen more of these undefines...

There are no plans. If there are clean ways to reduce this then I support patches that do this (obviously this would very low priority).

Brett

[Marijn Kruisselbrink]

On Fri, Aug 7, 2015 at 4:46 PM, Brett Wilson <bre...@chromium.org> wrote:

On Fri, Aug 7, 2015 at 4:35 PM, Marijn Kruisselbrink <m...@chromium.org> wrote:

On Fri, Jul 24, 2015 at 2:00 PM, Brett Wilson <bre...@chromium.org> wrote:

Our precompiled headers were set up many years ago when Chrome was Windows-only. We routinely used Windows headers.

Now, relatively few source files include the windows headers, so the benefit is reduced, Furthermore, the addition of windows.h makes the precompiled headers unnecessarily larger and slower for cases where windows headers are not used.

It probably doesn't matter, but I'm curious how you determined "relatively few"? Some pretty low level base/ header files indirectly still include windows.h, so some very non-scientific grepping shows me that at least almost half of the .cc files in content/ and chrome/ still include windows.h. Specifically atomics on windows still rely on windows.h, and atomics are used indirectly by things like scoped_refptr, base::Bind etc.

I just poked around. My standard may be off because we used to use it in a LOT more places.

Ah, okay. I still wouldn't exactly consider the majority (or at least close to that; my quick poking around showed about half of the codebase depending on something that pulls in windows.h) to be "relatively few", but of course it's a lot better than what it might have been in the past. 

 

Are there any plans for these low-level headers to not actually include windows.h? I'd love to be able to remove some of the #undef SendMessage and #undef PostMessage, but instead with now only roughly half of the codebase including windows.h I find myself having to add about a dozen more of these undefines...

There are no plans. If there are clean ways to reduce this then I support patches that do this (obviously this would very low priority).

Is there any reason atomics on windows don't use c++11 <atomic> while atomics on other platforms do? From a quick search MSVC 2013 seems to claim to provide support for <atomic>. It seems that just that change by itself would already be able to cut out a very large portion of windows.h dependencies.