patchMakefile: Use computed header dependencies if the compiler supports it

Previously you had to manually define COMPUTE_HEADER_DEPENDENCIES to enable this feature. It seemed a bit sad that such a useful feature had to be enabled manually.

Signed-off-by: Fredrik Kuivinen <frekui@gmail.com>
---

This is a resend, it has been rebased on top of master but otherwise the same patch was sent 2011-06-11. Jonathan Nieder has been added to the Cc list as he implemented the computed header dependencies feature.

 Makefile |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

Hi,

Fredrik Kuivinen wrote:

Yes!  Thanks for this.

I have a few thoughts about the implementation:

[...]

This causes "make foo" to run gcc and create a temporary file unconditionally, regardless of what foo is. In an ideal world:

 - the autodetection would only happen when building targets that
   care about it

 - the detection would happen once (creating some file to store the
   result) and not be repeated with each invocation of "make"

 - (maybe) there would be a way to override the detection with
   either a "yes" or "no" result, for those who really care to
   save a little time.

I was about to say that the GIT_VERSION variable has some of these properties, but now that I check, from the point of view of the Makefile it doesn't. ./GIT-VERSION-GEN is just very fast. :)

I wonder if we can make do with a faster check, like

	$(CC) -c -MF /dev/null -MMD -MP git.c --help >/dev/null 2>&1

What do you think?

Hi Jonathan,

Thanks for your comments.

On Sun, Aug 14, 2011 at 21:00, Jonathan Nieder <jrnieder@gmail.com> wrote:

In an ideal world, yes. But I don't see a simple and maintainable way to do it for only those targets.

I have done some benchmarking (see below) and considering the results I got, I think implementing any of these two is not worth it.

Here are some benchmarks done with 'perf stat'. Before each invocation of perf stat I ran a plain 'make' to make sure that everything was compiled. Note that a fully compiled source tree is the worst case when it comes to the overhead of the auto-detection.

Without patch (with COMPUTE_HEADER_DEPENDENCIES=Yes):
 Performance counter stats for 'make' (10 runs):

         1,566,393 cache-misses             #      1.557 M/sec   ( +-   0.264% )
         6,428,212 cache-references         #      6.391 M/sec   ( +-   0.168% )
        21,245,775 branch-misses            #      4.626 %       ( +-   0.015% )
       459,268,954 branches                 #    456.585 M/sec   ( +-   0.031% )
     2,594,717,999 instructions             #      1.177 IPC     ( +-   0.022% )
     2,205,246,745 cycles                   #   2192.359 M/sec   ( +-   0.136% )
            43,532 page-faults              #      0.043 M/sec   ( +-   0.034% )
               215 CPU-migrations           #      0.000 M/sec   ( +-   0.891% )
               457 context-switches         #      0.000 M/sec   ( +-   0.654% )
       1005.878305 task-clock-msecs         #      1.022 CPUs    ( +-   0.544% )

        0.984526665  seconds time elapsed   ( +-   0.591% )

With patch:
 Performance counter stats for 'make' (10 runs):

         1,796,342 cache-misses             #      1.732 M/sec   ( +-   0.702% )
         6,929,739 cache-references         #      6.682 M/sec   ( +-   0.186% )
        21,582,772 branch-misses            #      4.575 %       ( +-   0.032% )
       471,783,920 branches                 #    454.934 M/sec   ( +-   0.024% )
     2,662,671,428 instructions             #      1.166 IPC     ( +-   0.017% )
     2,282,907,087 cycles                   #   2201.372 M/sec   ( +-   0.162% )
            49,244 page-faults              #      0.047 M/sec   ( +-   0.031% )
               233 CPU-migrations           #      0.000 M/sec   ( +-   0.823% )
               489 context-switches         #      0.000 M/sec   ( +-   0.460% )
       1037.038252 task-clock-msecs         #      1.022 CPUs    ( +-   0.579% )

        1.014409177  seconds time elapsed   ( +-   0.476% )

With patch, but changed to use git.c instead of ++empty.c:
 Performance counter stats for 'make' (10 runs):

         2,125,147 cache-misses             #      1.737 M/sec   ( +-   0.287% )
         9,080,043 cache-references         #      7.423 M/sec   ( +-   0.185% )
        24,573,023 branch-misses            #      4.222 %       ( +-   0.032% )
       582,018,515 branches                 #    475.809 M/sec   ( +-   0.012% )
     3,185,328,930 instructions             #      1.165 IPC     ( +-   0.009% )
     2,734,176,502 cycles                   #   2235.229 M/sec   ( +-   0.122% )
            51,032 page-faults              #      0.042 M/sec   ( +-   0.034% )
               227 CPU-migrations           #      0.000 M/sec   ( +-   0.943% )
               515 context-switches         #      0.000 M/sec   ( +-   0.351% )
       1223.219930 task-clock-msecs         #      1.019 CPUs    ( +-   0.579% )

        1.200869268  seconds time elapsed   ( +-   0.555% )

So, on my machine the auto-detection logic adds a slight overhead (0.03s, 3% in a fully compiled tree). Using git.c is slower than using ++empty.c. IMHO adding any extra complexity to lower the 0.03s is not worth it.

- Fredrik

Fredrik Kuivinen wrote:

[...]

The case to compare against is when COMPUTE_HEADER_DEPENDENCIES is not set, I'd think, since that is the status quo. And I was talking about commands like "make clean" that do not care about that feature, not "make all".

[...]

Did you try with "--help"?

On Sun, Aug 14, 2011 at 03:02:55PM -0500, Jonathan Nieder wrote:

There is no measurable difference between setting and unsetting COMPUTE_HEADER_DEPENDENCIES for me (not surprising as nothing is actually built). "make clean" (in a clean tree) takes more time than "make" (in a fully built tree), so the relative overhead in the make clean case is even smaller. The absolute overhead is, of course, the same.

Oh, I missed "--help". But for me gcc always exits with status code 0 when I give it "--help", regardless of what other flags I provide. Therefore, I don't see how "--help" can be used to test for support of -MMD.

Here is an updated patch. It avoids the ++empty.c file by giving "-x c" to the compiler. It also avoids the auto-detection when COMPUTE_HEADER_DEPENDENCIES is set, so if you want to avoid the overhead you can set that in you config.mak.

-- 8< --

Subject: [PATCH] Makefile: Use computed header dependencies if the compiler supports it

Previously you had to manually define COMPUTE_HEADER_DEPENDENCIES to enable this feature. It seemed a bit sad that such a useful feature had to be enabled manually.

To avoid the small overhead we don't do the auto-detection if COMPUTE_HEADER_DEPENDENCIES is already set.

Signed-off-by: Fredrik Kuivinen <frekui@gmail.com>
---
 Makefile |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

Fredrik Kuivinen wrote:

Ah, my mistake.  Good catch.

Much nicer, thanks!

Unfortunately "ifdef" in Makefiles means "if nonempty", so the overhead of detection is still there if I want to explicitly disable COMPUTE_HEADER_DEPENDENCIES. That's okay, since that overhead is small. So for what it's worth,

Acked-by: Jonathan Nieder <jrnieder@gmail.com>