perf(doubles): drop the forks from the spy hot path - #978
Merged
Conversation
A spy-heavy test was paying subprocess forks per spied call and per assertion.
A 200-call test took 1111ms where the fork-free floor is ~111ms. It now takes
170ms.
Three fork classes removed, none of them inherent:
bashunit::spy::times_to_slot read its counter with `_c=$(cat "$file")`. The file
holds one short number, so `read -r` -- a builtin -- does the same job with no
fork. The generated spy body did the same `$(cat)` on *every spied invocation*,
which is the hottest spot of the three.
Eleven call sites in doubles/assertions.sh and three in doubles/spy.sh wrapped
bashunit's own pure-bash helpers in `$( )`, paying a subshell to compute
something the shell could compute in place. Both helpers already ship
return-slot variants for exactly this reason -- normalize_variable_name_to_slot
and normalize_test_function_name_to_slot -- and were simply not being used here.
The defaulted-label sites needed restructuring rather than substitution:
`local label="${2:-$(...)}"` only evaluates the default when $2 is absent, so
they became an explicit `if [ -z "$label" ]` to keep that laziness.
Two things checked rather than assumed. FUNCNAME is stable across a command
substitution -- verified directly -- so moving these calls out of `$( )` while
still passing "${FUNCNAME[1]}" resolves to the same frame and the failure labels
are unchanged. And the counter read is guarded with a numeric `case`, so a
truncated or empty file yields 0 exactly as the old `|| builtin echo 0` did.
Worth recording for whoever profiles this next: the first attempt converted only
the eleven assertion-level forks and moved 1111ms to 1031ms, a 7% win. The cost
was almost entirely inside the spy helper, not at the call sites. Measure before
concluding which fork matters.
Bash 3.0 safe: `read`, `case` and parameter expansion only. Fork budgets
unchanged (11/11); 1657 sequential / 1616 parallel.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤔 Background
A spy-heavy test paid subprocess forks per spied call and per assertion. A 200-call test took 1111 ms where the fork-free floor is ~111 ms.
assert_have_been_called×200assert_same×200 (fork-free floor)💡 Three fork classes removed, none inherent
$(cat "$file")for a counter.spy::times_to_slotread its count that way; the file holds one short number, soread -r— a builtin — does it with no fork. The generated spy body did the same$(cat)on every spied invocation, the hottest of the three.14 sites wrapping bashunit's own pure-bash helpers in
$( )— paying a subshell to compute something the shell could compute in place. Both helpers already ship return-slot variants for exactly this reason (normalize_variable_name_to_slot,normalize_test_function_name_to_slot) and simply weren't being used here.Defaulted labels needed restructuring, not substitution.
local label="${2:-$(...)}"only evaluates the default when$2is absent, so those became an explicitif [ -z "$label" ]to keep that laziness.✅ Checked, not assumed
FUNCNAMEis stable across a command substitution — verified directly. Moving these calls out of$( )while still passing"${FUNCNAME[1]}"resolves to the same frame, so failure labels are unchanged.case, so a truncated or empty file yields0exactly as the old|| builtin echo 0did.📊 A note for whoever profiles this next
My first attempt converted only the eleven assertion-level forks: 1111 ms → 1031 ms, a 7% win. The cost was almost entirely inside the spy helper, not at the call sites. Measure before concluding which fork matters.
🔒 Verification
Bash 3.0 safe —
read,caseand parameter expansion only; compat gate green (14/14). Fork budgets unchanged (11/11).make sa·make lint·bash build.sh bin -v→✅ Build verified ✅· 1657 sequential / 1616 parallel-simple-strict.