From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
"Elijah Newren" <newren@gmail.com>,
"Fabian Stelzer" <fs@gigacodes.de>,
"Eric Sunshine" <sunshine@sunshineco.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [RFC PATCH] t/Makefile: use dependency graph for "check-chainlint"
Date: Mon, 13 Dec 2021 11:09:42 +0100 [thread overview]
Message-ID: <RFC-patch-1.1-bb3f1577829-20211213T095456Z-avarab@gmail.com> (raw)
In-Reply-To: <20211213063059.19424-6-sunshine@sunshineco.com>
This gets rid of this fixed cost when running "make T=<name>":
$ git hyperfine -L rev HEAD~1,HEAD~0 -s 'make -C t check-chainlint' 'make -C t check-chainlint'
Benchmark 1: make -C t check-chainlint' in 'HEAD~1
Time (mean ± σ): 111.3 ms ± 0.5 ms [User: 116.9 ms, System: 26.2 ms]
Range (min … max): 110.5 ms … 112.5 ms 26 runs
Benchmark 2: make -C t check-chainlint' in 'HEAD~0
Time (mean ± σ): 12.5 ms ± 0.2 ms [User: 11.5 ms, System: 1.0 ms]
Range (min … max): 12.1 ms … 13.2 ms 223 runs
Summary
'make -C t check-chainlint' in 'HEAD~0' ran
8.92 ± 0.16 times faster than 'make -C t check-chainlint' in 'HEAD~1'
---
On Mon, Dec 13 2021, Eric Sunshine wrote:
> Rather than running `chainlint` and `diff` once per self-test -- which
> may become expensive as more tests are added -- instead run `chainlint`
> a single time over all tests bodies collectively and compare the result
> to the collective "expected" output.
I think that "optimizing" things like this is an anti-pattern. I.e. we
have N chainlint test files, and N potential outputs from that (ok or
not, and with/without error). If one of the chainlint tests changes
we'd like to re-run it, if not we can re-use an earlier run.
This is something make's dependency logic is perfectly suited for, and
will be faster than any optimization of turning a for-loop into a
"sed" command we run every time, since we'll only need to "stat" a few
things to see that there's nothing to do.
I've had the below as part of my local build for a while. It
conflicts/would be improved by my in-flight ab/make-dependency, which
you also ran into conflicts with.
This also has the advantage that you'll see what specific test failed
from the Makefile output. Aside from the ".build" rule needing to be
fixed up to use the "mkdir" trick in ab/make-dependency it should also
use $(QUIET_GEN) or whatever.
I'm not quite happy with the below, the wildcard/patsubst is a bit
messy, and it doesn't properly work around "make test" and how it runs
"clean" after its run, but you'll find that "make chainlint" will
properly cache things with this.
t/.gitignore | 1 +
t/Makefile | 29 +++++++++++++++--------------
2 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/t/.gitignore b/t/.gitignore
index 91cf5772fe5..f142d6d42fd 100644
--- a/t/.gitignore
+++ b/t/.gitignore
@@ -1,3 +1,4 @@
+/.build/
/trash directory*
/test-results
/.prove
diff --git a/t/Makefile b/t/Makefile
index 882d26eee30..9bac0e683c9 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -18,10 +18,8 @@ TEST_LINT ?= test-lint
ifdef TEST_OUTPUT_DIRECTORY
TEST_RESULTS_DIRECTORY = $(TEST_OUTPUT_DIRECTORY)/test-results
-CHAINLINTTMP = $(TEST_OUTPUT_DIRECTORY)/chainlinttmp
else
TEST_RESULTS_DIRECTORY = test-results
-CHAINLINTTMP = chainlinttmp
endif
# Shell quote;
@@ -29,13 +27,12 @@ SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
TEST_SHELL_PATH_SQ = $(subst ','\'',$(TEST_SHELL_PATH))
PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
TEST_RESULTS_DIRECTORY_SQ = $(subst ','\'',$(TEST_RESULTS_DIRECTORY))
-CHAINLINTTMP_SQ = $(subst ','\'',$(CHAINLINTTMP))
T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))
THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))
TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
-CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
+CHAINLINTTESTS = $(wildcard chainlint/*.test)
CHAINLINT = sed -f chainlint.sed
all: $(DEFAULT_TEST_TARGET)
@@ -67,16 +64,20 @@ clean: clean-except-prove-cache
$(RM) .prove
clean-chainlint:
- $(RM) -r '$(CHAINLINTTMP_SQ)'
-
-check-chainlint:
- @mkdir -p '$(CHAINLINTTMP_SQ)' && \
- err=0 && \
- for i in $(CHAINLINTTESTS); do \
- $(CHAINLINT) <chainlint/$$i.test | \
- sed -e '/^# LINT: /d' >'$(CHAINLINTTMP_SQ)'/$$i.actual && \
- diff -u chainlint/$$i.expect '$(CHAINLINTTMP_SQ)'/$$i.actual || err=1; \
- done && exit $$err
+ $(RM) -r .build/chainlint
+
+BUILT_CHAINLINTTESTS = $(patsubst %,.build/%.actual,$(CHAINLINTTESTS))
+
+.build/chainlint:
+ mkdir -p $@
+
+$(BUILT_CHAINLINTTESTS): | .build/chainlint
+$(BUILT_CHAINLINTTESTS): .build/%.actual: %
+ $(CHAINLINT) <$< | \
+ sed -e '/^# LINT: /d' >$@ && \
+ diff -u $(basename $<).expect $@
+
+check-chainlint: $(BUILT_CHAINLINTTESTS)
test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
test-lint-filenames
--
2.34.1.1025.g9a0c3a30920
next prev parent reply other threads:[~2021-12-13 10:36 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-13 6:30 [PATCH 00/15] generalize chainlint self-tests Eric Sunshine
2021-12-13 6:30 ` [PATCH 01/15] t/chainlint/*.test: don't use invalid shell syntax Eric Sunshine
2021-12-13 6:30 ` [PATCH 02/15] t/chainlint/*.test: fix invalid test cases due to mixing quote types Eric Sunshine
2021-12-13 6:30 ` [PATCH 03/15] t/chainlint/*.test: generalize self-test commentary Eric Sunshine
2021-12-13 6:30 ` [PATCH 04/15] t/chainlint/one-liner: avoid overly intimate chainlint.sed knowledge Eric Sunshine
2021-12-13 6:30 ` [PATCH 05/15] t/Makefile: optimize chainlint self-test Eric Sunshine
2021-12-13 10:09 ` Ævar Arnfjörð Bjarmason [this message]
2021-12-14 7:44 ` [RFC PATCH] t/Makefile: use dependency graph for "check-chainlint" Eric Sunshine
2021-12-14 12:34 ` Ævar Arnfjörð Bjarmason
2021-12-13 10:22 ` [PATCH 05/15] t/Makefile: optimize chainlint self-test Fabian Stelzer
2021-12-13 14:27 ` Eric Sunshine
2021-12-13 15:43 ` Fabian Stelzer
2021-12-13 16:02 ` Eric Sunshine
2021-12-13 16:11 ` Ævar Arnfjörð Bjarmason
2021-12-13 17:05 ` Eric Sunshine
2021-12-13 17:25 ` Eric Sunshine
2021-12-13 19:33 ` Ævar Arnfjörð Bjarmason
2021-12-13 21:37 ` Eric Sunshine
2021-12-13 16:14 ` Fabian Stelzer
2021-12-16 13:17 ` Ævar Arnfjörð Bjarmason
2021-12-16 15:47 ` Eric Sunshine
2021-12-16 19:26 ` Ævar Arnfjörð Bjarmason
2021-12-13 6:30 ` [PATCH 06/15] chainlint.sed: improve ?!AMP?! placement accuracy Eric Sunshine
2021-12-13 6:30 ` [PATCH 07/15] chainlint.sed: improve ?!SEMI?! " Eric Sunshine
2021-12-13 6:30 ` [PATCH 08/15] chainlint.sed: tolerate harmless ";" at end of last line in block Eric Sunshine
2021-12-13 6:30 ` [PATCH 09/15] chainlint.sed: drop unnecessary distinction between ?!AMP?! and ?!SEMI?! Eric Sunshine
2021-12-13 6:30 ` [PATCH 10/15] chainlint.sed: drop subshell-closing ">" annotation Eric Sunshine
2021-12-13 6:30 ` [PATCH 11/15] chainlint.sed: make here-doc "<<-" operator recognition more POSIX-like Eric Sunshine
2021-12-13 6:30 ` [PATCH 12/15] chainlint.sed: don't mistake `<< word` in string as here-doc operator Eric Sunshine
2021-12-13 6:30 ` [PATCH 13/15] chainlint.sed: stop throwing away here-doc tags Eric Sunshine
2021-12-13 6:30 ` [PATCH 14/15] chainlint.sed: swallow comments consistently Eric Sunshine
2021-12-13 6:30 ` [PATCH 15/15] chainlint.sed: stop splitting "(..." into separate lines "(" and "..." Eric Sunshine
2021-12-15 0:00 ` [PATCH 00/15] generalize chainlint self-tests Elijah Newren
2021-12-15 3:15 ` Eric Sunshine
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=RFC-patch-1.1-bb3f1577829-20211213T095456Z-avarab@gmail.com \
--to=avarab@gmail.com \
--cc=fs@gigacodes.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=newren@gmail.com \
--cc=peff@peff.net \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).