Hi,
Fredrik Kuivinen wrote:
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.
Yes! Thanks for this.
I have a few thoughts about the implementation:
quoted hunk
--- a/Makefile
+++ b/Makefile
[...]
quoted hunk
@@ -1236,6 +1232,15 @@ endif
ifdef CHECK_HEADER_DEPENDENCIES
COMPUTE_HEADER_DEPENDENCIES =
USE_COMPUTED_HEADER_DEPENDENCIES =
+else
+dep_check = $(shell sh -c \
+ ': > ++empty.c; \
+ $(CC) -c -MF /dev/null -MMD -MP ++empty.c -o /dev/null 2>&1; \
+ echo $$?; \
+ $(RM) ++empty.c')
+ifeq ($(dep_check),0)
+COMPUTE_HEADER_DEPENDENCIES=YesPlease
+endif
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 [off-list ref] wrote:
quoted
--- a/Makefile
+++ b/Makefile
[...]
quoted
@@ -1236,6 +1232,15 @@ endif
ifdef CHECK_HEADER_DEPENDENCIES
COMPUTE_HEADER_DEPENDENCIES =
USE_COMPUTED_HEADER_DEPENDENCIES =
+else
+dep_check = $(shell sh -c \
+ ': > ++empty.c; \
+ $(CC) -c -MF /dev/null -MMD -MP ++empty.c -o /dev/null 2>&1; \
+ echo $$?; \
+ $(RM) ++empty.c')
+ifeq ($(dep_check),0)
+COMPUTE_HEADER_DEPENDENCIES=YesPlease
+endif
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
In an ideal world, yes. But I don't see a simple and maintainable
way to do it for only those targets.
- 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 have done some benchmarking (see below) and considering the
results I got, I think implementing any of these two is not worth it.
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?
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:
On Sun, Aug 14, 2011 at 21:00, Jonathan Nieder [off-list ref] wrote:
quoted
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?
[...]
Without patch (with COMPUTE_HEADER_DEPENDENCIES=Yes):
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".
[...]
With patch, but changed to use git.c instead of ++empty.c:
Did you try with "--help"?
On Sun, Aug 14, 2011 at 03:02:55PM -0500, Jonathan Nieder wrote:
Fredrik Kuivinen wrote:
quoted
On Sun, Aug 14, 2011 at 21:00, Jonathan Nieder [off-list ref] wrote:
quoted
quoted
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?
[...]
quoted
Without patch (with COMPUTE_HEADER_DEPENDENCIES=Yes):
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".
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.
[...]
quoted
With patch, but changed to use git.c instead of ++empty.c:
Did you try with "--help"?
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 <redacted>
---
Makefile | 13 +++++++++----
1 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/Makefile b/Makefile
index 89cc624..c131439 100644
--- a/Makefile
+++ b/Makefile
@@ -250,10 +250,6 @@ all::
# DEFAULT_EDITOR='$GIT_FALLBACK_EDITOR',
# DEFAULT_EDITOR='"C:\Program Files\Vim\gvim.exe" --nofork'
#
-# Define COMPUTE_HEADER_DEPENDENCIES if your compiler supports the -MMD option
-# and you want to avoid rebuilding objects when an unrelated header file
-# changes.
-#
# Define CHECK_HEADER_DEPENDENCIES to check for problems in the hard-coded
# dependency rules.
#
@@ -1236,6 +1232,15 @@ endif
ifdef CHECK_HEADER_DEPENDENCIES
COMPUTE_HEADER_DEPENDENCIES =
USE_COMPUTED_HEADER_DEPENDENCIES =
+else
+ifndef COMPUTE_HEADER_DEPENDENCIES
+dep_check = $(shell sh -c \
+ '$(CC) -c -MF /dev/null -MMD -MP -x c /dev/null -o /dev/null 2>&1; \
+ echo $$?')
+ifeq ($(dep_check),0)
+COMPUTE_HEADER_DEPENDENCIES=YesPlease
+endif
+endif
endif
ifdef COMPUTE_HEADER_DEPENDENCIES
--
1.7.5.3.368.g8b1b7.dirty
Fredrik Kuivinen wrote:
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.
Ah, my mistake. Good catch.
Here is an updated patch. It avoids the ++empty.c file by giving "-x
c" to the compiler.
Much nicer, thanks!
It also avoids the auto-detection when
COMPUTE_HEADER_DEPENDENCIES is set
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 <redacted>