Tuple Support and WIP Object Support in JIT - #8188
Tuple Support and WIP Object Support in JIT#8188LuigiTheJokesterAlvarez wants to merge 16 commits into
Conversation
Sorry, something went wrong.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting. Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe JIT now treats tuples as heap-backed object values, threads a tuple allocator through compilation, and updates ABI, local-variable handling, tuple construction, and unpacking to use the new object shape. JIT tuple annotations and snippet coverage were also extended. ChangesHeap-backed JIT Tuple Objects
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: youknowone Sequence Diagram(s)sequenceDiagram
participant Jit
participant build_function
participant FunctionCompiler
participant jit_alloc_tuple
participant AbiValue
participant libffi
Jit->>Jit: import jit_alloc_tuple
build_function->>Jit: declare alloc_func_ref
build_function->>FunctionCompiler: new(..., alloc_func_ref)
FunctionCompiler->>jit_alloc_tuple: call(len)
FunctionCompiler->>AbiValue: create Object(usize)
AbiValue->>libffi: marshal pointer argument
libffi->>AbiValue: return object payload
❌ Failed checks (1 warning)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)crates/jit/src/instructions.rs (1)55-60: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve tuple shape for object arguments before supporting tuple unpacking there.
JitType::Object arguments are materialized as ObjectKind::Opaque, but UnpackSequence only accepts ObjectKind::Tuple. A JIT function taking a tuple argument and unpacking it will reject its own object ABI value despite the new tuple support.
Also applies to: 878-884
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/jit/src/instructions.rs` around lines 55 - 60, In from_type_and_value, JitType::Object is currently always wrapped as ObjectKind::Opaque, which breaks tuple arguments that later flow into UnpackSequence. Update the object-argument materialization path in from_type_and_value so tuple-shaped values are preserved as ObjectKind::Tuple when the ABI value represents a tuple, and keep UnpackSequence aligned to accept that preserved shape instead of rejecting the function’s own argument value.
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 `@crates/jit/src/instructions.rs`: - Around line 155-161: The changed Rust code needs rustfmt cleanup, with several signatures and calls in functions like local_to_value and other touched Rust sections needing standard formatting and trailing whitespace removed. Run cargo fmt on the affected code so the updated blocks follow the default Rust style consistently across the modified instructions and nearby changed lines. - Around line 429-435: The return ABI handling in build_function/instructions.rs is being mutated too late and can diverge across branches, so make the return signature fixed before any return emission and enforce that all subsequent returns match the first inferred ret_ty. Update the logic around self.sig.ret and builder.func.signature.returns so the signature is established once, then validate or reject mixed return types in the return-building path rather than appending/changing ABI params later. In `@crates/jit/src/lib.rs`: - Around line 41-50: Add a matching deallocation path for the heap tuples allocated by jit_alloc_tuple, since BuildTuple and tuple constants currently return raw allocations with no owner or free. Introduce a clear ownership strategy in crates/jit/src/lib.rs, and ensure the JIT runtime exposes a corresponding free/release function or otherwise transfers ownership so tuple allocations are reclaimed after use. --- Outside diff comments: In `@crates/jit/src/instructions.rs`: - Around line 55-60: In from_type_and_value, JitType::Object is currently always wrapped as ObjectKind::Opaque, which breaks tuple arguments that later flow into UnpackSequence. Update the object-argument materialization path in from_type_and_value so tuple-shaped values are preserved as ObjectKind::Tuple when the ABI value represents a tuple, and keep UnpackSequence aligned to accept that preserved shape instead of rejecting the function’s own argument value.
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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e9eeb398-7e6d-4522-bc07-630d242533be
📥 CommitsReviewing files that changed from the base of the PR and between 0009dd6 and bdf64b7.
📒 Files selected for processing (3)- crates/jit/src/instructions.rs
- crates/jit/src/lib.rs
- crates/vm/src/builtins/function.rs
Sorry, something went wrong.
| extern "C" fn jit_alloc_tuple(len: i64) -> i64 { | ||
| let layout = Layout::array::<i64>(len as usize + 1).unwrap(); | ||
| let ptr = unsafe { alloc(layout) }; | ||
| if ptr.is_null() { | ||
| handle_alloc_error(layout); | ||
| } | ||
| unsafe { | ||
| *(ptr as *mut i64) = len; | ||
| } | ||
| ptr as i64 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Add an ownership/freeing strategy for heap tuples.
jit_alloc_tuple allocates with the global allocator and returns a raw pointer, but this PR adds no matching deallocation path. Since BuildTuple and tuple constants call this allocator at runtime, repeated JIT invocations leak every tuple allocation.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/jit/src/lib.rs` around lines 41 - 50, Add a matching deallocation path for the heap tuples allocated by jit_alloc_tuple, since BuildTuple and tuple constants currently return raw allocations with no owner or free. Introduce a clear ownership strategy in crates/jit/src/lib.rs, and ensure the JIT runtime exposes a corresponding free/release function or otherwise transfers ownership so tuple allocations are reclaimed after use.
Sorry, something went wrong.
ShaharNaveh
left a comment
There was a problem hiding this comment.
Hi, thanks for contributing to RustPython:)
Can you please check the AI policy? (#8188 (comment))
Sorry, something went wrong.
| @@ -1,6 +1,8 @@ | |||
| // instructions.rs | |||
There was a problem hiding this comment.
What's the reasoning for this comment?
Sorry, something went wrong.
There was a problem hiding this comment.
when i forget what im doing i comment it out to make myself know again what im doing
yeah it's weird
Sorry, something went wrong.
There was a problem hiding this comment.
ill delete unnecessary comments and if i can right now, document the functions i did, and making the changes coderabbit requested
Sorry, something went wrong.
There was a problem hiding this comment.
i did documentation yesterday lol
Sorry, something went wrong.
| @@ -1,7 +1,9 @@ | |||
| // lib.rs | |||
There was a problem hiding this comment.
same
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)crates/jit/src/instructions.rs (1)382-406: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Heap tuple allocations need a reclamation path crates/jit/src/instructions.rs:382-406
build_heap_tuple allocates raw memory via jit_alloc_tuple, but nothing in the JIT frees those buffers. CompiledCode::drop only releases the compiled module, not tuple storage. Since both LoadConst tuples and BuildTuple hit this path, repeated execution will leak one allocation per tuple unless ownership is transferred to the VM or a matching free path is added.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/jit/src/instructions.rs` around lines 382 - 406, The heap tuple allocation in build_heap_tuple currently has no matching reclamation path, so tuple buffers allocated through alloc_func will leak across repeated execution. Update the JIT tuple lifecycle around build_heap_tuple, TupleShape, and CompiledCode::drop so ownership of these allocations is clearly transferred to the VM or a corresponding free is registered and invoked when the compiled result is released. Make sure both LoadConst tuples and BuildTuple allocations are covered by the same cleanup path.
crates/vm/src/builtins/function/jit.rs (1)🤖 Prompt for all review comments with AI agentscrates/jit/src/instructions.rs (1)55-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Error message is now stale — tuple is accepted but not mentioned.
The error message still says "Jit requires argument to be either int, float or bool"-style text, but the branch above now also accepts tuples. Update the message so it doesn't mislead users about supported argument types.
📝 Proposed fix🤖 Prompt for AI AgentsErr(new_jit_error( - "Jit requires argument to be either int, float or bool".to_owned(), + "Jit requires argument to be either int, float, bool or tuple".to_owned(), vm, ))Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vm/src/builtins/function/jit.rs` around lines 55 - 63, The fallback error in JIT argument validation is stale because the `jit` type check now also accepts tuples via `value.is(vm.ctx.types.tuple_type)`. Update the `new_jit_error` message in `jit.rs` to include tuple among the supported argument types, keeping the message aligned with the branches in the JIT type-detection logic.616-622: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Deduplicate the tuple JitValue::Object construction. The JitValue::Object(ptr, Rc::new(ObjectKind::Tuple(shape))) wrapping is identical here and in prepare_const (Lines 374-376). Consider having build_heap_tuple return the finished JitValue (or a small helper) so the wrapping lives in one place. As per coding guidelines: "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code."
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/jit/src/instructions.rs` around lines 616 - 622, The tuple JitValue::Object construction is duplicated in Instruction::BuildTuple and prepare_const, so move the wrapping into one shared place. Update build_heap_tuple to return the finished JitValue, or add a small helper used by both build_heap_tuple and prepare_const, and have Instruction::BuildTuple and prepare_const call that shared logic instead of rebuilding JitValue::Object(ptr, Rc::new(ObjectKind::Tuple(shape))) in multiple places.Source: Coding guidelines
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 `@crates/vm/src/builtins/function/jit.rs`:
- Around line 57-58: The conditional in the JIT function implementation is
formatted incorrectly, with the closing brace and else split across separate
lines. Update the `jit.rs` block around the `Function`/`jit` logic so it follows
rustfmt style and uses a single-line `} else {` form consistent with `cargo
fmt`.
---
Outside diff comments:
In `@crates/jit/src/instructions.rs`:
- Around line 382-406: The heap tuple allocation in build_heap_tuple currently
has no matching reclamation path, so tuple buffers allocated through alloc_func
will leak across repeated execution. Update the JIT tuple lifecycle around
build_heap_tuple, TupleShape, and CompiledCode::drop so ownership of these
allocations is clearly transferred to the VM or a corresponding free is
registered and invoked when the compiled result is released. Make sure both
LoadConst tuples and BuildTuple allocations are covered by the same cleanup
path.
---
Nitpick comments:
In `@crates/jit/src/instructions.rs`:
- Around line 616-622: The tuple JitValue::Object construction is duplicated in
Instruction::BuildTuple and prepare_const, so move the wrapping into one shared
place. Update build_heap_tuple to return the finished JitValue, or add a small
helper used by both build_heap_tuple and prepare_const, and have
Instruction::BuildTuple and prepare_const call that shared logic instead of
rebuilding JitValue::Object(ptr, Rc::new(ObjectKind::Tuple(shape))) in multiple
places.
In `@crates/vm/src/builtins/function/jit.rs`:
- Around line 55-63: The fallback error in JIT argument validation is stale
because the `jit` type check now also accepts tuples via
`value.is(vm.ctx.types.tuple_type)`. Update the `new_jit_error` message in
`jit.rs` to include tuple among the supported argument types, keeping the
message aligned with the branches in the JIT type-detection logic.
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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e4d2b7ab-8b30-431a-b98b-975a72432d63
📥 CommitsReviewing files that changed from the base of the PR and between f32149d and 661b659.
📒 Files selected for processing (3)- crates/jit/src/instructions.rs
- crates/jit/src/lib.rs
- crates/vm/src/builtins/function/jit.rs
- crates/jit/src/lib.rs
Sorry, something went wrong.
|
I will fix the leak with tuple alloc later |
Sorry, something went wrong.
There was a problem hiding this comment.
extra_tests/snippets/jit.py (1)🤖 Prompt for all review comments with AI agents24-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Variable name violates PEP 8 (snake_case).
tupleId should be tuple_id per PEP 8 naming conventions for local variables.
As per coding guidelines, **/*.py: "Follow PEP 8 style for custom Python code".
🎨 Proposed fix🤖 Prompt for AI Agents- tupleId = (1, 2) - assert tuple_identity(tupleId) == tupleId + tuple_id = (1, 2) + assert tuple_identity(tuple_id) == tuple_idVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extra_tests/snippets/jit.py` at line 24, Rename the local variable tupleId to tuple_id in jit.py to comply with PEP 8 snake_case naming; update any nearby references in the same snippet or function so the new identifier is used consistently.Source: Coding guidelines
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Nitpick comments: In `@extra_tests/snippets/jit.py`: - Line 24: Rename the local variable tupleId to tuple_id in jit.py to comply with PEP 8 snake_case naming; update any nearby references in the same snippet or function so the new identifier is used consistently.
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 01ba0ae0-098c-406f-9653-2d60610b3f24
📥 CommitsReviewing files that changed from the base of the PR and between 661b659 and c12d905.
📒 Files selected for processing (2)- crates/vm/src/builtins/function/jit.rs
- extra_tests/snippets/jit.py
- crates/vm/src/builtins/function/jit.rs
Sorry, something went wrong.
|
I have temporarily disabled the while-loop JIT test due to incorrect loop lowering causing infinite execution, i will re-enable it once SSA φ-node based loop construction is implemented |
Sorry, something went wrong.
|
Hi! i am sorry for late response.
I think the leak must be fixed. |
Sorry, something went wrong.
Reviewers
Requested changes must be addressed to merge this pull request.
Summary
This changes the JIT to support tuples and possibly, objects
Summary by CodeRabbit
New Features
Bug Fixes
Tests