Repository navigation
JIT self-recursion speedup, Phase 2 double-execution fix, test-framework infinite-loop fix - #48
Merged
Merged
Conversation
…k-harness double-execution bug Self-recursive calls in non-tail position (e.g. fib's two recursive calls inside `(+ (fib ...) (fib ...))`) were compiled through a forward-reference-cell wrapper lambda, paying an extra Python call per recursive call. The generated function is always named `_jit_fn`, so a self-reference can call it directly by that name instead - ~2.3-2.8x faster on naive/tree recursion across scales, with no change to tail-recursive, mutual-recursive, closure, HOF, map, or set!-based benchmarks. While re-measuring to validate this, found that scripts/benchmark.ss and best_case_bench.ss's `elapsed` timing macro silently doubled the execution of anything it measured that used Phase 2/JIT: it wraps the timed expression in a `let` (a closure call), and Phase 2's fallback mechanism re-executes a closure's *entire body* from scratch if any later statement hits a primitive outside its small fast-path allow-list (current-time, used by the macro itself, is one such primitive) - inflating every affected benchmark number by close to 2x relative to the separately-measured native-Python baseline. Fixed the macro to sequence with `begin` + a top-level `set!` instead of `let`, which isn't a closure call and isn't subject to the same fallback retry. Confirmed with an instrumented call counter (177 calls for fib(10), not 354) and re-measured every affected number in README-PERFORMANCE.md. The underlying interpreter behavior is unresolved and documented as an open correctness gap: any Phase-2-eligible closure that does real work and then calls a primitive outside that allow-list will have its side effects run twice, silently. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
apply_proc attempted Phase 2 for a whole closure body and, if any statement raised _TrampolineFallback (a call to a primitive outside the small _fast_prim_map allow-list, or to a set!-using closure), silently discarded the attempt and re-ran the entire body via the slow trampoline - including side effects from statements that had already completed. Nested closure calls are inlined into the same Python call chain (Phase 4's frame-reuse mechanism), so the failure could be several logical closures away from the one shared catch point, and could compound rather than just double (observed 2x/3x on a nested closure test, not just 2x). Fixed with _is_phase2_safe: a static, side-effect-free walk that transitively certifies a closure's whole reachable call graph will complete without ever needing a fallback, before apply_proc is allowed to attempt Phase 2 at all. Anything it can't prove safe - a call through a parameter or other computed operator, a call to a set!-using closure, map/for-each's callback - is conservatively treated as unsafe, so that closure runs via the always-correct slow trampoline instead of risking a partial execution. The old silent retry is gone: if a certified-safe closure still somehow raises _TrampolineFallback, that's now a loud crash (a checker bug to report) instead of a silent re-run. Real cost: Phase 5 (closure-factory/IIFE) and Phase 6 (HOF/parameter- as-operator) shapes lose their JIT/Phase-2 speedup at the outermost apply_proc entry point, measured ~1.1-1.85x slower in isolation. Naive/tail/mutual recursion, map, and set!-loops are unaffected. Added two regression tests (test_all.ss, jit-edge-cases group, cases 62-63) using a vector-based call counter to make double-execution directly observable; confirmed both fail against the pre-fix interpreter (177 -> 354, and a compounding (1,1) -> (2,3) case) before the fix, pass after. Documented as Phase 8 in README-PERFORMANCE.md, including the measured performance cost and what's deliberately left as future work (recovering Phase 5/6 speed for dynamic dispatch safely). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… by default; bump version to 2.1.0 run-unit-test-cases (interpreter-cps.ss) assumed every form in a define-tests block was assert-shaped, to build a diagnostic report by destructuring it on failure. define-tests blocks freely mix ordinary defines alongside asserts (test_all.ss does this constantly), and if a define raised instead, that assumption was wrong: destructuring it could itself throw. Since nothing updates the active handler register until after that destructuring succeeds, the failure caused trampoline's top-level exception handler to re-invoke the exact same handler with the exact same arguments - an unconditional infinite loop. Reproduces with a plain (/ 1 0) inside a define, no JIT involved, confirmed present on the original unmodified master (41c7814) - a latent bug never triggered before because nothing had previously made an exception propagate this far up. Fixed by checking the AST shape before attempting assert-specific destructuring instead of relying on catching a failure from it (this compiler's ds-transform pass doesn't support guard/exception forms), and giving the two "re-evaluate for diagnostics" calls (already flagged with FIXME comments in the original code) their own minimal fallback handler instead of reusing the outer one. The shared fallback had to be pulled out to a top-level define*: this specialized register-machine compiler only lifts lambdas passed through the CPS m/k/handler plumbing into their own functions, not a locally-defined, directly-called helper closure. Also: *use-stack-trace* now defaults to #f (~10-13% wall-clock and ~25-45% memory reduction on the trampoline path, per README-PERFORMANCE.md's own measurement of this pre-existing toggle); version bumped to 2.1.0. Added a permanent regression test (subprocess + hard timeout) so any future regression fails loudly instead of hanging the suite, plus a differential test that runs the whole test_all.ss corpus with Phase 2 forced on vs. off and asserts identical results, catching any future mismatch between _is_phase2_safe's static certification and _eval_direct's actual runtime behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
Three related pieces of work, in commit order:
1. JIT: drop indirection for self-recursive non-tail calls
Self-recursive calls in non-tail position (e.g.
fib's two recursive calls inside(+ (fib ...) (fib ...))) were compiled through a forward-reference-cell wrapper lambda, paying an extra Python call per recursive call. The generated function is always named_jit_fn, so a self-reference can call it directly by that name instead — ~2.3–2.8× faster on naive/tree recursion across scales, no change to tail-recursive/mutual-recursive/closure/HOF/map/set!shapes.While validating this, found and fixed a real bug in the benchmark harness itself: its
elapsedtiming macro wrapped the measured expression in alet(a closure call), and Phase 2's fallback mechanism silently re-executed a closure's entire body — including already-completed side effects — on any mid-body_TrampolineFallback. This inflated every affected benchmark number by close to 2× relative to the separately-measured native-Python baseline. Fixed the macro; documented the underlying interpreter bug as a known, still-open gap.2. Fix silent double-execution of side effects in Phase 2's fallback path
That "known, still-open gap" from commit 1 turned out to be worth fixing properly:
apply_procattempted Phase 2 for a whole closure body and silently discarded/retried the entire thing via the slow trampoline on any mid-body failure — including side effects from statements that had already run. Nested closure calls are inlined into the same Python call chain, so the failure could be several logical closures away from the shared catch point, and could compound (observed 2×/3×, not just 2×).Fixed with
_is_phase2_safe: a static, side-effect-free walk that transitively certifies a closure's whole reachable call graph will complete without ever needing a fallback, beforeapply_procis allowed to attempt Phase 2 at all. Anything unprovable (dynamic dispatch,set!-using callees,map/for-eachcallbacks) is conservatively treated as unsafe, so it runs via the always-correct slow trampoline instead. The old silent retry is gone — a soundness gap in the checker is now a loud crash, not a silent re-run.Real cost: Phase 5 (closure-factory/IIFE) and Phase 6 (HOF/parameter-as-operator) shapes lose their JIT/Phase-2 speedup at the outermost
apply_procentry point, ~1.1–1.85× slower in isolation. Naive/tail/mutual recursion,map, andset!-loops are unaffected.Two regression tests added (vector-based call counters, since
set!itself would disable Phase 2 for the counting closure), confirmed to fail against the pre-fix interpreter before the fix landed.3. Fix a pre-existing test-framework infinite loop; disable stack traces by default; bump version to 2.1.0
Verifying commit 2's design surfaced a separate, pre-existing bug:
run-unit-test-cases(the Scheme-level test-suite driver) assumed every form inside adefine-testsblock wasassert-shaped, for building diagnostic reports on failure.define-testsblocks freely mix ordinarydefines alongsideasserts, and if adefinefailed instead, destructuring it as an assert could itself throw — and since nothing updates the active handler register until after that destructuring succeeds, the failure re-invoked the exact same handler with the exact same arguments, forever. Reproduces with a plain(/ 1 0)inside adefine, confirmed present on the original unmodified master, unrelated to any JIT/Phase-2 behavior — just never triggered before because nothing had previously made an exception propagate this far up.Fixed by checking the AST shape before attempting assert-specific destructuring, and giving the two "re-evaluate for diagnostics" calls (already flagged
;; FIXME: could have an errorin the original code) their own minimal fallback handler.Also:
*use-stack-trace*now defaults to#f(~10–13% wall-clock, ~25–45% memory reduction on the trampoline path, per this repo's own prior measurement of the toggle); version bumped to 2.1.0.Test plan
pytest tests/(292 passed)jit-edge-casescases 62–65 intest_all.ss), confirmed to fail pre-fixtests/test_phase2_safety.py) runs the wholetest_all.sscorpus with Phase 2 forced on vs. off, asserts identical results — zero mismatchestests/test_unit_test_framework_error_handling.py), subprocess + hard timeout so a future regression fails loudly instead of hanging CIscripts/benchmark.py/scripts/best_case_bench.pyre-run before/after each change, isolated paired measurements (not noisy sequential runs) — seeREADME-PERFORMANCE.mdPhases 7–8 for full numbers🤖 Generated with Claude Code