Avoid stack overflow on deep mutual recursion under --effects=double-translation#2243
Draft
hhugo wants to merge 2 commits into
Draft
Avoid stack overflow on deep mutual recursion under --effects=double-translation#2243hhugo wants to merge 2 commits into
hhugo wants to merge 2 commits into
Conversation
Direct-style mutually recursive functions overflow the JS stack under --effects=double-translation because the trampoline pass (Generate_closure.f) is short-circuited for that mode and the per-function direct version has no depth guard. The test runs the deep recursion under the double-translation profile and emits the snapshot synthetically under other modes.
…ts=double-translation Under --effects=double-translation, cps_needed mutually recursive functions get both a direct and a CPS version (paired via caml_cps_closure). The CPS side trampolines on caml_stack_check_depth, but the direct side called its siblings via ordinary JS calls, so a sufficiently deep direct-style mutual recursion blew the JS stack. Generate_closure now runs under double-translation. For each SCC of paired closures it renames the direct half, gates every recursive tail call on caml_stack_check_depth (mirroring the CPS-side pattern), and replaces the direct slot of caml_cps_closure with a wrapper that drives a runtime trampoline. When the guard fires the call bounces to the same inner direct closure via caml_trampoline_return, and the wrapper's loop reapplies it with a fresh stack budget — no CPS involvement. The regression test in mutual_recursion_deep.ml now succeeds at depth 1,000,000 under double-translation (it already did under cps and native).
Member
Author
|
@4y8 do you want to try this PR with |
|
That worked, thanks! |
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
2 participants
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
--effects=double-translation, cps_needed mutually recursive functions get both a direct and a CPS version (paired viacaml_cps_closure). The CPS side trampolines oncaml_stack_check_depth, but the direct side called its siblings via ordinary JS calls, so a sufficiently deep direct-style mutual recursion blew the JS stack.Generate_closurenow runs under double-translation. For each SCC of paired closures it renames the direct half, gates every recursive tail call oncaml_stack_check_depth(mirroring the CPS-side pattern), and replaces the direct slot ofcaml_cps_closurewith a wrapper that drives a runtimecaml_direct_trampolineloop. When the guard fires the call bounces to the same inner direct closure viacaml_trampoline_return, and the wrapper's loop reapplies it with a fresh stack budget — no CPS involvement.compiler/tests-jsoo/lib-effects/mutual_recursion_deep.mlrunsping/pongmutual recursion at depth 1,000,000.Test plan
dune runtest compiler/tests-jsoo/lib-effects/clean underdefault,with-effects, andwith-effects-double-translationprofilesdune build @compiler/tests-jsoo/lib-effects/runtest-jsclean under all three profilesdune runtest compiler/tests-jsoo --profile=with-effects-double-translationcleandune build @compiler/tests-jsoo/runtest-js --profile=with-effects-double-translationcleandune build @compiler/tests-ocaml/effects/runtest --profile=with-effects-double-translationcleandune build @lib/tests/runtest-js --profile=with-effects-double-translationcleanjs_parser_printer.ml(using/html comments) failures unrelated to this change (reproduce on master under the same Node version)🤖 Generated with Claude Code