Big overhaul part 1 - replace PyRc with manual RefCount + WeakRefList #3386
Conversation
|
I'll get to this tonight if someone else doesn't beat me to the punch, but |
bfe7aff
to
37e1394
|
@fanninpm ooh yea, I noticed that myself, that's really old lol. |
7cf10e3
to
69f5c80
|
At this point, running Miri without |
|
Well, that's probably to be expected, since we don't implement cycle collection and especially type_type and object_type are codependent on each other |
d89e106
to
d41f3e0
The outer struct is only 1 word now, and inside the box it's a byte for the mutex (which gets padded to a word) + a word for the linked list (we don't use the tail) + a word for the pointer to the object.
I want to review drop part again, but looks great in general.
| @@ -9,8 +9,10 @@ pub mod cmp; | |||
| pub mod encodings; | |||
| pub mod float_ops; | |||
| pub mod hash; | |||
| pub mod linked_list; | |||
this is used because std::collections::LinkedList is not fit, right?
Yea, it's like a separate allocation thing. We wouldn't be able to make a PyRef<PyWeak> point at one of std::collection::LinkedList's Nodes. Plus tokio's version just gives more flexibility, cause it's designed to be an internal data structure maybe
| } | ||
|
|
||
| #[inline] | ||
| pub fn incref(&self) { |
Because it is already *Ref*Count
| pub fn incref(&self) { | |
| pub fn inc(&self) { |
Otherwise
| pub fn incref(&self) { | |
| pub fn inc_ref(&self) { |
Idk, I think I'd like us to use "incref" more in the codebase - like for the other borrowed PyObject change, I used incref because in general I think it's weird to clone()/to_owned() stuff that isn't actually owned separately. I can think of one instance where someone commenting on a PR thought that an issue was caused by clone() creating a new object, even though it doesn't. (This I think is why Rc/Arc encourages Rc::clone(&foo) instead of foo.clone() because it makes it always clear that it's actually just incr'ing a refcount.) So I'd like to switch to incref() instead of to_owned/clone at some point (another reason it was specifically in that pr - it makes so you don't have to think about whether the object is borrowed and you should do to_owned or owned (clone), if you want to incref you always just do incref()) and I think it makes sense to implement incref by calling an inner incref. (long explanation lol.) another thing ig is that cpython uses "incref" a lot, even the lowercase word (not the Py_INCREF macro) is used 50+ times just in comments/prose.
| @@ -150,6 +150,8 @@ pub(crate) type DescrSetFunc = | |||
| pub(crate) type NewFunc = fn(PyTypeRef, FuncArgs, &VirtualMachine) -> PyResult; | |||
| pub(crate) type DelFunc = fn(&PyObject, &VirtualMachine) -> PyResult<()>; | |||
|
|
|||
| pub use crate::builtins::object::{generic_getattr, generic_setattr}; | |||
are these used in macro?
Oh that was for part 2 of this, for filling in the default get/setattro slot. Is it ok if I leave it? I think it's more hassle to take it out and I think in general it'd be good to have definitive function pointers for generic slots rather than a bunch of wrappers that call those functions. Like 20 impl Getattro for _ { fn getattro() { vm.generic_getattro() } } vs just directly using a generic_getattr function.
| typ: PyTypeRef, | ||
| vm: &VirtualMachine, | ||
| ) -> PyResult<PyObjectWeak> { | ||
| let dict = if typ |
I am confused here. A dict is created when downgrading?
Yeah, for the weakref object - if it's a subclass it needs to have a dict for the user code to store attributes and stuff on, like any user subclass
This resolves #2382, resolves #2381 (afaict), and at the very least it will make it much easier to fix any UB now that we control the internals of all the data structures we use.
The text was updated successfully, but these errors were encountered: