| Message ID | 20220525205651.825669-1-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Makefile: build 'gitweb' in the default target | expand |
On Wed, May 25 2022, SZEDER Gábor wrote: > Our Makefile's default target used to build 'gitweb', though > indirectly: the 'all' target depended on 'git-instaweb', which in turn > depended on 'gitweb'. Then e25c7cc146 (Makefile: drop dependency > between git-instaweb and gitweb, 2015-05-29) removed the latter > dependency, and for good reasons (quoting its commit message): > > "1. git-instaweb has no build-time dependency on gitweb; it > is a run-time dependency > > 2. gitweb is a directory that we want to recursively make > in. As a result, its recipe is marked .PHONY, which > causes "make" to rebuild git-instaweb every time it is > run." > > Since then a simple 'make' doesn't build 'gitweb'. > > Luckily, installing 'gitweb' is not broken: although 'make install' > doesn't depend on the 'gitweb' target, it runs 'make -C gitweb > install' unconditionally, which does generate all the necessary files > for 'gitweb' and installs them. However, if someone runs 'make && > sudo make install', then those files in the 'gitweb' directory will be > generated and owned by root, which is not nice. > > List 'gitweb' as a direct dependency of the default target, so a plain > 'make' will build it. > > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> > --- > Makefile | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Makefile b/Makefile > index f8bccfab5e..ee74892b33 100644 > --- a/Makefile > +++ b/Makefile > @@ -2188,6 +2188,8 @@ ifneq (,$X) > $(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) git$X)), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';) > endif > > +all:: gitweb > + > all:: > ifndef NO_TCLTK > $(QUIET_SUBDIR0)git-gui $(QUIET_SUBDIR1) gitexecdir='$(gitexec_instdir_SQ)' all In various recent patches & some upcoming ones I plan to submit I've been trying to get the runtime of a noop "make" runs down, which really helps e.g. with "git rebase -x make ..." running faster on a large series. While you're right that this wasn't intentional to begin with, we have lacked the "gitweb" as part of the default target since v2.4.5 now, and adding it back is a major performance regression on noop "make" runs: $ git hyperfine -L rev HEAD~1,HEAD~0 -L t Y, -s 'make' 'make NO_TCLTK={t}' --warmup 1 -r 5 Benchmark 1: make NO_TCLTK=Y' in 'HEAD~1 Time (mean ± σ): 103.6 ms ± 1.1 ms [User: 83.8 ms, System: 32.1 ms] Range (min … max): 102.2 ms … 105.2 ms 5 runs Benchmark 2: make NO_TCLTK=Y' in 'HEAD~0 Time (mean ± σ): 191.4 ms ± 1.6 ms [User: 151.0 ms, System: 60.5 ms] Range (min … max): 189.2 ms … 193.3 ms 5 runs Benchmark 3: make NO_TCLTK=' in 'HEAD~1 Time (mean ± σ): 272.0 ms ± 5.0 ms [User: 206.3 ms, System: 83.3 ms] Range (min … max): 266.7 ms … 277.3 ms 5 runs Benchmark 4: make NO_TCLTK=' in 'HEAD~0 Time (mean ± σ): 358.3 ms ± 1.4 ms [User: 282.7 ms, System: 104.0 ms] Range (min … max): 356.6 ms … 360.0 ms 5 runs Summary 'make NO_TCLTK=Y' in 'HEAD~1' ran 1.85 ± 0.02 times faster than 'make NO_TCLTK=Y' in 'HEAD~0' 2.63 ± 0.06 times faster than 'make NO_TCLTK=' in 'HEAD~1' 3.46 ± 0.04 times faster than 'make NO_TCLTK=' in 'HEAD~0' I.e. this is with your patch here applied as HEAD~0 and HEAD~1 being 'master'. I think given that that a better solution would be to just declare this as a feature at this point, especially as gitweb/INSTALL notes that the way to install it is: $ make prefix=/usr gitweb ;# as yourself # make gitwebdir=/var/www/cgi-bin install-gitweb ;# as root Or we could just fold gitweb/Makefile into the main Makefile, unlike gitk and git-gui it's not externally maintained, and most of it is shimmying to work around not being part of the main Makefile (which it strongly inter-depends on anyway).
On Thu, May 26, 2022 at 02:14:33AM +0200, Ævar Arnfjörð Bjarmason wrote: > > Our Makefile's default target used to build 'gitweb', though > > indirectly: the 'all' target depended on 'git-instaweb', which in turn > > depended on 'gitweb'. Then e25c7cc146 (Makefile: drop dependency > > between git-instaweb and gitweb, 2015-05-29) removed the latter > > dependency, and for good reasons (quoting its commit message): > [...] > In various recent patches & some upcoming ones I plan to submit I've > been trying to get the runtime of a noop "make" runs down, which really > helps e.g. with "git rebase -x make ..." running faster on a large > series. > > While you're right that this wasn't intentional to begin with, we have > lacked the "gitweb" as part of the default target since v2.4.5 now, and > adding it back is a major performance regression on noop "make" runs: Yes, I don't think building gitweb is worth the performance cost. Speeding things up was part of my original goal in e25c7cc146. It would be one thing if this were a recent change and somebody might be broken or confused by it not being built by default. But after 7 years, I think the question is: why _would_ we want to change the status quo and build gitweb by default? To exercise its Makefile for bugs, I guess, but IMHO it is not worth inflicting that on random developers. People who care about gitweb (if any) can build it themselves. I'd be even happier if it were just carried in its own tree. -Peff
On Thu, May 26, 2022 at 02:14:33AM +0200, Ævar Arnfjörð Bjarmason wrote: > > On Wed, May 25 2022, SZEDER Gábor wrote: > > > Our Makefile's default target used to build 'gitweb', though > > indirectly: the 'all' target depended on 'git-instaweb', which in turn > > depended on 'gitweb'. Then e25c7cc146 (Makefile: drop dependency > > between git-instaweb and gitweb, 2015-05-29) removed the latter > > dependency, and for good reasons (quoting its commit message): > > > > "1. git-instaweb has no build-time dependency on gitweb; it > > is a run-time dependency > > > > 2. gitweb is a directory that we want to recursively make > > in. As a result, its recipe is marked .PHONY, which > > causes "make" to rebuild git-instaweb every time it is > > run." > > > > Since then a simple 'make' doesn't build 'gitweb'. > > > > Luckily, installing 'gitweb' is not broken: although 'make install' > > doesn't depend on the 'gitweb' target, it runs 'make -C gitweb > > install' unconditionally, which does generate all the necessary files > > for 'gitweb' and installs them. However, if someone runs 'make && > > sudo make install', then those files in the 'gitweb' directory will be > > generated and owned by root, which is not nice. > > > > List 'gitweb' as a direct dependency of the default target, so a plain > > 'make' will build it. > > > > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> > > --- > > Makefile | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/Makefile b/Makefile > > index f8bccfab5e..ee74892b33 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -2188,6 +2188,8 @@ ifneq (,$X) > > $(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) git$X)), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';) > > endif > > > > +all:: gitweb > > + > > all:: > > ifndef NO_TCLTK > > $(QUIET_SUBDIR0)git-gui $(QUIET_SUBDIR1) gitexecdir='$(gitexec_instdir_SQ)' all > > In various recent patches & some upcoming ones I plan to submit I've > been trying to get the runtime of a noop "make" runs down, which really > helps e.g. with "git rebase -x make ..." running faster on a large > series. > > While you're right that this wasn't intentional to begin with, we have > lacked the "gitweb" as part of the default target since v2.4.5 now, and > adding it back is a major performance regression on noop "make" runs: I think that generating stuff, potentially as root, during 'make install' is a more severe regression, than this noop make slowdown, which in practice tends to be lost in the noise anyway. Even in an unrealistic case (it doesn't modify any C source files explicitly, let alone a frequently included header file) like this: $ git checkout fddc3b420f^ $ make [...] $ for i in {1..10} ; do git commit --allow-empty -q -m $i ; done $ time git rebase -x 'make -j8 NO_TCLTK=Y >/dev/null' HEAD~10 [...] real 0m31.026s user 0m46.897s sys 0m11.492s $ git checkout fddc3b420f $ for i in {1..10} ; do git commit --allow-empty -q -m $i ; done $ time git rebase -x 'make -j8 NO_TCLTK=Y >/dev/null' HEAD~10 [...] real 0m30.865s user 0m48.315s sys 0m12.125s Hrm, it actually ended up slightly faster. > $ git hyperfine -L rev HEAD~1,HEAD~0 -L t Y, -s 'make' 'make NO_TCLTK={t}' --warmup 1 -r 5 > Benchmark 1: make NO_TCLTK=Y' in 'HEAD~1 > Time (mean ± σ): 103.6 ms ± 1.1 ms [User: 83.8 ms, System: 32.1 ms] > Range (min … max): 102.2 ms … 105.2 ms 5 runs > > Benchmark 2: make NO_TCLTK=Y' in 'HEAD~0 > Time (mean ± σ): 191.4 ms ± 1.6 ms [User: 151.0 ms, System: 60.5 ms] > Range (min … max): 189.2 ms … 193.3 ms 5 runs > > Benchmark 3: make NO_TCLTK=' in 'HEAD~1 > Time (mean ± σ): 272.0 ms ± 5.0 ms [User: 206.3 ms, System: 83.3 ms] > Range (min … max): 266.7 ms … 277.3 ms 5 runs > > Benchmark 4: make NO_TCLTK=' in 'HEAD~0 > Time (mean ± σ): 358.3 ms ± 1.4 ms [User: 282.7 ms, System: 104.0 ms] > Range (min … max): 356.6 ms … 360.0 ms 5 runs > > Summary > 'make NO_TCLTK=Y' in 'HEAD~1' ran > 1.85 ± 0.02 times faster than 'make NO_TCLTK=Y' in 'HEAD~0' > 2.63 ± 0.06 times faster than 'make NO_TCLTK=' in 'HEAD~1' > 3.46 ± 0.04 times faster than 'make NO_TCLTK=' in 'HEAD~0' > > I.e. this is with your patch here applied as HEAD~0 and HEAD~1 being > 'master'. > > I think given that that a better solution would be to just declare this > as a feature at this point As long as 'make install' installs 'gitweb', I don't think that's an option. > especially as gitweb/INSTALL notes that the > way to install it is: > > $ make prefix=/usr gitweb ;# as yourself > # make gitwebdir=/var/www/cgi-bin install-gitweb ;# as root Or are you suggesting not to install 'gitweb' during 'make install'? I'm fine with that, but I doubt I will argue about it convincingly in a commit message. > Or we could just fold gitweb/Makefile into the main Makefile, unlike > gitk and git-gui it's not externally maintained, and most of it is > shimmying to work around not being part of the main Makefile (which it > strongly inter-depends on anyway).
On Thu, May 26 2022, SZEDER Gábor wrote: > On Thu, May 26, 2022 at 02:14:33AM +0200, Ævar Arnfjörð Bjarmason wrote: >> >> On Wed, May 25 2022, SZEDER Gábor wrote: >> >> > Our Makefile's default target used to build 'gitweb', though >> > indirectly: the 'all' target depended on 'git-instaweb', which in turn >> > depended on 'gitweb'. Then e25c7cc146 (Makefile: drop dependency >> > between git-instaweb and gitweb, 2015-05-29) removed the latter >> > dependency, and for good reasons (quoting its commit message): >> > >> > "1. git-instaweb has no build-time dependency on gitweb; it >> > is a run-time dependency >> > >> > 2. gitweb is a directory that we want to recursively make >> > in. As a result, its recipe is marked .PHONY, which >> > causes "make" to rebuild git-instaweb every time it is >> > run." >> > >> > Since then a simple 'make' doesn't build 'gitweb'. >> > >> > Luckily, installing 'gitweb' is not broken: although 'make install' >> > doesn't depend on the 'gitweb' target, it runs 'make -C gitweb >> > install' unconditionally, which does generate all the necessary files >> > for 'gitweb' and installs them. However, if someone runs 'make && >> > sudo make install', then those files in the 'gitweb' directory will be >> > generated and owned by root, which is not nice. >> > >> > List 'gitweb' as a direct dependency of the default target, so a plain >> > 'make' will build it. >> > >> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> >> > --- >> > Makefile | 2 ++ >> > 1 file changed, 2 insertions(+) >> > >> > diff --git a/Makefile b/Makefile >> > index f8bccfab5e..ee74892b33 100644 >> > --- a/Makefile >> > +++ b/Makefile >> > @@ -2188,6 +2188,8 @@ ifneq (,$X) >> > $(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) git$X)), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';) >> > endif >> > >> > +all:: gitweb >> > + >> > all:: >> > ifndef NO_TCLTK >> > $(QUIET_SUBDIR0)git-gui $(QUIET_SUBDIR1) gitexecdir='$(gitexec_instdir_SQ)' all >> >> In various recent patches & some upcoming ones I plan to submit I've >> been trying to get the runtime of a noop "make" runs down, which really >> helps e.g. with "git rebase -x make ..." running faster on a large >> series. >> >> While you're right that this wasn't intentional to begin with, we have >> lacked the "gitweb" as part of the default target since v2.4.5 now, and >> adding it back is a major performance regression on noop "make" runs: > > I think that generating stuff, potentially as root, during 'make > install' is a more severe regression, than this noop make slowdown, > which in practice tends to be lost in the noise anyway. It really isn't lost in the noise, e.g. a common use-case of mine is to have let's say a 10 patch series that I want to frequently run a spot check like this on: git rebase -i -x 'make' On my system before your patch (this is with my config.mak, so it has e.g. NO_TCLTK=Y) it's ~105ms per run, with your change ~200ms[1]. So that's 1s before of "make" overhead, now 2s. Is it the end of the world? No, but it does add up. In particular with ccache we can end up mostly spending time on make itself deciding what it's going to do. For comparison running EDITOR=cat git rebase -i -x 'echo make' HEAD~10 takes around 100ms in total for me. > unrealistic case (it doesn't modify any C source files explicitly, let > alone a frequently included header file) like this: Yeah that's fair, of course no-op runs are where it matters the most. But e.g. for forcing a hook.c change (including re-linking "git") it's 10% slower[2] overall if we use ccache. > $ git checkout fddc3b420f^ > $ make > [...] > $ for i in {1..10} ; do git commit --allow-empty -q -m $i ; done > $ time git rebase -x 'make -j8 NO_TCLTK=Y >/dev/null' HEAD~10 > [...] > real 0m31.026s > user 0m46.897s > sys 0m11.492s > $ git checkout fddc3b420f > $ for i in {1..10} ; do git commit --allow-empty -q -m $i ; done > $ time git rebase -x 'make -j8 NO_TCLTK=Y >/dev/null' HEAD~10 > [...] > real 0m30.865s > user 0m48.315s > sys 0m12.125s > > Hrm, it actually ended up slightly faster. I should have said that I create a "version" file in my checkout dir, e.g.: 2.36.GIT-dev Otherwise you end up with a guarnteed recompile-re-link every time HEAD changes, so it's likely lost in that noise. I just run this (via a script): echo $(/usr/bin/git grep -h -o -P '(?<=^DEF_VER=v).*' 'HEAD:GIT-VERSION-GEN')-dev And echo it to the top-level "version". Aside from what we're discussing here you might want to try that, your "git version" won't be correct, but for that low cost (I do generate it for the version I actually install) you'll get actual no-op runs in the case of e.g. only modifying tests. >> $ git hyperfine -L rev HEAD~1,HEAD~0 -L t Y, -s 'make' 'make NO_TCLTK={t}' --warmup 1 -r 5 >> Benchmark 1: make NO_TCLTK=Y' in 'HEAD~1 >> Time (mean ± σ): 103.6 ms ± 1.1 ms [User: 83.8 ms, System: 32.1 ms] >> Range (min … max): 102.2 ms … 105.2 ms 5 runs >> >> Benchmark 2: make NO_TCLTK=Y' in 'HEAD~0 >> Time (mean ± σ): 191.4 ms ± 1.6 ms [User: 151.0 ms, System: 60.5 ms] >> Range (min … max): 189.2 ms … 193.3 ms 5 runs >> >> Benchmark 3: make NO_TCLTK=' in 'HEAD~1 >> Time (mean ± σ): 272.0 ms ± 5.0 ms [User: 206.3 ms, System: 83.3 ms] >> Range (min … max): 266.7 ms … 277.3 ms 5 runs >> >> Benchmark 4: make NO_TCLTK=' in 'HEAD~0 >> Time (mean ± σ): 358.3 ms ± 1.4 ms [User: 282.7 ms, System: 104.0 ms] >> Range (min … max): 356.6 ms … 360.0 ms 5 runs >> >> Summary >> 'make NO_TCLTK=Y' in 'HEAD~1' ran >> 1.85 ± 0.02 times faster than 'make NO_TCLTK=Y' in 'HEAD~0' >> 2.63 ± 0.06 times faster than 'make NO_TCLTK=' in 'HEAD~1' >> 3.46 ± 0.04 times faster than 'make NO_TCLTK=' in 'HEAD~0' >> >> I.e. this is with your patch here applied as HEAD~0 and HEAD~1 being >> 'master'. >> >> I think given that that a better solution would be to just declare this >> as a feature at this point > > As long as 'make install' installs 'gitweb', I don't think that's an > option. Yes, I'm not saying that we shouldn't fix this no matter what, but just suggesting that perhaps we come up with a better solution. >> especially as gitweb/INSTALL notes that the >> way to install it is: >> >> $ make prefix=/usr gitweb ;# as yourself >> # make gitwebdir=/var/www/cgi-bin install-gitweb ;# as root > > Or are you suggesting not to install 'gitweb' during 'make install'? > I'm fine with that, but I doubt I will argue about it convincingly in > a commit message. We could also: * $(error out if install is in MAKECMDGOALS and we don't have those generated files, that's a few-line ifdef change to the Makefile, "build it first". * Just fold gitweb/Makefile into the top-level Makefile >> Or we could just fold gitweb/Makefile into the main Makefile, unlike >> gitk and git-gui it's not externally maintained, and most of it is >> shimmying to work around not being part of the main Makefile (which it >> strongly inter-depends on anyway). 1. $ git hyperfine -L rev HEAD~1,HEAD~0 -s 'make' 'make' Benchmark 1: make' in 'HEAD~1 Time (mean ± σ): 106.9 ms ± 1.3 ms [User: 86.0 ms, System: 33.1 ms] Range (min … max): 105.3 ms … 110.2 ms 27 runs Benchmark 2: make' in 'HEAD~0 Time (mean ± σ): 199.6 ms ± 2.5 ms [User: 162.7 ms, System: 57.3 ms] Range (min … max): 197.4 ms … 207.4 ms 15 runs Summary 'make' in 'HEAD~1' ran 1.87 ± 0.03 times faster than 'make' in 'HEAD~0' 2. $ git hyperfine -L rev HEAD~1,HEAD~0 -s 'make' 'make -W hook.c NO_TCLTK=Y CC="ccache cc"' --warmup 1 -r 5 Benchmark 1: make -W hook.c NO_TCLTK=Y CC="ccache cc"' in 'HEAD~1 Time (mean ± σ): 742.7 ms ± 7.1 ms [User: 1965.5 ms, System: 561.6 ms] Range (min … max): 730.3 ms … 748.2 ms 5 runs Benchmark 2: make -W hook.c NO_TCLTK=Y CC="ccache cc"' in 'HEAD~0 Time (mean ± σ): 819.2 ms ± 7.4 ms [User: 2013.9 ms, System: 583.3 ms] Range (min … max): 811.0 ms … 830.0 ms 5 runs Summary 'make -W hook.c NO_TCLTK=Y CC="ccache cc"' in 'HEAD~1' ran 1.10 ± 0.01 times faster than 'make -W hook.c NO_TCLTK=Y CC="ccache cc"' in 'HEAD~0'
diff --git a/Makefile b/Makefile index f8bccfab5e..ee74892b33 100644 --- a/Makefile +++ b/Makefile @@ -2188,6 +2188,8 @@ ifneq (,$X) $(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) git$X)), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';) endif +all:: gitweb + all:: ifndef NO_TCLTK $(QUIET_SUBDIR0)git-gui $(QUIET_SUBDIR1) gitexecdir='$(gitexec_instdir_SQ)' all
Our Makefile's default target used to build 'gitweb', though indirectly: the 'all' target depended on 'git-instaweb', which in turn depended on 'gitweb'. Then e25c7cc146 (Makefile: drop dependency between git-instaweb and gitweb, 2015-05-29) removed the latter dependency, and for good reasons (quoting its commit message): "1. git-instaweb has no build-time dependency on gitweb; it is a run-time dependency 2. gitweb is a directory that we want to recursively make in. As a result, its recipe is marked .PHONY, which causes "make" to rebuild git-instaweb every time it is run." Since then a simple 'make' doesn't build 'gitweb'. Luckily, installing 'gitweb' is not broken: although 'make install' doesn't depend on the 'gitweb' target, it runs 'make -C gitweb install' unconditionally, which does generate all the necessary files for 'gitweb' and installs them. However, if someone runs 'make && sudo make install', then those files in the 'gitweb' directory will be generated and owned by root, which is not nice. List 'gitweb' as a direct dependency of the default target, so a plain 'make' will build it. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- Makefile | 2 ++ 1 file changed, 2 insertions(+)