Skip to content

perf(doubles): drop the forks from the spy hot path - #978

Merged
Chemaclass merged 1 commit into
mainfrom
perf/doubles-drop-normalize-forks
Aug 2, 2026
Merged

perf(doubles): drop the forks from the spy hot path#978
Chemaclass merged 1 commit into
mainfrom
perf/doubles-drop-normalize-forks

Conversation

@Chemaclass

Copy link
Copy Markdown
Member

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

before after
assert_have_been_called ×200 1111 ms 170 ms
assert_same ×200 (fork-free floor) 111 ms 111 ms

💡 Three fork classes removed, none inherent

$(cat "$file") for a counter. spy::times_to_slot read its count that way; the file holds one short number, so read -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 $2 is absent, so those became an explicit if [ -z "$label" ] to keep that laziness.

✅ Checked, not assumed

  • FUNCNAME is 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.
  • 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.

📊 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, case and 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.

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.
@Chemaclass Chemaclass added the enhancement New feature or request label Aug 2, 2026
@Chemaclass Chemaclass self-assigned this Aug 2, 2026
@Chemaclass
Chemaclass merged commit c62b44d into main Aug 2, 2026
37 checks passed
@Chemaclass
Chemaclass deleted the perf/doubles-drop-normalize-forks branch August 2, 2026 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant