Make PyList to ThreadSafe #1871
Conversation
vm/src/obj/objlist.rs
Outdated
| let mut elements = Vec::<u8>::with_capacity(self.get_len()); | ||
| for elem in self.elements.borrow().iter() { | ||
| let mut elements = Vec::<u8>::with_capacity(self.borrow_elements().len()); | ||
| for elem in self.borrow_elements().clone().iter() { |
There was a problem hiding this comment.
is this cloning of Vec or lock?
There was a problem hiding this comment.
The Vec. RwLock does not implement Clone
There was a problem hiding this comment.
it seems additional clone is added to a few places. But this is using the iterator of the vector instead of the vector itself. Is there no way to avoid this kind of clone?
There was a problem hiding this comment.
I don't think so, if there is python code running inside the loop body, then it could theoretically lock the pylist again and cause a panic or deadlock. Although that doesn't seem necessary for this one, @palaviv, none of the code inside the loop calls into the interpreter. Is there another reason you cloned the vec here?
There was a problem hiding this comment.
I removed the clone here. I tried to clone only where python code is called.
vm/src/pyobject.rs
Outdated
| @@ -1222,6 +1222,10 @@ pub trait PyValue: fmt::Debug + Sized + 'static { | |||
|
|
|||
| // Temporary trait to follow the progress of threading conversion | |||
| pub trait ThreadSafe: Send + Sync {} | |||
| // Temporary definitions to help with converting object that contain PyObjectRef to ThreadSafe. | |||
| // Should be removed before threading is allowed. | |||
There was a problem hiding this comment.
Maybe mention here how this is technically unsound, and that PyObjects shouldn't actually be passed to or accessed by other threads?
There was a problem hiding this comment.
I hope the new message is better. This should be a very temporary thing.
vm/src/obj/objlist.rs
Outdated
| let mut elements = PyList::_replace(self.borrow_elements_mut(), vec![]); | ||
| do_sort(vm, &mut elements, options.key, options.reverse)?; | ||
| let temp_elements = self.elements.replace(elements); | ||
| let temp_elements = PyList::_replace(self.borrow_elements_mut(), elements); |
There was a problem hiding this comment.
I think if you borrow mutably here, you can just use std::mem::take() or std::mem::replace(). Something like:
let mut elements = self.borrow_elements_mut();
let mut temp_elements = std::mem::take(&mut elements);
do_sort(vm, &mut temp_elements, options.key, options.reverse)?;
std::mem::swap(&mut temp_elements, &mut elements);| } | ||
|
|
||
| #[pymethod] | ||
| fn count(&self, needle: PyObjectRef, vm: &VirtualMachine) -> PyResult<usize> { | ||
| let mut count: usize = 0; | ||
| for element in self.elements.borrow().iter() { | ||
| for element in self.borrow_elements().clone().iter() { |
There was a problem hiding this comment.
It'd be really nice if we could somehow detect if something tries to lock while we already have one and then give it a clone, but I think any solution to that would violate rust's aliasing rules.
There was a problem hiding this comment.
The big question is how expensive is the clone. I hope that this is very quick because of the Arc.
No description provided.