Lazy type objects without once_cell #758
Conversation
kngwyu
force-pushed
the
remove-oncecell
branch
3 times, most recently
from
c8da41c to
65adef8
Compare
kngwyu
force-pushed
the
remove-oncecell
branch
from
65adef8 to
58a0841
Compare
|
One problem with |
There was a problem hiding this comment.
I like the idea of removing once_cell and using the GIL instead - makes total sense! I have a minor nitpick with the implementation to make it more bombproof.
I also have another concern with &'static - which is that I'm not sure it will play nicely with multiple sub-interpreters. According to PEP 554 and the docs for Py_NewInterpreter, even builtin type objects are separate between each sub-interpreter. So eventually we'd have to store instances of each type object for each sub-interpreter, and I don't think we can use &'static with that.
So if we want to one day have support for sub-interpreters, then we probably don't want type_object() to return &'static. We've had at least one request for sub-interpreters in the past (#576)
src/type_object.rs
Outdated
| return value.clone(); | ||
| } | ||
| // We have to get the GIL before setting the value to the global!!! | ||
| let gil = Python::acquire_gil(); |
There was a problem hiding this comment.
It's unlikely to happen in practice, but there's nothing to prevent two or more threads reaching this point at the same time, which would then lead to value being initialized multiple times. Whether this is a problem, I'm not sure.
We could avoid this by checking again that value is still None after the GIL lock is acquired, and exiting early if some other thread initialized value while this thread was waiting for the GIL.
There was a problem hiding this comment.
Thank you.
I changed this to use AtomicBool
| } | ||
| } | ||
| pub fn get<T: PyClass>(&self) -> &ffi::PyTypeObject { | ||
| if !self.initialized.get() { | ||
| let gil = Python::acquire_gil(); |
There was a problem hiding this comment.
Same comment applies as for LazyHeapType.
|
Thank you for the reviews, but I think |
|
Ok. I'll cross this hurdle if we ever get to sub-interpreter support :) |
The point is the use of our great GIL to make the global operation safe, and now we don't need
Box::into_rawfor type objects.I'm sure this does not bring huge merits to users who have only a few
#[pyclass]es in their crates... but somehow I enjoyed trying to removeBox. It was fun 🙂