jit: module-scope LOAD_NAME builtins fold; interpreter: shutdown module teardown - #1187
Conversation
|
Warning Review limit reached
Next review available in: 6 seconds 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 (13)
WalkthroughThe PR adds JIT builtin fallback folding for module-scope ChangesJIT builtin lookup
Runtime shutdown teardown
Hashlib behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🟠 High · up to The current changes still contain a lookup-folding path that can return the wrong value and a BLAKE2 input path that may exhaust memory instead of rejecting oversized input; the added benchmark also fails reported lint rules. Merge should wait for these issues to be addressed. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc4f22d2a0
ℹ️ 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".
| clear_shutdown_module_dict(dict); | ||
| collect_and_run_finalizers(ec_ptr); |
There was a problem hiding this comment.
When an imported module stores a finalizable object under a private name such as _obj, and its __del__ reads a public module global, clear_shutdown_module_dict first replaces _obj but then clears every public global before this collection runs. Unlike CPython's refcounting _PyModule_ClearDict, assigning None does not immediately finalize the object here, so the underscore-first pass provides no protection and __del__ observes its dependencies as None. Collect after releasing private bindings (or otherwise preserve per-binding finalization ordering) before clearing the second pass.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/bench/synth/load_name_builtin_cell_fold.py`:
- Around line 13-20: Update the benchmark loops to avoid unused loop-variable
warnings, and replace the module-scope lambda assignment to len with a named
function that preserves the constant-returning behavior and unused argument.
Retain the module-scope len binding for invalidation coverage, adding an
explicit A001 suppression for the intentional builtin shadowing.
In `@pyre/pyre-interpreter/src/module/_hashlib/mod.rs`:
- Around line 856-875: Update the BLAKE2 parameter handling around the
max_key_size, salt_size, and person_size checks to obtain length-only buffer
views for key, salt, and person before calling read_hash_buffer. Validate each
view’s length against its corresponding limit first, then copy only validated
buffers so oversized memoryviews return ValueError without allocating their full
contents.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs`:
- Line 13171: Update the builtin-fold path around emit_builtins_cell_fold to
return Ok(false) when live_globals is null or does not equal w_globals,
preventing specialization against a non-authoritative dictionary. Preserve the
existing fold only when both references identify the same globals dictionary.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9cbcb7bf-e23a-4665-b594-949e756af029
📒 Files selected for processing (9)
pyre/bench/synth/load_name_builtin_cell_fold.cranelift.jitstatspyre/bench/synth/load_name_builtin_cell_fold.dynasm.jitstatspyre/bench/synth/load_name_builtin_cell_fold.pypyre/bench/synth/load_name_builtin_cell_fold.wasm.jitstatspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/module/_blake2/_blake2_app.pypyre/pyre-interpreter/src/module/_hashlib/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyrex/src/lib.rs
💤 Files with no reviewable changes (1)
- pyre/pyre-interpreter/src/module/_blake2/_blake2_app.py
| for i in range(N): | ||
| total = total + len(s) | ||
| print(total) | ||
|
|
||
| len = lambda x: 100 | ||
| shadowed = 0 | ||
| for i in range(M): | ||
| shadowed = shadowed + len(s) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the benchmark pass Ruff.
Ruff reports unused loop variables, builtin shadowing, lambda assignment, and an unused lambda argument. Keep the module-scope len binding for invalidation coverage, but use a named function and an explicit A001 suppression.
Proposed fix
-for i in range(N):
+for _i in range(N):
total = total + len(s)
print(total)
-len = lambda x: 100
+def len(_x): # noqa: A001
+ return 100
+
shadowed = 0
-for i in range(M):
+for _i in range(M):
shadowed = shadowed + len(s)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for i in range(N): | |
| total = total + len(s) | |
| print(total) | |
| len = lambda x: 100 | |
| shadowed = 0 | |
| for i in range(M): | |
| shadowed = shadowed + len(s) | |
| for _i in range(N): | |
| total = total + len(s) | |
| print(total) | |
| def len(_x): # noqa: A001 | |
| return 100 | |
| shadowed = 0 | |
| for _i in range(M): | |
| shadowed = shadowed + len(s) |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 13-13: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
[error] 17-17: Variable len is shadowing a Python builtin
(A001)
[error] 17-17: Do not assign a lambda expression, use a def
Rewrite len as a def
(E731)
[warning] 17-17: Unused lambda argument: x
(ARG005)
[warning] 19-19: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/bench/synth/load_name_builtin_cell_fold.py` around lines 13 - 20, Update
the benchmark loops to avoid unused loop-variable warnings, and replace the
module-scope lambda assignment to len with a named function that preserves the
constant-returning behavior and unused argument. Retain the module-scope len
binding for invalidation coverage, adding an explicit A001 suppression for the
intentional builtin shadowing.
Source: Linters/SAST tools
| let (max_key_size, salt_size, person_size) = match name { | ||
| "blake2b" => (64, 16, 16), | ||
| "blake2s" => (32, 8, 8), | ||
| _ => unreachable!(), | ||
| }; | ||
| if key.len() > max_key_size { | ||
| return Err(crate::PyError::value_error(format!( | ||
| "maximum key length is {max_key_size} bytes" | ||
| ))); | ||
| } | ||
| if salt.len() > salt_size { | ||
| return Err(crate::PyError::value_error(format!( | ||
| "maximum salt length is {salt_size} bytes" | ||
| ))); | ||
| } | ||
| if person.len() > person_size { | ||
| return Err(crate::PyError::value_error(format!( | ||
| "maximum person length is {person_size} bytes" | ||
| ))); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target context ---'
sed -n '800,890p' pyre/pyre-interpreter/src/module/_hashlib/mod.rs
printf '%s\n' '--- read_hash_buffer definitions and calls ---'
rg -n -C 5 'read_hash_buffer' pyre
printf '%s\n' '--- BLAKE2 constructor call sites ---'
rg -n -C 4 '_blake2_new|blake2b|blake2s' pyre/pyre-interpreter/src/module/_hashlibRepository: youknowone/pyre
Length of output: 22416
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- read_hash_buffer implementation ---'
sed -n '955,1010p' pyre/pyre-interpreter/src/module/_hashlib/mod.rs
printf '%s\n' '--- buffer protocol implementation references ---'
rg -n -C 5 'PyBUF_SIMPLE|buffer.*view|BufferView|view.*len|as_bytes|to_vec|Vec::from|copy_from_slice' pyre/pyre-interpreter/src pyre/pyre-object/src | head -n 240
printf '%s\n' '--- BLAKE2 app-level validation ---'
sed -n '1,180p' pyre/pyre-interpreter/src/module/_hashlib/_hashlib_app.pyRepository: youknowone/pyre
Length of output: 21510
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- simple_buffer_bytes definition ---'
rg -n -C 10 'fn simple_buffer_bytes|simple_buffer_bytes' pyre
printf '%s\n' '--- buffer view types and byte conversion ---'
rg -n -C 8 'struct BufferView|impl BufferView|fn as_bytes|pub fn as_bytes|Buffer::|getbuffer|buffer.*bytes' pyre/pyre-object/src pyre/pyre-interpreter/src | head -n 320
printf '%s\n' '--- large-buffer constructors and allocation paths ---'
rg -n -C 5 'bytearray|w_bytearray|memoryview|Buffer::Owned|Vec<u8>|with_capacity' pyre/pyre-object/src | head -n 320Repository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- buffer acquisition implementation ---'
sed -n '8460,8615p' pyre/pyre-interpreter/src/baseobjspace.rs
printf '%s\n' '--- SimpleBufferBytes definition ---'
rg -n -C 12 'struct SimpleBufferBytes|impl SimpleBufferBytes|enum BufferRequest' pyre/pyre-interpreter/src/baseobjspace.rsRepository: youknowone/pyre
Length of output: 8542
🏁 Script executed:
#!/bin/bash
set -e
sed -n '8613,8755p' pyre/pyre-interpreter/src/baseobjspace.rsRepository: youknowone/pyre
Length of output: 5373
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- _blake2 bindings and app-level callers ---'
rg -n -C 8 '_blake2_new|_blake2|blake2_new' pyre --glob '*.rs' --glob '*.py'
printf '%s\n' '--- buffer gather implementation ---'
rg -n -C 8 'fn memoryview_gather_bytes|memoryview_gather_bytes' pyre
printf '%s\n' '--- BLAKE2 tests and size limits ---'
rg -n -C 6 'maximum key length|maximum salt length|maximum person length|blake2b.*key|blake2s.*key|memoryview' pyre --glob '*test*' --glob '*.py' --glob '*.rs' | head -n 260Repository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -e
sed -n '35,175p' pyre/pyre-interpreter/src/module/_blake2/_blake2_app.pyRepository: youknowone/pyre
Length of output: 5849
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
hashlib = Path("pyre/pyre-interpreter/src/module/_hashlib/mod.rs").read_text()
base = Path("pyre/pyre-interpreter/src/baseobjspace.rs").read_text()
builtins = Path("pyre/pyre-interpreter/src/builtins.rs").read_text()
def body(source, signature, end_marker):
start = source.index(signature)
end = source.index(end_marker, start)
return source[start:end]
blake = body(hashlib, "fn blake2_new(", "/// Keyword names")
reader = body(hashlib, "fn read_hash_buffer(", "/// Keyword names")
buffer = body(base, "fn buffer_bytes(", "/// Look up a descriptor")
gather = body(builtins, "pub(crate) unsafe fn memoryview_gather_bytes", "/// `PyBuffer_ToContiguous")
calls = [blake.index(f"read_hash_buffer(arg({i}))?") for i in (3, 4, 5)]
checks = [blake.index(token) for token in ("if key.len()", "if salt.len()", "if person.len()")]
assert calls == sorted(calls), "BLAKE2 parameter buffers are not read in source order"
assert max(calls) < min(checks), "A parameter buffer is checked before materialization"
assert "buf.as_bytes().to_vec()" in reader, "read_hash_buffer no longer copies the acquired view"
assert "memoryview_gather_bytes" in buffer, "buffer_bytes does not gather memoryview contents"
assert "-> Vec<u8>" in gather, "memoryview_gather_bytes is not Vec-returning"
print("read_hash_buffer copies acquired buffer contents")
print("buffer_bytes gathers memoryview contents into Vec<u8>")
print("all BLAKE2 key/salt/person buffers are materialized before size checks")
PYRepository: youknowone/pyre
Length of output: 294
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
hashlib = Path("pyre/pyre-interpreter/src/module/_hashlib/mod.rs").read_text()
base = Path("pyre/pyre-interpreter/src/baseobjspace.rs").read_text()
builtins = Path("pyre/pyre-interpreter/src/builtins.rs").read_text()
def after(source, start_text, end_text):
start = source.index(start_text)
end = source.index(end_text, start)
return source[start:end]
blake = after(hashlib, "fn blake2_new(", "const NEW_KEYWORDS")
reader = after(hashlib, "fn read_hash_buffer(", "const NEW_KEYWORDS")
buffer = after(base, "fn buffer_bytes(", "/// Look up a descriptor")
gather = after(
builtins,
"pub(crate) unsafe fn memoryview_gather_bytes",
"/// `PyBuffer_ToContiguous",
)
calls = [blake.index(f"read_hash_buffer(arg({i}))?") for i in (3, 4, 5)]
checks = [blake.index(token) for token in ("if key.len()", "if salt.len()", "if person.len()")]
assert calls == sorted(calls)
assert max(calls) < min(checks)
assert "buf.as_bytes().to_vec()" in reader
assert "memoryview_gather_bytes" in buffer
assert "-> Vec<u8>" in gather
print("read_hash_buffer copies acquired buffer contents")
print("buffer_bytes gathers memoryview contents into Vec<u8>")
print("all BLAKE2 key/salt/person buffers are materialized before size checks")
PYRepository: youknowone/pyre
Length of output: 294
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
hashlib = Path("pyre/pyre-interpreter/src/module/_hashlib/mod.rs").read_text()
base = Path("pyre/pyre-interpreter/src/baseobjspace.rs").read_text()
builtins = Path("pyre/pyre-interpreter/src/builtins.rs").read_text()
start = hashlib.index("fn blake2_new(")
param_reads = [
hashlib.index(f"read_hash_buffer(arg({i}))?", start)
for i in (3, 4, 5)
]
checks = [
hashlib.index(token, start)
for token in ("if key.len()", "if salt.len()", "if person.len()")
]
assert param_reads == sorted(param_reads)
assert max(param_reads) < min(checks)
assert "buf.as_bytes().to_vec()" in hashlib
assert "memoryview_gather_bytes" in base
assert "pub(crate) unsafe fn memoryview_gather_bytes" in builtins
assert "-> Vec<u8>" in builtins[builtins.index("pub(crate) unsafe fn memoryview_gather_bytes"):]
print("read_hash_buffer copies acquired buffer contents")
print("buffer_bytes gathers memoryview contents into Vec<u8>")
print("all BLAKE2 key/salt/person buffers are materialized before size checks")
PYRepository: youknowone/pyre
Length of output: 327
Reject oversized BLAKE2 parameter buffers before copying them.
Use a length-only buffer view for key, salt, and person before read_hash_buffer copies them. A large memoryview can otherwise allocate its full contents and abort on OOM instead of returning ValueError.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/module/_hashlib/mod.rs` around lines 856 - 875,
Update the BLAKE2 parameter handling around the max_key_size, salt_size, and
person_size checks to obtain length-only buffer views for key, salt, and person
before calling read_hash_buffer. Validate each view’s length against its
corresponding limit first, then copy only validated buffers so oversized
memoryviews return ValueError without allocating their full contents.
Source: MCP tools
| unsafe { pyre_object::w_dict_getitem_str(w_globals, "__builtins__") } | ||
| .unwrap_or(pyre_object::PY_NULL) | ||
| }; | ||
| emit_builtins_cell_fold(ctx, op_pc, dst, dst_bank, w_globals, w_builtin, &name) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Decline the builtin fold when the authoritative globals differ.
If live_globals differs from w_globals, module_dict_cell_slot_direct cannot prove that an ordinary dictionary lacks name. The call then pins and checks w_globals, but the residual lookup reads live_globals. If that dictionary contains name, the trace returns the builtin value instead of the global value.
Return Ok(false) whenever live_globals is null or differs from w_globals. Alternatively, perform and guard an exact lookup against live_globals.
Proposed fix
- if !live_globals.is_null()
- && live_globals as usize != w_globals as usize
- && crate::state::module_dict_cell_slot_direct(live_globals, &name).is_some()
- {
+ if live_globals.is_null() || live_globals as usize != w_globals as usize {
return Ok(false);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| emit_builtins_cell_fold(ctx, op_pc, dst, dst_bank, w_globals, w_builtin, &name) | |
| if live_globals.is_null() || live_globals as usize != w_globals as usize { | |
| return Ok(false); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs` at line 13171, Update
the builtin-fold path around emit_builtins_cell_fold to return Ok(false) when
live_globals is null or does not equal w_globals, preventing specialization
against a non-authoritative dictionary. Preserve the existing fold only when
both references identify the same globals dictionary.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 0e837d4). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
cc4f22d to
8baaf87
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8baaf874c4
ℹ️ 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".
| let keys: Vec<rustpython_wtf8::Wtf8Buf> = unsafe { pyre_object::w_dict_str_entries_wtf8(dict) } | ||
| .into_iter() | ||
| .map(|(name, _)| name) | ||
| .collect(); |
There was a problem hiding this comment.
When a program stores a finalizable object as globals()[1] = C() or module.__dict__[1] = C(), w_dict_str_entries_wtf8 omits that binding, so this teardown never replaces it with None. The modules remain pinned through the last collection in clear_shutdown_modules—and native modules such as __main__ retain permanently rooted dictionaries—so the object stays reachable and its __del__ is never run before process::exit. The shutdown walk must also release non-string-keyed values, or perform a collection after the module roots are actually released.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
| if salt.len() > salt_size { | ||
| return Err(crate::PyError::value_error(format!( | ||
| "maximum salt length is {salt_size} bytes" | ||
| ))); | ||
| } | ||
| if person.len() > person_size { | ||
| return Err(crate::PyError::value_error(format!( | ||
| "maximum person length is {person_size} bytes" | ||
| ))); |
There was a problem hiding this comment.
Moving these size checks into _blake2_new means they run only after the app-level wrapper has validated fanout, depth, and the remaining tree parameters. For example, hashlib.blake2b(salt=b'x' * 17, fanout=256) now reports the fanout error, whereas the prior PyPy-shaped implementation and CPython report the oversized-salt error; the same regression occurs for person combined with invalid fanout or depth. Keep byte-accurate buffer validation, but perform it at the original point before the later range checks so exception precedence remains compatible.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
`try_walker_load_global_cell_fold` folds a name that misses the module dict and resolves through `get_builtin().getdictvalue`. `try_walker_load_name_cell_fold` ended at `emit_module_dict_cell_fold`, so module-scope `LOAD_NAME` of such a name residualized `bh_load_name_fn` on every iteration. Move that leg into `emit_builtins_cell_fold` and call it from both folds. The guard sequence is unchanged: the name must be absent from the module dict, whose `version?` is pinned so a later shadowing insert fails GUARD_NOT_INVALIDATED, and `emit_namespace_cell_fold` pins the builtins dict's own `version?`. A 2.4M-iteration module-scope `total + len(s)` loop compiles to 21 ops with no `call_may_force`, matching the same loop with `len` bound to a module global; it recorded 30 ops and one `call_may_force` before. Assisted-by: Claude
The hot loop reads `len` at module scope, where the name misses the module dict and resolves through the frame's builtin module. The trailing `len = lambda x: 100` plus a second loop pins the invalidation: the module dict `version?` bump has to be seen, so the second loop prints 40000000. Output matches CPython and PyPy. Ceiling 8 against measured 1.6x dynasm, 2.1x cranelift, 1.9x wasm. The pypy denominator sits near the execution floor, so the ratios moved by about a quarter between runs; the residual form this gates measured about 140x. The shape is load-bearing: inside a function the read compiles to LOAD_GLOBAL, which folded already, and a module-scope `del` of a global drops `mc_entered` to 0 and runs the loop interpreted. Assisted-by: Claude
`finalize_runtime` ended after the `ObjSpace.finish()` phases, so nothing held only by a namespace was finalized: `struct.x = C()` whose class defines `__del__` printed nothing at exit, and a `__main__` global's `__del__` did not run either. Restore `collect_and_run_finalizers`, `release_frees_nothing` and the `__main__` newest-to-oldest release loop that #1158 removed. A `__del__` reading a module global needs a collection to run while the remaining names are still bound; `test_start_new_thread_at_finalization` reads `_thread` and otherwise sees `None`. Port `finalize_modules` / `_PyModule_ClearDict` for the namespaces `__main__` does not reach. `release_sys_modules_for_shutdown` snapshots `sys.modules` in insertion order and detaches every entry except `sys` and `builtins`, which the unraisable path still reads while the released modules are finalized. `clear_shutdown_modules` then walks the snapshot newest-first, skipping those two, and clears each module dict in two name passes -- a single leading underscore first, then every name but `__builtins__` -- assigning `None` rather than deleting. One collection follows the whole walk: a sweep per module costs a full mark-and-sweep for each of the ~100 modules a bare `import unittest` loads, which measured 905ms of teardown against 39ms for the single sweep, and `test_regrtest` spends it once per subprocess. `test.test_struct` and `test.test_threading` return to PASS in the baseline; `test_struct_cleans_up_at_runtime_shutdown` and `test_start_new_thread_at_finalization` are the tests they cover. Assisted-by: Claude
guard_failures 659 -> 658. Assisted-by: Claude
The suite ran twice per CI round: once inside `pyre/check.py` on the macos-latest leg, and once in the dedicated `cpython-tests` job. It dominates this script's wall time, so make it opt-in -- `--cpython-suite` replaces `--no-cpython-suite` and no CI job passes it, leaving the dedicated job as the only place CI pays for the run. The stage itself is unchanged and still skips off darwin-arm64. Its docstring no longer claims the CI job pins `runs-on: macos-latest`, which `ci: make the CPython gate host-aware` changed to ubuntu-24.04. Assisted-by: Claude
8baaf87 to
0e837d4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e837d457e
ℹ️ 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".
| // reads a global reaches its own already-cleared namespace either way, and | ||
| // it costs a full mark-and-sweep for each of the ~100 modules a bare | ||
| // `import unittest` loads. | ||
| collect_and_run_finalizers(ec_ptr); |
There was a problem hiding this comment.
When sys.modules contains a managed ModuleType subclass whose only remaining reference is the import cache, the snapshot roots every module at line 1261 and those roots are still active during this final collection. Consequently the module itself cannot become unreachable, and after _roots is dropped the function returns without another collection, so its user-defined __del__ never runs before process exit. Release the temporary module roots before performing the final sweep while retaining whatever core roots shutdown still needs.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
…erves #1187 re-recorded the shared `str_fstring.cranelift` baseline 659 -> 658. No `linux` overlay exists, so the ubuntu leg reads that shared file, and it observes 659 — its check.py leg on #1187 was already red on this line and has stayed red on main since, including at d953ddc. The shared file goes back to 659 and darwin's 658 moves into `str_fstring.cranelift.darwin.jitstats`. The `darwin.github-actions` and `win32.github-actions` overlays already carry 658 and are unchanged, so the macos and windows legs read the same value as before. Assisted-by: Claude
… observes `str_fstring` reads a different `guard_failures` on each host, and the overlays no longer matched. Both backends are now keyed the same way: the shared file carries what linux and windows observe, and darwin gets its own overlay. cranelift linux 659 windows 658 darwin 658 dynasm linux 658 windows 658 darwin 659 For cranelift the shared file goes back to 659, undoing #1187's re-record — no `linux` overlay exists, so the ubuntu leg reads that file and observes 659; #1187's own ubuntu leg was already red on this line and main has stayed red since, including at df365f9. darwin's 658 moves into a `.darwin` overlay beside the existing `darwin.github-actions` and `win32.github-actions` ones, which already read 658 and are unchanged. For dynasm the shared file becomes 658 and the `win32.github-actions` overlay is dropped, since windows observes the same 658; darwin's 659 moves into a `.darwin` overlay. main fails this row on both its ubuntu and windows legs at df365f9 with the identical `guard_failures 659 -> 658`. Assisted-by: Claude
… observes `str_fstring` reads a different `guard_failures` on each host, and the overlays no longer matched. Both backends are now keyed the same way: the shared file carries what linux and windows observe, and darwin gets its own overlay. cranelift linux 659 windows 658 darwin 658 dynasm linux 658 windows 658 darwin 659 For cranelift the shared file goes back to 659, undoing #1187's re-record — no `linux` overlay exists, so the ubuntu leg reads that file and observes 659; since, including at df365f9. darwin's 658 moves into a `.darwin` overlay beside the existing `darwin.github-actions` and `win32.github-actions` ones, which already read 658 and are unchanged. For dynasm the shared file becomes 658 and the `win32.github-actions` overlay is dropped, since windows observes the same 658; darwin's 659 moves into a `.darwin` overlay. main fails this row on both its ubuntu and windows legs at df365f9 with the identical `guard_failures 659 -> 658`. Assisted-by: Claude
…1188) * jit: carry NotVirtualStateInfoPtr.lenbound on the virtual-state pointer leaf NotVirtualStateInfoPtr.__init__ (virtualstate.py:508-518) records info.getlenbound(None).widen() on every non-virtual pointer leaf, and _generate_guards (virtualstate.py:529-537) compares it before dispatching on LEVEL_NONNULL / LEVEL_KNOWNCLASS / the base level, raising VirtualStatesCantMatch("length bound does not match") when the incoming bound is not within the expected range. An incoming leaf with no bound is treated as IntBound.nonnegative(). pyre exported PtrInfo::Array and PtrInfo::Str as a bare NonNull leaf, so the bound was dropped at export and no arm of generate_guards_for_entry_recursive compared one. Store it on VirtualStateInfoNode, which already carries the other per-instance NotVirtualStateInfo attributes (position, position_in_notvirtuals), rather than widening the VirtualStateInfo variants. Populate it at export for the four pointer levels and preserve it across clone and deep_clone_node. The single comparison site sits where upstream puts it, after the force-to-virtual branch and the incoming-is-virtual rejection and before the level dispatch; both VirtualState::generalization_of and VirtualState::generate_guards route through that function, matching virtualstate.py:636-644 reaching the check through the same generate_guards. Moves nothing on the bench corpus: an A/B over check.py --backend dynasm with and without the change reported 419/419 passed on both arms. Full gate green at dynasm 420/420, cranelift 419/419, wasm 414/414. Assisted-by: Claude * jit: publish quasi-immutable deps and the invalidation flag on every compiled artifact record_loop_or_bridge (compile.py:204-207) registers a trace's quasi_immutable_deps against its loop token unconditionally, and it runs for every compiled loop and bridge. pyre cannot register at that depth (the dependency target is a pyre-interpreter watcher and majit-metainterp sits below the pyre crates), so it stages onto last_quasi_immutable_deps plus last_compiled_artifact_invalidation_flag and drains both from register_quasi_immutable_deps in eval.rs. That drain takes the deps first and then returns early when the flag is None, so any path that stages one without the other silently discards the dependencies. Three paths never reached it: - compile_entry_bridge published neither the deps nor the flag. - compile_simple_loop assigned the deps but never the flag, so the drain bailed. - handle_fail called the drain from three of the four BridgeResolution arms; ResumeBlackhole was an empty block, and a bridge that compiled and attached still resolves that way — call_jit.rs returns it on the CALL_ASSEMBLER ca-finish-noreplay path. Hoist the single call out of the match so it runs for every resolution, matching record_loop_or_bridge running for every compiled bridge regardless of what the interpreter does next. A compiled artifact whose deps never reached a watcher has a GUARD_NOT_INVALIDATED watching an AtomicBool on nobody's list: a later _version_tag or dict-version bump does not arm it and the artifact keeps returning the pre-mutation constant. Add MC_DIAG slots 75-77 (qmut_deps_simple_loop, qmut_deps_entry_bridge, qmut_deps_blackhole_arm), each bumped only when that path publishes a non-empty dependency list. Over 427 bench files under MAJIT_STATS: entry_bridge 10 across 10 benches, simple_loop 0, blackhole_arm 0 — so the entry-bridge path was dropping dependencies in practice and the other two are latent. Gate green: dynasm 422/422, cranelift 421/421, wasm 416/416. Assisted-by: Claude * jit: decay the jitcounter periodically, not only when a bound is reached JitCounter.__init__ (counter.py:104-121) installs invoke_after_minor_collection into translator._jit2gc, and the GC transformer reads it back out as finished_minor_collection (framework.py:135-138), so it runs after every minor collection and calls decay_all_counters() on every 32nd. decay_all_counters (counter.py:266-278) states the purpose: "to gradually decay counters that didn't reach their maximum. Thus if a counter is incremented very slowly, it will never reach the maximum. This avoids altogether the JIT compilation of rare paths." pyre ported decay_all_counters and calls set_decay(40), so decay_by_mult is 0.96 in production, but only the warmstate.py:429 bound_reached call site was wired. Once every hot key is compiled, bound_reached stops firing and the counters are effectively monotonic, so a guard that fails sparsely still accumulates to the bound and gets a bridge. majit-gc gains a fn() hook invoked after minor_collections is incremented; majit-trace registers it from JitCounter::new and keeps the 32-step count in module statics. The hook body only touches two atomics: it runs inside a minor collection, and reaching the counter table from the collector would re-enter a borrow the GC does not hold. The decay is applied instead at the top of the next JitCounter::tick. That is a deferral, not upstream's synchronous application; the counters are only read at tick time, so the deferral is observationally equivalent rather than identical. Re-record guard_failures on six synth benches across the backends: arith_int_bool 2214->2219 (dynasm, cranelift), exception_traceback_loop_forms 811->812 (dynasm, cranelift), generator_tree_recursion 1240->1243 (dynasm) and ->1241 (cranelift), inline_chain_depth_typeflip 3818->3820 (all three), binary_int_overflow_local_resume 647->651 (wasm), recursion_memo_branch 4703->4728 (wasm). loops_compiled and bridges_compiled are unchanged on every one of them: the decayed counters reach the bound later, so the same bridges are compiled after more guard failures have accumulated. Nothing on this corpus is rare enough for the decay to suppress a compilation outright. Gate green: dynasm 422/422, cranelift 421/421, wasm 416/416. Assisted-by: Claude * jit: drain quasi-immutable deps on the CALL_ASSEMBLER bridge path, and start every bridge attempt without an artifact flag `register_quasi_immutable_deps` is the only drain of `last_quasi_immutable_deps`. It ran from the loop path and from the general guard-failure path in `handle_fail`, but not from the two CALL_ASSEMBLER callers of `trace_and_compile_from_bridge` in call_jit.rs, so a bridge compiled through either of those staged its dependencies and no watcher received the invalidation flag. `trace_and_compile_from_bridge` now clears `last_compiled_artifact_invalidation_flag` on entry; only `compile_bridge` cleared it, so an attempt that gave up before reaching `compile_bridge` left the previous compilation's flag in place for the drain to attach to. Assisted-by: Claude * bench: re-record the jit-stats baselines this branch moves `arith_int_bool` guard_failures reads 2214 on every platform now, so the shared files carry it and the darwin override is gone. `class_body_exec_hot_loop` no longer compiles its bridge on dynasm and cranelift; wasm still does. Assisted-by: Claude * bench: split str_fstring's jit-stats baselines by the value each host observes `str_fstring` reads a different `guard_failures` on each host, and the overlays no longer matched. Both backends are now keyed the same way: the shared file carries what linux and windows observe, and darwin gets its own overlay. cranelift linux 659 windows 658 darwin 658 dynasm linux 658 windows 658 darwin 659 For cranelift the shared file goes back to 659, undoing #1187's re-record — no `linux` overlay exists, so the ubuntu leg reads that file and observes 659; since, including at df365f9. darwin's 658 moves into a `.darwin` overlay beside the existing `darwin.github-actions` and `win32.github-actions` ones, which already read 658 and are unchanged. For dynasm the shared file becomes 658 and the `win32.github-actions` overlay is dropped, since windows observes the same 658; darwin's 659 moves into a `.darwin` overlay. main fails this row on both its ubuntu and windows legs at df365f9 with the identical `guard_failures 659 -> 658`. Assisted-by: Claude
…and a whole-heap collection per released `__main__` global (#1505) * jit: decline bridge-tracing frames before install_current_frame `install_current_frame` and `ExecutionContext::enter` both link a frame into the `topframeref`/`f_backref` chain, and `execute_frame_plain` reaches `enter` through `eval_frame_plain_with_resume`. The bridge-tracing decline sat between the two calls, so the frame was linked twice and the second link made `f_backref` name the frame itself; `walk_pyframe_roots` follows `f_backref` with no cycle guard. Move the decline above `install_current_frame`, where every other decline in the function already sits, and add a `debug_assert_ne!` in `enter` for the same pairing. Assisted-by: Claude * function: barrier the old-gen Function after its initializing write `function_new_impl` publishes every field with one `ptr::write`, which no write barrier sees, so an old-gen Function that never joins the remembered set is not scanned by a minor collection. Call `function_write_barrier` after the write. Assisted-by: Claude * gate-triage: add the PYRE_GC_SIZE_AUDIT row `every_live_gate_has_a_triage_entry` reports it: the gate is read from the environment in `majit-gc/src/collector.rs` and had no entry in `pyre/gate-triage.md`. List it in §6c and say what it checks. Assisted-by: Claude * shutdown: stop collecting once per released `__main__` global `finalize_runtime` releases `__main__`'s globals newest-to-oldest and called `collect_and_run_finalizers` after every name whose value was not an exact scalar or a `sys.modules`-registered module. `release_frees_nothing` answers `false` for every function, class and container, so that was a whole-heap mark-and-sweep per binding: `PYRE_GC_DIAG` reports `major` = names + 3, and a script binding 400 globals spent 3015ms against pypy3's 54ms. `runpy. _run_module_as_main` execs a module's code into the real `__main__` dict, so `pyre -m inspect sys` walked `inspect`'s 182 names -- 1854ms, of which the Python work is 104ms. `test.test_inspect` pays it in four subprocesses. The collection is there to *find* a finalizer, so replace it with two O(1) questions where the collection is O(heap). `gc_has_pending_finalizers` asks whether the collector owes any delivery at all: `deal_with_objects_with_ finalizers` is the one pass that hands control back to the program, and `rawrefcount` answers for the whole of itself. `gc_object_finalizer_pending` asks whether the released object is one it owes, reading the `FINALIZER_REGISTERED` bit `register_finalizer` sets -- the whole of `hasuserdel`, plus the `_io`, coroutine and weakref-lifeline registrations whose types carry no such flag. The destructor lists are deliberately not consulted. A destructor is RPython's light finalizer and may not run interpreter code, so where in the walk it runs is not observable; in pyre they are drop glue (`function_destructor`, `type_object_destructor`, `pycode_destructor`, `storage_box_destructor`), which puts every function, type and code object on them and makes a predicate that counted them answer non-zero in every program. `major` for 400 globals 403 -> 3, 3015ms -> 115ms; `pyre -m inspect sys` 1854ms -> 179ms; `test.test_inspect` 7.56s -> 3.17s with its 362 tests still passing. `test_start_new_thread_at_finalization` and `test_struct_cleans_up_at_runtime_shutdown`, the two tests #1187 restored this loop for, still pass, as do the `__del__`-ordering and unclosed-file-flush shapes. Releasing a *container* of a finalizable is where this stops being exact: no O(1) test on the container sees the object it frees, so that `__del__` runs at the walk's trailing collection and reads `None` for the globals released after it. It still runs. `PYRE_GC_DIAG` gains `teardown_released`/`teardown_swept` so a run that falls back to a sweep per name is visible without a rebuild. Assisted-by: Claude * jit: keep an operand store out of the virtualizable identity slot `virtualizable_boxes` is `[static fields][array slots][identity]`, and the trailing identity entry is the box `_nonstandard_virtualizable` compares every vable op's frame against. STORE_DEREF's codewriter lowering read the cell with `emit_load_fast_ref!` -- the LOAD_FAST opcode, a `getarrayitem_vable_r` plus a `pushvalue` -- while the value it is about to pop still held the top of the stack, so the scratch push addressed one slot past `co_stacksize`, which is that identity entry. A class body reaches it with no headroom. `class C: def m(self): pass` compiles to `co_stacksize=1` with a single `__classdict__` cellvar, so `locals_cells_stack_w` is 2 slots and `STORE_DEREF __classdict__` runs with the `LOAD_LOCALS` result on the stack: the push lands on flat index 7 of an 8-slot shadow. The trace then records `PtrEq(frame, cell)` + `GuardFalse`, declares the real frame nonstandard, and writes `PyFrame.last_instr`, the force token and the exit flush into the cell. The same class statement in a loop past the function-entry threshold dies reading the cell's `locals_cells_stack_w` at `obj - 4` inside the write-barrier fast path -- `EXC_BAD_ACCESS` at 0xfffffffffffffffc, one unsymbolicated frame. `emit_read_local_ref!` emits the read alone, which is what `pyopcode.py` STORE_DEREF's `cells[varindex]` is. The six other `emit_load_fast_ref!` + pop scratch reads are net-push or run at depth 0, so their slot is in range. Two bounds close the class rather than the one instance. `vable_array_flat_index` restores `_get_arrayitem_vable_index`'s `assert 0 <= index < get_array_length` as a `None` return -- the read paths already fall back to a heap `GETARRAYITEM_GC_*` and the write path reports `VableArrayStore::OutOfVable` -- and `set_virtualizable_entry_at` refuses the identity slot outright. `write_stack_slot` / `swap_stack_slots` bound at `len - 1`: the existing `>= len` test was one slot too loose and passed exactly this store through. The regression test scores the exit status. The crash prints nothing on either stream, so a harness reading output calls it a pass. Assisted-by: Claude
1 participant
Two independent pieces of work, plus the shutdown teardown they uncovered.
jit: fold the builtins fallback for module-scopeLOAD_NAMEtry_walker_load_global_cell_foldalready folded a name that misses the moduledict and resolves through
get_builtin().getdictvalue.try_walker_load_name_cell_foldstopped atemit_module_dict_cell_fold, so amodule-scope
LOAD_NAMEof a builtins-only name residualizedbh_load_name_fnon every iteration. The shared leg now lives in
emit_builtins_cell_foldandboth folds call it; the guard sequence is unchanged.
The discriminator was not the call but the lookup: binding the same builtin to a
module global (
_len = len) erased the residual. Inside a function the readcompiles to
LOAD_GLOBALand already folded, so the gap was module-scope only.total + len(s)call_may_force=1call_may_force=0_len = lencontrolFaithfulness: at module scope
load_name_valueskips the locals probe and callsload_global_value, i.e. globals thenget_builtin().getdictvalue— thepyopcode
LOAD_NAME→_load_globalchain. A non-module globals is refused bywalker_pin_namespace_version, which is what closes the "a plain dict slotlookup returns None, meaning absent" trap.
bench/synth: gate itThere was no test anywhere for either fold.
load_name_builtin_cell_fold.pyruns the hot loop at module scope and then rebinds
len, so the module dictversion?bump has to invalidate the folded loop. Ceiling 8 against measured1.6x dynasm / 2.1x cranelift / 1.9x wasm; the residual form this gates measured
about 140x, and the pypy denominator sits near the execution floor so the ratios
moved by about a quarter between runs.
The shape is load-bearing and is documented in the file: inside a function the
read compiles to
LOAD_GLOBAL, and a module-scopedelof a global dropsmc_enteredto 0 and runs the loop interpreted.interpreter: clear module globals at shutdownfinalize_runtimereleased only__main__'s globals, so an object stored inanother module's namespace was never finalized —
struct.x = C()with a__del__printed nothing at exit. This isfinalize_modules/_PyModule_ClearDict: snapshotsys.modulesin insertion order, detach everyentry except
sysandbuiltins(the unraisable path reads their live dictswhile released modules are finalized), then walk newest-first and clear each
module dict in two name passes — single leading underscore, then every name but
__builtins__— assigningNonerather than deleting.Keeping
__builtins__bound, the underscore-first pass, the reverse importorder and skipping
sys/builtinsare the upstream mitigation for "clearing amodule invalidates globals a pending callback needs"; no extra guard was added.
An earlier attempt cleared only values whose type carried
hasuserdeland wasremoved in 55e9816, which recorded
test.test_structas FAIL. That baselineentry returns to PASS. Note that
FAILis module-granular, so it had alsoungated every other test in that module.
Verification
cargo fmt --checkcleancargo test --all --no-default-features --features dynasm— 7790 passed, 0 failedpyre/cpython_tests/run.py --backend dynasm— 209 PASS, 0 FAIL, no regressionspyre/check.py --synthetic-only --backend dynasm,cranelift,wasmon the new bench — 3/3 backends passmid-loop,
builtinsrebind, unshadow viadel,del builtins.abs→NameError,globals()[...]insert, class body inside the loop, and__builtins__rebound to a dict.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests