X Tutup
Skip to content

Separate WeakRefList from ObjExt prefix#7365

Merged
youknowone merged 1 commit intoRustPython:mainfrom
youknowone:managed-weakref
Mar 8, 2026
Merged

Separate WeakRefList from ObjExt prefix#7365
youknowone merged 1 commit intoRustPython:mainfrom
youknowone:managed-weakref

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Mar 5, 2026

Summary

  • Split the monolithic ObjExt prefix into two independent prefixes: ObjExt (dict + slots) and WeakRefList (weakref support)
  • Objects needing only weakref no longer allocate unnecessary dict/slots space
  • Add MANAGED_WEAKREF flag (1 << 3) matching CPython's Py_TPFLAGS_MANAGED_WEAKREF

Memory layout

Without prefix: [PyInner<T>]
Dict only:      [ObjExt][PyInner<T>]
Weakref only:   [WeakRefList][PyInner<T>]
Both:           [ObjExt][WeakRefList][PyInner<T>]

Test plan

  • cargo test --workspace passes
  • Miri tests pass (no Stacked Borrows violations)
  • test_weakref passes (137 tests)
  • test_gc, test_class, test_descr pass

Summary by CodeRabbit

  • Refactor
    • Consistent propagation and normalization of weak-reference metadata across type creation and inheritance, preventing mismatches in weakref behavior.
    • Reworked object layout and allocation to support an optional weak-reference prefix per object, improving weakref reliability and correctness.
    • Updated runtime weakref and garbage-collection paths to align with the new layout and propagation rules.
  • Notes
    • No public APIs or signatures were changed.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 5, 2026

📝 Walkthrough

Walkthrough

Adds a MANAGED_WEAKREF type flag and propagates it alongside HAS_WEAKREF during type creation. Introduces an optional two-prefix object layout (ObjExt and WeakRefList) and updates allocation, deallocation, and weakref accessors to compute and handle these prefixes.

Changes

Cohort / File(s) Summary
Type flag infrastructure
crates/vm/src/types/slot.rs
Added MANAGED_WEAKREF flag to PyTypeFlags.
Type creation & inheritance
crates/vm/src/builtins/type.rs
Propagate `HAS_WEAKREF
Object memory layout & accessors
crates/vm/src/object/core.rs
Removed weak_list from ObjExt; added WeakRefList prefix support and WEAKREF_OFFSET; updated ext_ref(), added weakref_list_ref(), and changed PyInner::new/dealloc and init path to support two-prefix allocations.
Weakref / GC callsites
crates/vm/src/object/..., crates/vm/src/weakref/...
Replaced direct ext.weak_list accesses with weakref_list_ref()/offset lookups; updated weakref drop and GC paths to use new prefix layout.
Traversal comment
crates/vm/src/object/traverse_object.rs
Removed obsolete comment about weak_list tracing; no functional change.

Sequence Diagram(s)

sequenceDiagram
  participant TypeCreator as TypeCreator
  participant Allocator as Allocator
  participant Memory as PyInner
  participant ObjExt as ObjExt
  participant WeakRef as WeakRefList

  TypeCreator->>Allocator: request allocation (flags include HAS_WEAKREF?/MANAGED_WEAKREF?)
  Allocator->>Allocator: decide layout ([ObjExt?][WeakRefList?][PyInner])
  Allocator->>Memory: allocate bytes for layout
  Allocator->>ObjExt: initialize ObjExt (if present)
  Allocator->>WeakRef: initialize WeakRefList (if present)
  Allocator->>TypeCreator: return object pointer + provenance
  TypeCreator->>Memory: set slots/flags (ensure MANAGED_WEAKREF when HAS_WEAKREF)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • ShaharNaveh
  • arihant2math

Poem

🐇
I hop through headers, prefixes bright,
ObjExt greets WeakRef in morning light.
Flags pair up, a tidy dance,
Memory hums with careful stance.
A rabbit cheers — allocation’s right! 🥕

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the primary structural change: separating WeakRefList from ObjExt prefix into two independent allocation prefixes to optimize memory layout.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2026

Code has been automatically formatted

The code in this PR has been formatted using:

  • cargo fmt --all
    Please pull the latest changes before pushing again:
git pull origin managed-weakref

@youknowone youknowone force-pushed the managed-weakref branch 3 times, most recently from 43df2a2 to 28cc5f5 Compare March 8, 2026 12:21
@youknowone youknowone marked this pull request as ready for review March 8, 2026 13:55
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
crates/vm/src/object/core.rs (1)

2426-2441: Use the computed weakref offset in the bootstrap allocator.

alloc_type_with_prefixes() already asks Layout::extend() where the WeakRefList lands, but then writes it at alloc_ptr.add(EXT_OFFSET) anyway. That duplicates the no-padding assumption instead of reusing the actual layout result, so this path can drift first if WeakRefList's alignment ever changes.

♻️ Proposed change
-            let (layout, _ext_offset) = ext_layout.extend(weakref_layout).unwrap();
+            let (layout, weakref_offset) = ext_layout.extend(weakref_layout).unwrap();
             let (combined, inner_offset) = layout.extend(inner_layout).unwrap();
             let combined = combined.pad_to_align();
@@
-                let weakref_ptr = alloc_ptr.add(EXT_OFFSET) as *mut WeakRefList;
+                let weakref_ptr = alloc_ptr.add(weakref_offset) as *mut WeakRefList;
                 weakref_ptr.write(WeakRefList::new());
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/object/core.rs` around lines 2426 - 2441, The code ignores the
computed weakref offset and uses the hardcoded EXT_OFFSET; change the binding
let (layout, _ext_offset) = ext_layout.extend(weakref_layout).unwrap() to
capture the offset (e.g. ext_offset) and use that value when computing the
weakref pointer (replace alloc_ptr.add(EXT_OFFSET) with alloc_ptr.add(ext_offset
as usize) or equivalent), so alloc_type_with_prefixes() uses the actual layout
result; ensure the variable name matches uses and types (usize) for pointer
arithmetic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/vm/src/builtins/type.rs`:
- Around line 595-600: The final type-flag assembly must always normalize
MANAGED_WEAKREF from HAS_WEAKREF regardless of which branch set it; after
assembling flags (e.g., following the MRO inheritance block that touches
mro.iter() and slots.flags and the other similar branches), add a
post-normalization step that if slots.flags contains PyTypeFlags::HAS_WEAKREF
then also |= PyTypeFlags::MANAGED_WEAKREF so static/builtin types that declare
HAS_WEAKREF (and the type objects given a managed weakref preheader in
object/core.rs) get a consistent MANAGED_WEAKREF bit; apply the same
post-normalization in the other flag-assembly sites referenced (the branches
around the other occurrences).

---

Nitpick comments:
In `@crates/vm/src/object/core.rs`:
- Around line 2426-2441: The code ignores the computed weakref offset and uses
the hardcoded EXT_OFFSET; change the binding let (layout, _ext_offset) =
ext_layout.extend(weakref_layout).unwrap() to capture the offset (e.g.
ext_offset) and use that value when computing the weakref pointer (replace
alloc_ptr.add(EXT_OFFSET) with alloc_ptr.add(ext_offset as usize) or
equivalent), so alloc_type_with_prefixes() uses the actual layout result; ensure
the variable name matches uses and types (usize) for pointer arithmetic.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: b04db21e-8745-4b0c-84a5-7a043c705bf1

📥 Commits

Reviewing files that changed from the base of the PR and between 46abff6 and 28cc5f5.

📒 Files selected for processing (4)
  • crates/vm/src/builtins/type.rs
  • crates/vm/src/object/core.rs
  • crates/vm/src/object/traverse_object.rs
  • crates/vm/src/types/slot.rs
💤 Files with no reviewable changes (1)
  • crates/vm/src/object/traverse_object.rs

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/vm/src/object/core.rs`:
- Around line 322-335: The hard-coded WEAKREF_OFFSET/EXT_OFFSET assume no
padding between prefix structs and PyInner<T>, which can be false on some
targets; replace the fixed size_of::<WeakRefList>() / size_of::<ObjExt>()
offsets with offsets computed via core::alloc::Layout::extend (derive the offset
from Layout::new::<WeakRefList>().extend(Layout::new::<PyInner<()>>()) and
similarly for ObjExt) and use that computed offset in weakref_list_ref(),
ext_ref(), gc_clear_raw() (and the other occurrences around lines 395-410) so
the pointer arithmetic accounts for any padding/alignment introduced by
Layout::extend instead of assuming a padding-free layout.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 1f65fb19-7d09-4139-becf-956e7c4afea6

📥 Commits

Reviewing files that changed from the base of the PR and between 28cc5f5 and 4ee068e.

📒 Files selected for processing (4)
  • crates/vm/src/builtins/type.rs
  • crates/vm/src/object/core.rs
  • crates/vm/src/object/traverse_object.rs
  • crates/vm/src/types/slot.rs
💤 Files with no reviewable changes (1)
  • crates/vm/src/object/traverse_object.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • crates/vm/src/types/slot.rs
  • crates/vm/src/builtins/type.rs

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
crates/vm/src/object/core.rs (1)

322-335: ⚠️ Potential issue | 🟠 Major

Derive the prefix offsets from Layout::extend, not size_of.

The fixed EXT_OFFSET / WEAKREF_OFFSET math is still only proven against PyInner<()>, but the unsafe consumers here operate on arbitrary PyInner<T>. If any payload pushes align_of::<PyInner<T>>() above 8, these subtractions land before the real prefix and turn ext_ref(), weakref_list_ref(), and gc_clear_raw() into invalid reads/writes. Please compute the negative offsets from Layout::extend(..., Layout::new::<Self>()) instead of hard-coding size_of::<ObjExt>() / size_of::<WeakRefList>().

Also applies to: 376-392, 395-410, 1759-1772

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/object/core.rs` around lines 322 - 335, Compute the prefix
offsets using Layout::extend rather than size_of so they account for arbitrary
PyInner<T> alignments: replace the hard-coded EXT_OFFSET and WEAKREF_OFFSET
calculations with values derived from Layout::extend(Layout::new::<ObjExt>(),
Layout::new::<PyInner<()>>()) (and similarly for WeakRefList) then subtract the
size/offset returned by extend for the PyInner layout to produce the negative
offsets used by ext_ref(), weakref_list_ref(), and gc_clear_raw(); ensure the
new asserts validate using Layout::extend results (referencing EXT_OFFSET,
WEAKREF_OFFSET, ObjExt, WeakRefList, PyInner, ext_ref, weakref_list_ref,
gc_clear_raw) so offsets remain correct for any PyInner<T> alignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@crates/vm/src/object/core.rs`:
- Around line 322-335: Compute the prefix offsets using Layout::extend rather
than size_of so they account for arbitrary PyInner<T> alignments: replace the
hard-coded EXT_OFFSET and WEAKREF_OFFSET calculations with values derived from
Layout::extend(Layout::new::<ObjExt>(), Layout::new::<PyInner<()>>()) (and
similarly for WeakRefList) then subtract the size/offset returned by extend for
the PyInner layout to produce the negative offsets used by ext_ref(),
weakref_list_ref(), and gc_clear_raw(); ensure the new asserts validate using
Layout::extend results (referencing EXT_OFFSET, WEAKREF_OFFSET, ObjExt,
WeakRefList, PyInner, ext_ref, weakref_list_ref, gc_clear_raw) so offsets remain
correct for any PyInner<T> alignment.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: d464627d-f6f4-4525-a7e3-e275a434bf56

📥 Commits

Reviewing files that changed from the base of the PR and between 4ee068e and 59ae6f1.

📒 Files selected for processing (4)
  • crates/vm/src/builtins/type.rs
  • crates/vm/src/object/core.rs
  • crates/vm/src/object/traverse_object.rs
  • crates/vm/src/types/slot.rs
💤 Files with no reviewable changes (1)
  • crates/vm/src/object/traverse_object.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/vm/src/types/slot.rs

- Remove weak_list from ObjExt, allocate WeakRefList as its own
  prefix slot before PyInner
- Add MANAGED_WEAKREF flag (1 << 3) to PyTypeFlags
- Normalize MANAGED_WEAKREF from HAS_WEAKREF after flag assembly
- Use Layout::extend offsets in bootstrap allocator
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
crates/vm/src/object/core.rs (1)

322-326: ⚠️ Potential issue | 🔴 Critical

The prefix offsets are still assuming a padding-free layout.

new()/dealloc() build the allocation with Layout::extend, but the read/clear paths recover prefixes by subtracting size_of::<WeakRefList>() / size_of::<ObjExt>(). That is only correct when Layout::extend inserts no padding before PyInner<T>. For a more-aligned payload, inner_offset can exceed those raw sizes, so ext_ref(), weakref_list_ref(), and gc_clear_raw() can read the wrong address.

💡 Minimal direction
-const EXT_OFFSET: usize = core::mem::size_of::<ObjExt>();
-const WEAKREF_OFFSET: usize = core::mem::size_of::<WeakRefList>();
+#[inline(always)]
+fn weakref_to_inner_offset<T>() -> usize {
+    core::alloc::Layout::new::<WeakRefList>()
+        .extend(core::alloc::Layout::new::<PyInner<T>>())
+        .unwrap()
+        .1
+}
+
+#[inline(always)]
+fn ext_to_inner_offset<T>(has_weakref: bool) -> usize {
+    if has_weakref {
+        let (layout, _) = core::alloc::Layout::new::<ObjExt>()
+            .extend(core::alloc::Layout::new::<WeakRefList>())
+            .unwrap();
+        layout.extend(core::alloc::Layout::new::<PyInner<T>>()).unwrap().1
+    } else {
+        core::alloc::Layout::new::<ObjExt>()
+            .extend(core::alloc::Layout::new::<PyInner<T>>())
+            .unwrap()
+            .1
+    }
+}

Use those computed offsets everywhere prefix pointers are reconstructed.

Also applies to: 376-410, 1759-1772

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/object/core.rs` around lines 322 - 326, The code incorrectly
reconstructs prefix pointer addresses by subtracting raw sizes
(size_of::<ObjExt>() / size_of::<WeakRefList>()) instead of using the computed
allocation offsets—update all places that recover prefixes (e.g., in new(),
dealloc(), ext_ref(), weakref_list_ref(), gc_clear_raw(), and the similar blocks
around lines 376-410 and 1759-1772) to use the precomputed
inner_offset/EXT_OFFSET/WEAKREF_OFFSET values produced when building the Layout
(or the computed inner_offset variable) so the pointer arithmetic accounts for
any padding inserted by Layout::extend; replace any direct size_of::<...>()
subtractions with the corresponding computed offset variables when
reconstructing prefix pointers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@crates/vm/src/object/core.rs`:
- Around line 322-326: The code incorrectly reconstructs prefix pointer
addresses by subtracting raw sizes (size_of::<ObjExt>() /
size_of::<WeakRefList>()) instead of using the computed allocation
offsets—update all places that recover prefixes (e.g., in new(), dealloc(),
ext_ref(), weakref_list_ref(), gc_clear_raw(), and the similar blocks around
lines 376-410 and 1759-1772) to use the precomputed
inner_offset/EXT_OFFSET/WEAKREF_OFFSET values produced when building the Layout
(or the computed inner_offset variable) so the pointer arithmetic accounts for
any padding inserted by Layout::extend; replace any direct size_of::<...>()
subtractions with the corresponding computed offset variables when
reconstructing prefix pointers.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: f4b18d16-96f9-4050-87d9-aaf7f3cbaabe

📥 Commits

Reviewing files that changed from the base of the PR and between 59ae6f1 and 3913928.

📒 Files selected for processing (4)
  • crates/vm/src/builtins/type.rs
  • crates/vm/src/object/core.rs
  • crates/vm/src/object/traverse_object.rs
  • crates/vm/src/types/slot.rs
💤 Files with no reviewable changes (1)
  • crates/vm/src/object/traverse_object.rs

@youknowone youknowone merged commit ead2d98 into RustPython:main Mar 8, 2026
13 checks passed
@youknowone youknowone deleted the managed-weakref branch March 8, 2026 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

X Tutup