734845 - xplat: add support for precompiled headers

Status: RESOLVED FIXED

(Tamarin Graveyard :: Build Config, defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

Xcode build times are between 50% and 66% faster than xplat builds (on fklockii-iMac, at least).  (The 2x speedup was for a Release build, 3x speedup for a Debug build.)

One reason (among many real or potential reasons) for this difference in compile times is that Xcode is using precompiled headers, and xplat is not.

gcc supports precompiled headers, though, so it seems like we should be able to add support for them to xplat.

I hacked up something last Friday that seemed promising, and revised it a little bit today.  It makes xplat builds from scratch take about 2/3's as much time (i.e. 33% speed up), in both Release and Debug.  (So it doesn't buy back the full amount that Xcode is winning by, but its nothing to sneeze at.)

Since the bulk of any benefit of this is likely to be for engineers who are doing development (and not e.g. for the avmshell sandbox system where the bulk of the time is spent in running tests), it probably would be worthwhile to double-check the speed-up for the more common cases of just editting a single .cpp file and/or a header file, rather than comparing based on the worst-case scenario of a full build from scratch.

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch P: semi-precompiled header support for xplat

This patch was developed atop attachment 604872 [details] [diff] [review] for Bug 615532.  It won't apply cleanly without first putting that patch on.

There are some nasty little gotcha's here (it wasn't as easy a task as I had hoped).

One of the biggest problems: gcc won't let you subvert the include search path for user-header-includes (i.e. ``#include "foo.h"'') to force it to search some other directory *before* the directory that the original source file falls into.  * My inference is that the gcc developers either intend for you to only use PCH with system-header-includes (i.e. ``#include <foo.h>''), or intend for you to put PCH files into the source tree itself (so that ``#include "foo.h"'', when it looks in the current directory, will see and pick up a locally written "foo.h.gch").

* Neither of the above scenarios really suit our situation, where we would want to put build products like precompiled headers into the chosen objdir directory.

* There's the obvious option of rewriting our code so that we would always say ``#include <avmplus.h>'' instead of ``#include "avmplus.h"'', but I found that very distasteful.

* After much floundering trying to find some way to force gcc to do what I want here, I found a work-around documented here:
  http://stackoverflow.com/a/3164874/36585

That is essentially the strategy I have adopted, generating the following three precompiled headers:
  <objs>/core/avmplus-precompiled.h.gch
  <objs>/MMgc/MMgc-precompiled.h.gch
  <objs>/vmbase/vmbase-precompiled.h.gch
and then explicitly including them in the command lines of the compiler invocations.

Anyway, its a start.  I do not yet know enough about gcc's PCH infrastructure to know if we can easily generalize this to precompile other headers like the exactgc tracer headers and so on.

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #1)
> Created attachment 604896 [details] [diff] [review]
> patch P: semi-precompiled header support for xplat
> 
> [...]
> 
> Anyway, its a start.

And certainly not a finish; I've only tested when running in "make -j1" mode and various settings for the VERBOSE flag.  It breaks immediately in "make -j4" mode because I do not have the exactgc tracer dependencies set up correctly.

(VM engineers in Xcode need to be running the tracer dependency script manually anyway; for the VM team, this issue is unique to xplat where I have attempted to get xplat to drive all of the code-generation if it can.)

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch P v2: semi-precompiled header support for xplat

Fixed dependencies so that it seems to work with '-j4' (and thus hopefully '-jN' in general).

Removed precompiled headers for C source since I do not think we actually use any of the three for our C source files.

----

Something I should have noted in earlier comments on this technique: while this code is just taking the "main.h" header and turning it into the "main-precompiled.h.gch" header, we could be a little more intelligent here and make a distinct precompiled header that just includes all of the headers of interest.  This might even be a way to ease precompilation of e.g. the exact-tracer headers (which, importantly, are not overwritten during the unconditional generation unless they contain some diff from the previous version).  (The reason I am only generating a single "main-precompiled.h.gch" header is to deal with the issue discussed in the stackoverflow link from comment 1.)

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch P v3: semi-precompiled header support for xplat

Previous version was failing to build up a .deps file for the precompiled headers and subsequently was always rebuilding everything, which is not a good way to attempt to speed up the build process.

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

This still isn't perfect.

In particular I've noted the following when operating with MQ patch queues:
  cc1plus: error: ./core/avmplus-precompiled.h.gch: not used because `HGVERSION' defined as ` 89:43f6814fd5a9' not ` 89:8753d8426b1b'
cc1plus: error: core/avmplus-precompiled.h: No such file or directory
cc1plus: error: one or more PCH files were found, but they were invalid

which then dies.

The error is because the tip of my patch queue changed, and so the HGVERSION setting changes to a different hash.  So the preprocessor-symbols changed their values, and gcc complains that the prior compiled header is no good.

A more robust approach would be for the system to throw away the old precompiled header when it is invalid and start fresh with a new one.  (The calcdepends infrastructure should be getting us most of the way there on this front.)

Anyway the fix here is to manually delete the precompiled header so that it gets regenerated.

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch P v4: semi-precompiled header support with built-in sanity prepass.

This version addresses the problem discussed in comment 5 by adding an order-dependency on a sanity-checking prepass that compiles an empty file (and forcing the pch to be forcibly included), and checks that gcc is okay with this.  If gcc returns an error code, it assumes that this was because of some change in the context that requires the pch to be regenerated, and thus it does so.

This version also separately defines the source for the PCH (aka $(thing)_PCH_SRC) from the PCH itself ($(thing_PCH)); its something that one can infer from the filename with the current settings, but such inference ugly, confusing, and really unnecessary since it is natural to specify what the source header is to be precompiled.

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch P v5: precompiled header support with built-in sanity prepass and Makefile guard.

(as defined by configure.py, the Makefile guard (USE_GCC_PRECOMPILED_HEADERS) predefined to on (1); but in the absence of a definition of the guard, it is treated as off by the xplat infrastructure.)

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch P v6: precompiled header support

Patch includes:
* built-in sanity prepass
* Makefile guard (and uses it to toggle off precompilation on
  windows, where we do not use GCC).
* drive-by fix to definition of GARBAGE (it wasn't cleaning up I_SUFFIX files)
* fixes to the rules that properly use the THING_PCH and THING_PCH_SRC definitions
  (which we need corrected to accomodate halfmoon's 'hm-main.h' header file).
* a teensy bit of cleanup (getting rid of the conditional portion that looks if
  Make symbol is set to 0; it just as well to leave the right-hand side of the
  definition empty, just like is done for e.g. MMGC_DYNAMIC in the default config.

Comment 9

[Tamarin Bot]

changeset: 7345:f69b176692ce
user:      Felix Klock II <[e-mail]>
summary:   Bug 734845: semi-precompiled header support (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/f69b176692ce

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch Q v1: dont emit nonsense rules

Comment 11

[Tamarin Bot]

changeset: 7361:fb04d2b34bcc
user:      Felix Klock II <[e-mail]>
summary:   Bug 734845: [xplat] do not emit nonsensical PCH rules (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/fb04d2b34bcc

Comment 12

[Tamarin Bot]

changeset: 7364:88df94fad631
user:      Felix Klock II <[e-mail]>
summary:   Bug 734845: [xplat] tell us explicitly when pch is regenerated (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/88df94fad631