Use borrowed pointers in TYPE_CACHE instead of strong references - #7384
Use borrowed pointers in TYPE_CACHE instead of strong references#7384youknowone merged 1 commit into
Conversation
Sorry, something went wrong.
The method cache (TYPE_CACHE) was storing strong references (Arc clones) to cached attribute values, which inflated sys.getrefcount(). This caused intermittent test_memoryview failures where refcount assertions would fail depending on GC collection timing. Store borrowed raw pointers instead. Safety is guaranteed because: - type_cache_clear() nullifies all entries during GC collection, before the collector breaks cycles - type_cache_clear_version() nullifies entries when a type is modified, before the source dict entry is removed - Readers use try_to_owned_from_ptr (safe_inc) to atomically validate and increment the refcount on cache hit
📝 Walkthrough
WalkthroughThe PR converts the type cache from storing owned PyObject references to borrowed pointers, reducing reference counting overhead. The take_value method is replaced with clear_value for cache invalidation. Cache insertion and clearing operations are updated to work with borrowed pointers while maintaining correctness with GC behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem🚥 Pre-merge checks | ✅ 3 ✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches 🧪 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agentsVerify each finding against the current code and only fix it if needed. Inline comments: In `@crates/vm/src/builtins/type.rs`: - Around line 84-89: Update the unsafe lifetime doc for the type cache to cover both roots: state that entries referencing heap types are cleared by GC via type_cache_clear() and type_cache_clear_version(), while static (untracked/immortal) types are not collected and remain valid for the program lifetime (refer to the static-type untracking logic). Mention both behaviors explicitly so callers understand that cached pointers may be nulled during GC for heap types but are permanently valid for static types. - Around line 805-809: The type dict mutation pathway must always call the cache-invalidation path before changing attributes; create and use a helper like mutate_type_dict(&self, f: impl FnOnce(&mut PyAttributes)) that calls self.modified() then invokes f on self.attributes.write(), and replace all direct mutations (e.g., calls to self.attributes.write().insert, swap_remove, remove) in the annotation/cache setters and the sites around the symbols mentioned (lines handling __annotations_cache__, __annotate_func__, and the blocks currently using insert/swap_remove) to use this helper so every post-construction write funnels through the invalidation path.
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 35c55559-b67c-4e53-ad9e-c8a48f63fa18
📥 CommitsReviewing files that changed from the base of the PR and between 46abff6 and b7b23e7.
📒 Files selected for processing (1)- crates/vm/src/builtins/type.rs
Sorry, something went wrong.
| /// The cache holds a **borrowed** pointer (no refcount increment). | ||
| /// Safety: `type_cache_clear()` nullifies all entries during GC, | ||
| /// and `type_cache_clear_version()` nullifies entries when a type | ||
| /// is modified — both before the source dict entry is removed. | ||
| /// Types are always part of reference cycles (via `mro` self-reference) | ||
| /// so they are always collected by the cyclic GC (never refcount-freed). |
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor
The unsafe lifetime note needs the static-type case too.
Lines 689-696 explicitly untrack static types, so "Types are always collected by the cyclic GC" is not true for every cache owner here. Please document both supported roots explicitly: heap types are cleared by GC, while static types are immortal.
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@crates/vm/src/builtins/type.rs` around lines 84 - 89, Update the unsafe lifetime doc for the type cache to cover both roots: state that entries referencing heap types are cleared by GC via type_cache_clear() and type_cache_clear_version(), while static (untracked/immortal) types are not collected and remain valid for the program lifetime (refer to the static-type untracking logic). Mention both behaviors explicitly so callers understand that cached pointers may be nulled during GC for heap types but are permanently valid for static types.
Sorry, something went wrong.
| // Invalidate caches BEFORE modifying attributes so that borrowed | ||
| // pointers in cache entries are nullified while the source objects | ||
| // are still alive. | ||
| self.modified(); | ||
| self.attributes.write().insert(attr_name, value); |
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical
Borrowed-cache safety is still bypassed by other type-dict mutations.
After this switch, ordinary keys like __annotations_cache__ and __annotate_func__ can be cached as borrowed pointers, but at minimum Lines 1379, 1406-1408, 1470-1472, and 1489-1525 still replace/remove those entries with raw self.attributes.write().insert/swap_remove() and no modified() call. A simple t.__annotations_cache__ → mutate annotations → t.__annotations_cache__ sequence can leave the cache pointing at freed storage. Please funnel every post-construction attributes mutation through the same invalidation path.
Suggested directionfn mutate_type_dict(&self, f: impl FnOnce(&mut PyAttributes)) {
self.modified();
f(&mut self.attributes.write());
}Use that helper for the annotation/cache setters and any other runtime self.attributes writes.
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@crates/vm/src/builtins/type.rs` around lines 805 - 809, The type dict mutation pathway must always call the cache-invalidation path before changing attributes; create and use a helper like mutate_type_dict(&self, f: impl FnOnce(&mut PyAttributes)) that calls self.modified() then invokes f on self.attributes.write(), and replace all direct mutations (e.g., calls to self.attributes.write().insert, swap_remove, remove) in the annotation/cache setters and the sites around the symbols mentioned (lines handling __annotations_cache__, __annotate_func__, and the blocks currently using insert/swap_remove) to use this helper so every post-construction write funnels through the invalidation path.
Sorry, something went wrong.
…tPython#7384) The method cache (TYPE_CACHE) was storing strong references (Arc clones) to cached attribute values, which inflated sys.getrefcount(). This caused intermittent test_memoryview failures where refcount assertions would fail depending on GC collection timing. Store borrowed raw pointers instead. Safety is guaranteed because: - type_cache_clear() nullifies all entries during GC collection, before the collector breaks cycles - type_cache_clear_version() nullifies entries when a type is modified, before the source dict entry is removed - Readers use try_to_owned_from_ptr (safe_inc) to atomically validate and increment the refcount on cache hit
The method cache (TYPE_CACHE) was storing strong references (Arc clones) to cached attribute values, which inflated sys.getrefcount(). This caused intermittent test_memoryview failures where refcount assertions would fail depending on GC collection timing.
Store borrowed raw pointers instead. Safety is guaranteed because:
Summary by CodeRabbit