Fix O(N^2) tail-call trampoline teardown under GCC/MSVC (fixes #50) - #51
Merged
Conversation
Both trampolines (evalFunctionBodyTrampoline, user_calls.cpp; and runCompiledFunctionTrampoline/runCompiledFunctionFromBoundTrampoline, bytecode_vm.cpp) keep every hop's own EvalContext alive in a `chain` vector for the whole run ($-vars stay dynamically scoped through even an isolated call, so a later hop may need to walk back through an earlier one's dyn level). Tearing `chain` down was left to its own std::vector<EvalContext> destructor -- whose element-destruction order is unspecified by the C++ standard. libc++ (Clang) destroys back-to-front, matching ScopeTrailStorage::popLevel's "scan from the back" optimization (its own doc comment assumes the level being popped is usually the most recently pushed one); libstdc++ (GCC) destroys front-to-back, the adversarial order for that same scan -- each pop then walks almost the entire remaining vector, turning an intended O(N) teardown into a real, measured O(N^2). Confirmed via a minimal repro (std::vector<T>::clear() literally prints destruction order in opposite directions under clang++ vs g++-16 for identical source) and an N-scaling experiment (Clang: clean O(N); GCC: time roughly quadruples when N doubles, at every step). The two previously-skipped tests (see #50) went from 25-50+ minutes each on GCC to under 1 second, matching Clang throughout -- verified locally with a real Homebrew GCC 16 build (same machine/OS/allocator as the Clang baseline) and against the full 1292-test suite on both compilers. Fix: an RAII guard tears `chain` down back-to-front explicitly, on every exit path (normal return OR exception unwind -- the recursion- guard error these tests hit is exactly the case a trailing-statement teardown would miss). Re-enables the two tests previously skipped on non-Apple CI, plus a new timing-bounded regression tripwire. 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
1 participant
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
EvalContextalive in achainvector for the whole run, then relied onstd::vector<EvalContext>'s own destructor to tear it down. That destruction order is unspecified by the C++ standard -- libc++ (Clang) destroys back-to-front, matchingScopeTrailStorage::popLevel's "scan from the back" optimization; libstdc++ (GCC) destroys front-to-back, the adversarial order for that same scan, turning an intended O(N) teardown into a real O(N^2).std::vector<T>::clear()prints opposite destruction order under clang++ vs g++-16 for identical source) and an N-scaling experiment (Clang: clean O(N); GCC: time quadruples when N doubles).chaindown back-to-front explicitly, on every exit path (normal return and exception unwind -- the recursion-guard error these tests hit is exactly the case a trailing-statement teardown would have missed).TailCalls.InfiniteTailRecursionHitsTheIterationCapInsteadOfHanging,TailCalls.InfiniteTailRecursionErrorMentionsTheFunctionName), plus adds a timing-bounded regression tripwire so a future regression fails fast instead of silently reintroducing a 50-minute CI run.Test plan
TailCalls.InfiniteTailRecursionHitsTheCapInBoundedWallTimeregression test (generous 30s ceiling, won't trip on ordinary machine noise).🤖 Generated with Claude Code