Separate WeakRefList from ObjExt prefix by youknowone · Pull Request #7365 · RustPython/RustPython · GitHub

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.

📝 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)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Replace GC tracking HashSet with intrusive linked list #7328: Overlaps on PyInner/ObjExt layout, allocation/deallocation, and weakref-prefix accessors.
Object header slimming: prefix allocation for ObjExt #7334: Related weakref handling and object header layout changes; interacts with flag propagation.
Harden CALL specialization guards and cache callables #7360: Touches the same type-creation/allocation and flag propagation paths.

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.

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

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

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

♻️ 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

♻️ 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

- 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

youknowone force-pushed the managed-weakref branch 3 times, most recently from 43df2a2 to 28cc5f5 Compare March 8, 2026 12:21

youknowone marked this pull request as ready for review March 8, 2026 13:55

coderabbitai Bot reviewed Mar 8, 2026

View reviewed changes

Comment thread crates/vm/src/builtins/type.rs

youknowone force-pushed the managed-weakref branch from 28cc5f5 to 4ee068e Compare March 8, 2026 14:19

coderabbitai Bot reviewed Mar 8, 2026

View reviewed changes

Comment thread crates/vm/src/object/core.rs

youknowone force-pushed the managed-weakref branch from 4ee068e to 59ae6f1 Compare March 8, 2026 15:01

coderabbitai Bot reviewed Mar 8, 2026

View reviewed changes

youknowone force-pushed the managed-weakref branch from 59ae6f1 to 3913928 Compare March 8, 2026 15:14

coderabbitai Bot reviewed Mar 8, 2026

View reviewed changes

youknowone merged commit ead2d98 into RustPython:main Mar 8, 2026
13 checks passed

youknowone deleted the managed-weakref branch March 8, 2026 17:09

Separate WeakRefList from ObjExt prefix by youknowone · Pull Request #7365 · RustPython/RustPython · GitHub

Uh oh!

Separate WeakRefList from ObjExt prefix - #7365

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

youknowone commented Mar 5, 2026 •
edited by coderabbitai Bot
Loading

Uh oh!

Uh oh!

coderabbitai Bot commented Mar 5, 2026 •
edited
Loading

Uh oh!

Uh oh!

github-actions Bot commented Mar 5, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Separate WeakRefList from ObjExt prefix by youknowone · Pull Request #7365 · RustPython/RustPython · GitHub

Uh oh!

Separate WeakRefList from ObjExt prefix - #7365

Separate WeakRefList from ObjExt prefix#7365youknowone merged 1 commit intoRustPython:mainRustPython/RustPython:mainfrom youknowone:managed-weakrefyouknowone/RustPython:managed-weakrefCopy head branch name to clipboard

Conversation

youknowone commented Mar 5, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Memory layout

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

github-actions Bot commented Mar 5, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

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

youknowone commented Mar 5, 2026 •
edited by coderabbitai Bot
Loading

coderabbitai Bot commented Mar 5, 2026 •
edited
Loading