git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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


  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).