jit: expose builtin gateways and preserve recursion budget - #1172
Conversation
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit b5f45fa). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
`MAX_STACK_SIZE` is the byte budget a recursion limit of 1000 buys, and it was calibrated on the interpreter's ~1.7 KB per Python call level. A level that enters compiled code and leaves it through a guard failure costs ~6.3 KB, because `call_assembler_helper_trampoline` -> `jit_blackhole_resume_from_guard` -> `blackhole_resume_via_rd_numb` -> `BlackholeInterpreter::run` -> `handler_residual_call_r_r` -> `bh_call_fn` nests on the native stack once per level. At the default limit, calling one recursive function five times moved the depth it reached from 997 to 458. - `MAX_STACK_SIZE` 11 << 18 -> 48 << 18. - `DEFAULT_RUNTIME_THREAD_STACK_SIZE` 8 -> 64 MiB, leaving the OS clamp room for the whole budget. - `main_entry`'s interpreter thread announces its stack through `configure_current_thread_stack_size`, as `_thread`'s worker already did. Without it `effective_stack_length` fell back to 3/4 of `RLIMIT_STACK`, which describes the process's original thread and not that one. `sys.setrecursionlimit(2000)` now reaches 1997 rather than 999. Three budget assertions read `REQUESTED_STACK_LENGTH` rather than the stored length, which the OS clamp makes host-dependent. Assisted-by: Claude
fdbdbf9 to
3af0d52
Compare
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/3af0d52b3a5a143c1b397faab3b9ef15dd0dc7db/pyre-interpreter/src/stack_check.rs#L115
Avoid reserving 64 MiB for every default worker
When _thread.stack_size() remains zero, every Python-created worker now reserves 64 MiB instead of 8 MiB. In processes with an address-space limit or many threads, these reservations can exhaust virtual memory and make otherwise lightweight workers fail with can't start new thread—for example, a 1 GiB address-space limit leaves room for fewer than 16 such stacks before accounting for the rest of the process. The surrounding comments identify the larger reservation as compensation for the JIT guard-resume chain's amplified native-stack use, so that root cause should be fixed or the recursion budget decoupled rather than multiplying every worker's reservation.
AGENTS.md reference: AGENTS.md:L252-L254
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
What changed
lenthrough an interp2app-style wrapper and expose its translated JitCodeisinstancewrapper expose the positional argument-read shape used by builtin gateway descentRoot cause and final stack sizing
The original macOS
test_deep_nested_struct_frozensetregression was reproducible only under load and raisedRecursionError. The old native guard reserved 2.8 MiB for a recursion limit of 1000. A Python recursion level that enters compiled code and resumes through a failed guard costs about 6.3 KiB of native stack, so the native guard could fire far before the Python recursion limit.The first fix used a 64 MiB default worker stack. That removed the pickle failure, but it over-reserved address space per worker and let CPython's
support.infinite_recursion(20_000)JSON test spend longer than its 300-second timeout below the native guard.The final sizing keeps the 48 MiB requested budget used for JIT resume accounting, but gives ordinary runtime threads a 20 MiB native stack. The existing OS clamp leaves a 15 MiB effective guard: enough for the measured hot-resume cost at the default limit and for the 2000-level cold/hot parity fixture, while still bounding intentionally huge recursion limits. It changes no public recursion-limit value and adds no baseline FAIL/SKIP.
Validation
pyre-interpreterandpyre-jitLLBC; prepass completed with the existing 416 known Charon warningscargo fmt --all -- --checkcargo check --features dynasmcargo test --features dynasmtest_struct2/2,test_pickle3/3,test_json1/1test_pickle.py: 12/12 passed at concurrency 6