From ae51415d6fafb844ff12fb79fb0f51d55be8e7f3 Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Wed, 9 Sep 2026 12:29:58 +0200 Subject: [PATCH] test(runner): pin the clock probe to once per run, not once per worker Resolving the clock implementation used to happen inside a `$( )`, so the resolved value died with that subshell and the main shell was left empty. Under --parallel every worker inherited that empty value and re-probed; on a shell without EPOCHREALTIME the probe forks `perl`, so the count scaled one-for-one with the tests. Measured here on a 500-test file, macOS arm64, bash 3.2.57: old init --no-parallel 3 --parallel 502 new init --no-parallel 2 --parallel 2 It fired even with per-test timing off, because deciding that timing is off is what asks whether the clock is expensive, and answering that resolves the impl. The behaviour is already fixed: #1358 converted clock::init to now_to_slot for its own reasons, which resolves the impl in the main shell before any worker is dispatched. Nothing pinned it there, so this adds the two guards. The unit test is the invariant itself -- init must leave _BASHUNIT_CLOCK_NOW_IMPL set in the caller -- and the acceptance test is the differential the issue asked for: eight times the tests must not cost more probes. Both fail against the old init (7 probes against 42). A differential rather than a budget, because a platform whose clock is EPOCHREALTIME or `date` forks no `perl` at all; comparing two sizes still fails loudly if the count starts tracking the test count. perf-fork-budget.md stated the parallel budget from a sequential census fixture, which cannot see a per-worker cost by construction. It now says so. Closes #1353 Claude-Session: https://claude.ai/code/session_01EXYWTGLjf7qM8Ru3GakDRm --- .claude/rules/perf-fork-budget.md | 10 +++- CHANGELOG.md | 1 + tests/acceptance/bashunit_run_forks_test.sh | 62 +++++++++++++++++++++ tests/unit/util/clock_test.sh | 38 +++++++++++++ 4 files changed, 110 insertions(+), 1 deletion(-) diff --git a/.claude/rules/perf-fork-budget.md b/.claude/rules/perf-fork-budget.md index f0422416..3a5ee8d9 100644 --- a/.claude/rules/perf-fork-budget.md +++ b/.claude/rules/perf-fork-budget.md @@ -258,7 +258,15 @@ Verify such a rewrite by running both implementations over the same input that cannot fail proves nothing. **Parallel 10-test file run (CI's mode):** ~11 forks — 3 `mkdir`, 4 `rm`, -3 `awk` (#813; was 61). The per-test result file is named by a per-suite +3 `awk` (#813; was 61). That count was taken with a sequential census fixture +and so missed the one cost that only exists in this mode: every worker used to +re-probe the clock, because the probe resolved the implementation inside a +`$( )` and the resolved value died with that subshell. On a shell without +`EPOCHREALTIME` the probe forks `perl`, so it scaled one-for-one with the tests +— 502 execs for a 500-test file against 2 — and it fired even with per-test +timing off, since deciding that timing is off is what asks whether the clock is +expensive (#1353). **Measure the parallel budget with a parallel fixture**: a +per-worker cost is invisible to a sequential census by construction. The per-test result file is named by a per-suite ordinal the single-threaded dispatcher assigns just before each `&` (the fork inherits it), so it costs **no** `mktemp` + `mv` per test (#851; was 10 `mktemp` + 10 `mv`). This replaced the old sanitized-test-name scheme, whose diff --git a/CHANGELOG.md b/CHANGELOG.md index ec407078..2ea7192b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Unreleased ### Changed +- Performance: a `--parallel` run on a shell without `EPOCHREALTIME` no longer forks `perl` once per test. Every worker re-probed the clock, because the probe resolved the implementation inside a command substitution and the resolved value died with it: 502 `perl` execs for a 500-test file, now 2. It happened even with per-test timing off, since deciding that timing is off is what asks whether the clock is expensive. Fixed by the `clock::init` change in #1358; this adds the guards that keep it fixed (#1353) - A helper can now ship a faster body for a newer Bash and keep the Bash 3.0 one as a fallback, chosen once at load time. The floor does not move: 3.0 keeps working and keeps being tested. The compatibility rules now record each construct's minimum version instead of only banning it, and allow one only inside a matching version gate — never for a construct that is a parse error on the floor, since those kill the file even in a branch that shell never takes. See `adrs/adr-013-bash-version-gated-fast-paths.md` (#1352) - Performance: a file defining `set_up_before_script` or `tear_down_after_script` costs about 5.6ms less on Bash 3.2, and a test rendered with `--show-execution-time` about 0.74ms less. Four clock reads per file and the per-test padding still went through a capture subshell, next to return-slot variants that were already there (#1348) - Performance: startup is faster on suites of plain test files. Printing "Running N tests" sourced every file a second time and re-ran every data provider before the run began. A file whose functions the provider scan can already see is counted from that scan instead: over this repo's 241 files the counting pass went from 1.72s to 1.51s, and 124 of them no longer source or run a provider twice. Files with a data provider, a heredoc, a multi-line string, an `eval`, a nested `source` or a conditional definition keep the old path, so the count can never disagree with the run (#1347) diff --git a/tests/acceptance/bashunit_run_forks_test.sh b/tests/acceptance/bashunit_run_forks_test.sh index fd23913d..8d9c2db2 100644 --- a/tests/acceptance/bashunit_run_forks_test.sh +++ b/tests/acceptance/bashunit_run_forks_test.sh @@ -335,3 +335,65 @@ function test_test_hooks_do_not_fork_mktemp_or_rm_per_test() { # The run's own scratch-dir cleanup, and nothing per test. assert_less_or_equal_than 1 "$rm_forks" } + +# Regression guard for the parallel clock probe. Resolving the clock +# implementation used to happen inside a `$( )`, so the resolved value died +# with that subshell and every --parallel worker re-probed. On a shell without +# EPOCHREALTIME the probe forks `perl`, so the count scaled one-for-one with +# the tests: 502 execs for a 500-test file against 2 (#1353). It fired even +# with per-test timing off, because deciding that timing is off is what asks +# whether the clock is expensive, which resolves the impl. +# +# Asserted as a differential rather than a budget: on a platform whose clock is +# `EPOCHREALTIME` or `date` this forks no `perl` at all, and comparing two sizes +# still fails loudly if the count ever starts tracking the test count. +function test_parallel_clock_probes_do_not_scale_with_the_test_count() { + if bashunit::check_os::is_windows; then + bashunit::skip "PATH shims are unreliable under Git Bash" && return + fi + + local dir + dir="$(bashunit::temp_dir)" + local count_file="$dir/perl_calls" + local real_perl + real_perl="$(command -v perl)" + if [ -z "$real_perl" ]; then + bashunit::skip "no perl on this machine to shim" && return + fi + + { + echo '#!/usr/bin/env bash' + echo "echo x >>\"$count_file\"" + echo "exec \"$real_perl\" \"\$@\"" + } >"$dir/perl" + chmod +x "$dir/perl" + + local few="$dir/few_test.sh" + local many="$dir/many_test.sh" + local i=0 + : >"$few" + while [ $i -lt 5 ]; do + echo "function test_f$i() { assert_true true; }" >>"$few" + i=$((i + 1)) + done + i=0 + : >"$many" + while [ $i -lt 40 ]; do + echo "function test_m$i() { assert_true true; }" >>"$many" + i=$((i + 1)) + done + + : >"$count_file" + PATH="$dir:$PATH" ./bashunit --parallel "$few" >/dev/null 2>&1 + local few_calls + few_calls="$(grep -c . "$count_file" || true)" + + : >"$count_file" + PATH="$dir:$PATH" ./bashunit --parallel "$many" >/dev/null 2>&1 + local many_calls + many_calls="$(grep -c . "$count_file" || true)" + + # Eight times the tests must not cost more probes. Equality, not a budget: + # the run resolves the clock once whatever the size. + assert_same "$few_calls" "$many_calls" +} diff --git a/tests/unit/util/clock_test.sh b/tests/unit/util/clock_test.sh index d288c392..61d7b94d 100644 --- a/tests/unit/util/clock_test.sh +++ b/tests/unit/util/clock_test.sh @@ -220,3 +220,41 @@ function test_clock_is_expensive_false_for_native_impls() { done assert_same " shell:no date:no date-seconds:no" "$result" } + +# Resolving the clock implementation writes _BASHUNIT_CLOCK_NOW_IMPL, and the +# whole point is that it survives into the caller. `init` used to resolve it +# inside `$( )`, so the assignment died with that subshell and the main shell +# was left empty. Under --parallel every worker then re-probed, and on a shell +# without EPOCHREALTIME the probe forks `perl`: 502 execs for a 500-test file +# against 2 (#1353). +function test_clock_init_resolves_the_impl_in_the_calling_shell() { + local saved_impl=${_BASHUNIT_CLOCK_NOW_IMPL:-} + local saved_start=${_BASHUNIT_START_TIME:-} + _BASHUNIT_CLOCK_NOW_IMPL="" + + bashunit::clock::init + + local resolved=$_BASHUNIT_CLOCK_NOW_IMPL + + _BASHUNIT_CLOCK_NOW_IMPL=$saved_impl + _BASHUNIT_START_TIME=$saved_start + + assert_not_empty "$resolved" +} + +# is_expensive answers a property of the resolved impl, and is asked once per +# test through BASHUNIT_SHOW_EXECUTION_TIME=auto. With the impl already +# resolved it must be a variable read, not another probe. +function test_is_expensive_does_not_reprobe_a_resolved_impl() { + local saved_impl=${_BASHUNIT_CLOCK_NOW_IMPL:-} + bashunit::clock::now_to_slot + + local before=$_BASHUNIT_CLOCK_NOW_IMPL + bashunit::clock::is_expensive || true + local after=$_BASHUNIT_CLOCK_NOW_IMPL + + _BASHUNIT_CLOCK_NOW_IMPL=$saved_impl + + assert_not_empty "$before" + assert_same "$before" "$after" +}