From 2412264cc0c679915d0811f178d2c4b68b4e8b2f Mon Sep 17 00:00:00 2001 From: Aviv Palivoda Date: Tue, 14 Apr 2020 13:55:20 +0300 Subject: [PATCH 1/4] Temporary define PyObject as Send+Sync --- vm/src/pyobject.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/vm/src/pyobject.rs b/vm/src/pyobject.rs index 03987778af..33a06a3aa6 100644 --- a/vm/src/pyobject.rs +++ b/vm/src/pyobject.rs @@ -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. +unsafe impl Send for PyObject {} +unsafe impl Sync for PyObject {} pub trait PyObjectPayload: Any + fmt::Debug + 'static { fn as_any(&self) -> &dyn Any; From b63b9fca46006a5598764fe1faf2ccfe22dc28fc Mon Sep 17 00:00:00 2001 From: Aviv Palivoda Date: Tue, 14 Apr 2020 13:55:44 +0300 Subject: [PATCH 2/4] Mark PyTuple as ThreadSafe --- vm/src/obj/objtuple.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/vm/src/obj/objtuple.rs b/vm/src/obj/objtuple.rs index b316b589e7..622da42766 100644 --- a/vm/src/obj/objtuple.rs +++ b/vm/src/obj/objtuple.rs @@ -9,7 +9,7 @@ use crate::pyhash; use crate::pyobject::{ IntoPyObject, PyArithmaticValue::{self, *}, - PyClassImpl, PyComparisonValue, PyContext, PyObjectRef, PyRef, PyResult, PyValue, + PyClassImpl, PyComparisonValue, PyContext, PyObjectRef, PyRef, PyResult, PyValue, ThreadSafe, }; use crate::sequence::{self, SimpleSeq}; use crate::vm::{ReprGuard, VirtualMachine}; @@ -23,6 +23,8 @@ pub struct PyTuple { elements: Vec, } +impl ThreadSafe for PyTuple {} + impl fmt::Debug for PyTuple { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { // TODO: implement more informational, non-recursive Debug formatter @@ -247,6 +249,8 @@ pub struct PyTupleIterator { tuple: PyTupleRef, } +impl ThreadSafe for PyTupleIterator {} + impl PyValue for PyTupleIterator { fn class(vm: &VirtualMachine) -> PyClassRef { vm.ctx.tupleiterator_type() From 970d17f098bb597f5edb0325f66c8c11fc0c3440 Mon Sep 17 00:00:00 2001 From: Aviv Palivoda Date: Wed, 15 Apr 2020 13:00:27 +0300 Subject: [PATCH 3/4] Make PyList ThreadSafe --- vm/src/obj/objlist.rs | 283 ++++++++++++++++++++++++------------------ vm/src/sequence.rs | 25 ++++ 2 files changed, 186 insertions(+), 122 deletions(-) diff --git a/vm/src/obj/objlist.rs b/vm/src/obj/objlist.rs index 4821350903..81d9cc42e1 100644 --- a/vm/src/obj/objlist.rs +++ b/vm/src/obj/objlist.rs @@ -1,7 +1,7 @@ -use std::cell::RefCell; use std::fmt; use std::mem::size_of; use std::ops::Range; +use std::sync::{RwLock, RwLockReadGuard, RwLockWriteGuard}; use crossbeam_utils::atomic::AtomicCell; use num_bigint::{BigInt, ToBigInt}; @@ -17,7 +17,7 @@ use super::objtype::PyClassRef; use crate::function::OptionalArg; use crate::pyobject::{ IdProtocol, PyArithmaticValue::*, PyClassImpl, PyComparisonValue, PyContext, PyIterable, - PyObjectRef, PyRef, PyResult, PyValue, TryFromObject, TypeProtocol, + PyObjectRef, PyRef, PyResult, PyValue, ThreadSafe, TryFromObject, TypeProtocol, }; use crate::sequence::{self, SimpleSeq}; use crate::vm::{ReprGuard, VirtualMachine}; @@ -29,10 +29,11 @@ use crate::vm::{ReprGuard, VirtualMachine}; #[pyclass] #[derive(Default)] pub struct PyList { - // TODO: make this a RwLock at the same time as PyObjectRef is Send + Sync - elements: RefCell>, + elements: RwLock>, } +impl ThreadSafe for PyList {} + impl fmt::Debug for PyList { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { // TODO: implement more detailed, non-recursive Debug formatter @@ -43,7 +44,7 @@ impl fmt::Debug for PyList { impl From> for PyList { fn from(elements: Vec) -> Self { PyList { - elements: RefCell::new(elements), + elements: RwLock::new(elements), } } } @@ -55,38 +56,32 @@ impl PyValue for PyList { } impl PyList { - pub fn borrow_sequence<'a>(&'a self) -> impl SimpleSeq + 'a { - self.elements.borrow() - } - - pub fn borrow_elements<'a>(&'a self) -> impl std::ops::Deref> + 'a { - self.elements.borrow() + pub fn borrow_elements(&self) -> RwLockReadGuard<'_, Vec> { + self.elements.read().unwrap() } -} -impl PyList { - fn get_len(&self) -> usize { - self.elements.borrow().len() + pub fn borrow_elements_mut(&self) -> RwLockWriteGuard<'_, Vec> { + self.elements.write().unwrap() } - fn get_pos(&self, p: i32) -> Option { + fn get_pos(p: i32, len: usize) -> Option { // convert a (potentially negative) positon into a real index if p < 0 { - if -p as usize > self.get_len() { + if -p as usize > len { None } else { - Some(self.get_len() - ((-p) as usize)) + Some(len - ((-p) as usize)) } - } else if p as usize >= self.get_len() { + } else if p as usize >= len { None } else { Some(p as usize) } } - fn get_slice_pos(&self, slice_pos: &BigInt) -> usize { + fn get_slice_pos(slice_pos: &BigInt, len: usize) -> usize { if let Some(pos) = slice_pos.to_i32() { - if let Some(index) = self.get_pos(pos) { + if let Some(index) = PyList::get_pos(pos, len) { // within bounds return index; } @@ -97,16 +92,19 @@ impl PyList { 0 } else { // slice past end bound, round to end - self.get_len() + len } } - fn get_slice_range(&self, start: &Option, stop: &Option) -> Range { - let start = start.as_ref().map(|x| self.get_slice_pos(x)).unwrap_or(0); + fn get_slice_range(start: &Option, stop: &Option, len: usize) -> Range { + let start = start + .as_ref() + .map(|x| PyList::get_slice_pos(x, len)) + .unwrap_or(0); let stop = stop .as_ref() - .map(|x| self.get_slice_pos(x)) - .unwrap_or_else(|| self.get_len()); + .map(|x| PyList::get_slice_pos(x, len)) + .unwrap_or_else(|| len); start..stop } @@ -115,8 +113,8 @@ impl PyList { &self, vm: &VirtualMachine, ) -> PyResult { - let mut elements = Vec::::with_capacity(self.get_len()); - for elem in self.elements.borrow().iter() { + let mut elements = Vec::::with_capacity(self.borrow_elements().len()); + for elem in self.borrow_elements().clone().iter() { match PyIntRef::try_from_object(vm, elem.clone()) { Ok(result) => match result.as_bigint().to_u8() { Some(result) => elements.push(result), @@ -150,20 +148,20 @@ pub type PyListRef = PyRef; impl PyList { #[pymethod] pub(crate) fn append(&self, x: PyObjectRef) { - self.elements.borrow_mut().push(x); + self.borrow_elements_mut().push(x); } #[pymethod] fn extend(&self, x: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { let mut new_elements = vm.extract_elements(&x)?; - self.elements.borrow_mut().append(&mut new_elements); + self.borrow_elements_mut().append(&mut new_elements); Ok(()) } #[pymethod] fn insert(&self, position: isize, element: PyObjectRef) { - let mut vec = self.elements.borrow_mut(); - let vec_len = vec.len().to_isize().unwrap(); + let mut elements = self.borrow_elements_mut(); + let vec_len = elements.len().to_isize().unwrap(); // This unbounded position can be < 0 or > vec.len() let unbounded_position = if position < 0 { vec_len + position @@ -172,14 +170,14 @@ impl PyList { }; // Bound it by [0, vec.len()] let position = unbounded_position.max(0).min(vec_len).to_usize().unwrap(); - vec.insert(position, element.clone()); + elements.insert(position, element.clone()); } #[pymethod(name = "__add__")] fn add(&self, other: PyObjectRef, vm: &VirtualMachine) -> PyResult { if let Some(other) = other.payload_if_subclass::(vm) { - let e1 = self.borrow_sequence(); - let e2 = other.borrow_sequence(); + let e1 = self.borrow_elements().clone(); + let e2 = other.borrow_elements().clone(); let elements = e1.iter().chain(e2.iter()).cloned().collect(); Ok(vm.ctx.new_list(elements)) } else { @@ -195,7 +193,7 @@ impl PyList { fn iadd(zelf: PyRef, other: PyObjectRef, vm: &VirtualMachine) -> PyResult { if let Ok(new_elements) = vm.extract_elements(&other) { let mut e = new_elements; - zelf.elements.borrow_mut().append(&mut e); + zelf.borrow_elements_mut().append(&mut e); Ok(zelf.into_object()) } else { Ok(vm.ctx.not_implemented()) @@ -204,37 +202,37 @@ impl PyList { #[pymethod(name = "__bool__")] fn bool(&self) -> bool { - !self.elements.borrow().is_empty() + !self.borrow_elements().is_empty() } #[pymethod] fn clear(&self) { - self.elements.borrow_mut().clear(); + self.borrow_elements_mut().clear(); } #[pymethod] fn copy(&self, vm: &VirtualMachine) -> PyObjectRef { - vm.ctx.new_list(self.elements.borrow().clone()) + vm.ctx.new_list(self.borrow_elements().clone()) } #[pymethod(name = "__len__")] fn len(&self) -> usize { - self.elements.borrow().len() + self.borrow_elements().len() } #[pymethod(name = "__sizeof__")] fn sizeof(&self) -> usize { - size_of::() + self.elements.borrow().capacity() * size_of::() + size_of::() + self.borrow_elements().capacity() * size_of::() } #[pymethod] fn reverse(&self) { - self.elements.borrow_mut().reverse(); + self.borrow_elements_mut().reverse(); } #[pymethod(name = "__reversed__")] fn reversed(zelf: PyRef) -> PyListReverseIterator { - let final_position = zelf.elements.borrow().len(); + let final_position = zelf.borrow_elements().len(); PyListReverseIterator { position: AtomicCell::new(final_position as isize), list: zelf, @@ -246,7 +244,7 @@ impl PyList { get_item( vm, zelf.as_object(), - &zelf.elements.borrow(), + &zelf.borrow_elements(), needle.clone(), ) } @@ -278,8 +276,9 @@ impl PyList { } fn setindex(&self, index: i32, value: PyObjectRef, vm: &VirtualMachine) -> PyResult { - if let Some(pos_index) = self.get_pos(index) { - self.elements.borrow_mut()[pos_index] = value; + let mut elements = self.borrow_elements_mut(); + if let Some(pos_index) = PyList::get_pos(index, elements.len()) { + elements[pos_index] = value; Ok(vm.get_none()) } else { Err(vm.new_index_error("list assignment index out of range".to_owned())) @@ -288,75 +287,97 @@ impl PyList { fn setslice(&self, slice: PySliceRef, sec: PyIterable, vm: &VirtualMachine) -> PyResult { let step = slice.step_index(vm)?.unwrap_or_else(BigInt::one); + let start = slice.start_index(vm)?; + let stop = slice.stop_index(vm)?; + // consume the iter, we need it's size + // and if it's going to fail we want that to happen *before* we start modifing + // In addition we want to iterate before taking the list lock. + let items: Result, _> = sec.iter(vm)?.collect(); + let items = items?; + let elements = self.borrow_elements_mut(); if step.is_zero() { Err(vm.new_value_error("slice step cannot be zero".to_owned())) } else if step.is_positive() { - let range = self.get_slice_range(&slice.start_index(vm)?, &slice.stop_index(vm)?); + let range = PyList::get_slice_range(&start, &stop, elements.len()); if range.start < range.end { match step.to_i32() { - Some(1) => self._set_slice(range, sec, vm), + Some(1) => PyList::_set_slice(elements, range, items, vm), Some(num) => { // assign to extended slice - self._set_stepped_slice(range, num as usize, sec, vm) + PyList::_set_stepped_slice(elements, range, num as usize, items, vm) } None => { // not sure how this is reached, step too big for i32? // then step is bigger than the than len of the list, no question #[allow(clippy::range_plus_one)] - self._set_stepped_slice(range.start..(range.start + 1), 1, sec, vm) + PyList::_set_stepped_slice( + elements, + range.start..(range.start + 1), + 1, + items, + vm, + ) } } } else { // this functions as an insert of sec before range.start - self._set_slice(range.start..range.start, sec, vm) + PyList::_set_slice(elements, range.start..range.start, items, vm) } } else { // calculate the range for the reverse slice, first the bounds needs to be made // exclusive around stop, the lower number - let start = &slice.start_index(vm)?.as_ref().map(|x| { + let start = &start.as_ref().map(|x| { if *x == (-1).to_bigint().unwrap() { - self.get_len() + BigInt::one() //.to_bigint().unwrap() + elements.len() + BigInt::one() //.to_bigint().unwrap() } else { x + 1 } }); - let stop = &slice.stop_index(vm)?.as_ref().map(|x| { + let stop = &stop.as_ref().map(|x| { if *x == (-1).to_bigint().unwrap() { - self.get_len().to_bigint().unwrap() + elements.len().to_bigint().unwrap() } else { x + 1 } }); - let range = self.get_slice_range(&stop, &start); + let range = PyList::get_slice_range(&stop, &start, elements.len()); match (-step).to_i32() { - Some(num) => self._set_stepped_slice_reverse(range, num as usize, sec, vm), + Some(num) => { + PyList::_set_stepped_slice_reverse(elements, range, num as usize, items, vm) + } None => { // not sure how this is reached, step too big for i32? // then step is bigger than the than len of the list no question - self._set_stepped_slice_reverse(range.end - 1..range.end, 1, sec, vm) + PyList::_set_stepped_slice_reverse( + elements, + range.end - 1..range.end, + 1, + items, + vm, + ) } } } } - fn _set_slice(&self, range: Range, sec: PyIterable, vm: &VirtualMachine) -> PyResult { - // consume the iter, we need it's size - // and if it's going to fail we want that to happen *before* we start modifing - let items: Result, _> = sec.iter(vm)?.collect(); - let items = items?; - + fn _set_slice( + mut elements: RwLockWriteGuard<'_, Vec>, + range: Range, + items: Vec, + vm: &VirtualMachine, + ) -> PyResult { // replace the range of elements with the full sequence - self.elements.borrow_mut().splice(range, items); + elements.splice(range, items); Ok(vm.get_none()) } fn _set_stepped_slice( - &self, + elements: RwLockWriteGuard<'_, Vec>, range: Range, step: usize, - sec: PyIterable, + items: Vec, vm: &VirtualMachine, ) -> PyResult { let slicelen = if range.end > range.start { @@ -364,17 +385,13 @@ impl PyList { } else { 0 }; - // consume the iter, we need it's size - // and if it's going to fail we want that to happen *before* we start modifing - let items: Result, _> = sec.iter(vm)?.collect(); - let items = items?; let n = items.len(); if range.start < range.end { if n == slicelen { let indexes = range.step_by(step); - self._replace_indexes(indexes, &items); + PyList::_replace_indexes(elements, indexes, &items); Ok(vm.get_none()) } else { Err(vm.new_value_error(format!( @@ -395,10 +412,10 @@ impl PyList { } fn _set_stepped_slice_reverse( - &self, + elements: RwLockWriteGuard<'_, Vec>, range: Range, step: usize, - sec: PyIterable, + items: Vec, vm: &VirtualMachine, ) -> PyResult { let slicelen = if range.end > range.start { @@ -407,17 +424,12 @@ impl PyList { 0 }; - // consume the iter, we need it's size - // and if it's going to fail we want that to happen *before* we start modifing - let items: Result, _> = sec.iter(vm)?.collect(); - let items = items?; - let n = items.len(); if range.start < range.end { if n == slicelen { let indexes = range.rev().step_by(step); - self._replace_indexes(indexes, &items); + PyList::_replace_indexes(elements, indexes, &items); Ok(vm.get_none()) } else { Err(vm.new_value_error(format!( @@ -437,12 +449,13 @@ impl PyList { } } - fn _replace_indexes(&self, indexes: I, items: &[PyObjectRef]) - where + fn _replace_indexes( + mut elements: RwLockWriteGuard<'_, Vec>, + indexes: I, + items: &[PyObjectRef], + ) where I: Iterator, { - let mut elements = self.elements.borrow_mut(); - for (i, value) in indexes.zip(items) { // clone for refrence count elements[i] = value.clone(); @@ -452,8 +465,9 @@ impl PyList { #[pymethod(name = "__repr__")] fn repr(zelf: PyRef, vm: &VirtualMachine) -> PyResult { let s = if let Some(_guard) = ReprGuard::enter(zelf.as_object()) { - let mut str_parts = Vec::with_capacity(zelf.elements.borrow().len()); - for elem in zelf.elements.borrow().iter() { + let elements = zelf.borrow_elements().clone(); + let mut str_parts = Vec::with_capacity(elements.len()); + for elem in elements.iter() { let s = vm.to_repr(elem)?; str_parts.push(s.as_str().to_owned()); } @@ -471,7 +485,7 @@ impl PyList { #[pymethod(name = "__mul__")] fn mul(&self, counter: isize, vm: &VirtualMachine) -> PyObjectRef { - let new_elements = sequence::seq_mul(&self.borrow_sequence(), counter) + let new_elements = sequence::seq_mul(&self.borrow_elements(), counter) .cloned() .collect(); vm.ctx.new_list(new_elements) @@ -484,17 +498,17 @@ impl PyList { #[pymethod(name = "__imul__")] fn imul(zelf: PyRef, counter: isize) -> PyRef { - let new_elements = sequence::seq_mul(&zelf.borrow_sequence(), counter) - .cloned() - .collect(); - zelf.elements.replace(new_elements); - zelf + let elements = zelf.borrow_elements_mut(); + let new_elements: Vec = + sequence::seq_mul(&elements, counter).cloned().collect(); + PyList::_replace(elements, new_elements); + zelf.clone() } #[pymethod] fn count(&self, needle: PyObjectRef, vm: &VirtualMachine) -> PyResult { let mut count: usize = 0; - for element in self.elements.borrow().iter() { + for element in self.borrow_elements().clone().iter() { if vm.identical_or_equal(element, &needle)? { count += 1; } @@ -504,7 +518,7 @@ impl PyList { #[pymethod(name = "__contains__")] fn contains(&self, needle: PyObjectRef, vm: &VirtualMachine) -> PyResult { - for element in self.elements.borrow().iter() { + for element in self.borrow_elements().clone().iter() { if vm.identical_or_equal(element, &needle)? { return Ok(true); } @@ -515,7 +529,7 @@ impl PyList { #[pymethod] fn index(&self, needle: PyObjectRef, vm: &VirtualMachine) -> PyResult { - for (index, element) in self.elements.borrow().iter().enumerate() { + for (index, element) in self.borrow_elements().clone().iter().enumerate() { if vm.identical_or_equal(element, &needle)? { return Ok(index); } @@ -527,7 +541,7 @@ impl PyList { #[pymethod] fn pop(&self, i: OptionalArg, vm: &VirtualMachine) -> PyResult { let mut i = i.into_option().unwrap_or(-1); - let mut elements = self.elements.borrow_mut(); + let mut elements = self.borrow_elements_mut(); if i < 0 { i += elements.len() as isize; } @@ -543,7 +557,7 @@ impl PyList { #[pymethod] fn remove(&self, needle: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { let mut ri: Option = None; - for (index, element) in self.elements.borrow().iter().enumerate() { + for (index, element) in self.borrow_elements().clone().iter().enumerate() { if vm.identical_or_equal(element, &needle)? { ri = Some(index); break; @@ -551,7 +565,8 @@ impl PyList { } if let Some(index) = ri { - self.elements.borrow_mut().remove(index); + // TODO: Check if value was removed after lock released + self.borrow_elements_mut().remove(index); Ok(()) } else { let needle_str = vm.to_str(&needle)?; @@ -565,7 +580,10 @@ impl PyList { F: Fn(&Vec, &Vec) -> PyResult, { let r = if let Some(other) = other.payload_if_subclass::(vm) { - Implemented(op(&*self.borrow_elements(), &*other.borrow_elements())?) + Implemented(op( + &self.borrow_elements().clone(), + &other.borrow_elements().clone(), + )?) } else { NotImplemented }; @@ -623,8 +641,9 @@ impl PyList { } fn delindex(&self, index: i32, vm: &VirtualMachine) -> PyResult<()> { - if let Some(pos_index) = self.get_pos(index) { - self.elements.borrow_mut().remove(pos_index); + let mut elements = self.borrow_elements_mut(); + if let Some(pos_index) = PyList::get_pos(index, elements.len()) { + elements.remove(pos_index); Ok(()) } else { Err(vm.new_index_error("Index out of bounds!".to_owned())) @@ -636,23 +655,24 @@ impl PyList { let stop = slice.stop_index(vm)?; let step = slice.step_index(vm)?.unwrap_or_else(BigInt::one); + let elements = self.borrow_elements_mut(); if step.is_zero() { Err(vm.new_value_error("slice step cannot be zero".to_owned())) } else if step.is_positive() { - let range = self.get_slice_range(&start, &stop); + let range = PyList::get_slice_range(&start, &stop, elements.len()); if range.start < range.end { #[allow(clippy::range_plus_one)] match step.to_i32() { Some(1) => { - self._del_slice(range); + PyList::_del_slice(elements, range); Ok(()) } Some(num) => { - self._del_stepped_slice(range, num as usize); + PyList::_del_stepped_slice(elements, range, num as usize); Ok(()) } None => { - self._del_slice(range.start..range.start + 1); + PyList::_del_slice(elements, range.start..range.start + 1); Ok(()) } } @@ -665,31 +685,31 @@ impl PyList { // exclusive around stop, the lower number let start = start.as_ref().map(|x| { if *x == (-1).to_bigint().unwrap() { - self.get_len() + BigInt::one() //.to_bigint().unwrap() + elements.len() + BigInt::one() //.to_bigint().unwrap() } else { x + 1 } }); let stop = stop.as_ref().map(|x| { if *x == (-1).to_bigint().unwrap() { - self.get_len().to_bigint().unwrap() + elements.len().to_bigint().unwrap() } else { x + 1 } }); - let range = self.get_slice_range(&stop, &start); + let range = PyList::get_slice_range(&stop, &start, elements.len()); if range.start < range.end { match (-step).to_i32() { Some(1) => { - self._del_slice(range); + PyList::_del_slice(elements, range); Ok(()) } Some(num) => { - self._del_stepped_slice_reverse(range, num as usize); + PyList::_del_stepped_slice_reverse(elements, range, num as usize); Ok(()) } None => { - self._del_slice(range.end - 1..range.end); + PyList::_del_slice(elements, range.end - 1..range.end); Ok(()) } } @@ -700,14 +720,17 @@ impl PyList { } } - fn _del_slice(&self, range: Range) { - self.elements.borrow_mut().drain(range); + fn _del_slice(mut elements: RwLockWriteGuard<'_, Vec>, range: Range) { + elements.drain(range); } - fn _del_stepped_slice(&self, range: Range, step: usize) { + fn _del_stepped_slice( + mut elements: RwLockWriteGuard<'_, Vec>, + range: Range, + step: usize, + ) { // no easy way to delete stepped indexes so here is what we'll do let mut deleted = 0; - let mut elements = self.elements.borrow_mut(); let mut indexes = range.clone().step_by(step).peekable(); for i in range.clone() { @@ -725,10 +748,13 @@ impl PyList { elements.drain((range.end - deleted)..range.end); } - fn _del_stepped_slice_reverse(&self, range: Range, step: usize) { + fn _del_stepped_slice_reverse( + mut elements: RwLockWriteGuard<'_, Vec>, + range: Range, + step: usize, + ) { // no easy way to delete stepped indexes so here is what we'll do let mut deleted = 0; - let mut elements = self.elements.borrow_mut(); let mut indexes = range.clone().rev().step_by(step).peekable(); for i in range.clone().rev() { @@ -751,9 +777,9 @@ impl PyList { // replace list contents with [] for duration of sort. // this prevents keyfunc from messing with the list and makes it easy to // check if it tries to append elements to it. - let mut elements = self.elements.replace(vec![]); + 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); if !temp_elements.is_empty() { return Err(vm.new_value_error("list modified during sort".to_owned())); @@ -762,6 +788,15 @@ impl PyList { Ok(()) } + fn _replace( + mut elements: RwLockWriteGuard<'_, Vec>, + mut new: Vec, + ) -> Vec { + let old = elements.drain(..).collect(); + elements.append(&mut new); + old + } + #[pyslot] fn tp_new( cls: PyClassRef, @@ -850,6 +885,8 @@ pub struct PyListIterator { pub list: PyListRef, } +impl ThreadSafe for PyListIterator {} + impl PyValue for PyListIterator { fn class(vm: &VirtualMachine) -> PyClassRef { vm.ctx.listiterator_type() @@ -860,7 +897,7 @@ impl PyValue for PyListIterator { impl PyListIterator { #[pymethod(name = "__next__")] fn next(&self, vm: &VirtualMachine) -> PyResult { - let list = self.list.elements.borrow(); + let list = self.list.borrow_elements(); let pos = self.position.fetch_add(1); if let Some(obj) = list.get(pos) { Ok(obj.clone()) @@ -876,7 +913,7 @@ impl PyListIterator { #[pymethod(name = "__length_hint__")] fn length_hint(&self) -> usize { - let list = self.list.elements.borrow(); + let list = self.list.borrow_elements(); let pos = self.position.load(); list.len().saturating_sub(pos) } @@ -889,6 +926,8 @@ pub struct PyListReverseIterator { pub list: PyListRef, } +impl ThreadSafe for PyListReverseIterator {} + impl PyValue for PyListReverseIterator { fn class(vm: &VirtualMachine) -> PyClassRef { vm.ctx.listreverseiterator_type() @@ -899,7 +938,7 @@ impl PyValue for PyListReverseIterator { impl PyListReverseIterator { #[pymethod(name = "__next__")] fn next(&self, vm: &VirtualMachine) -> PyResult { - let list = self.list.elements.borrow(); + let list = self.list.borrow_elements(); let pos = self.position.fetch_sub(1); if pos > 0 { if let Some(ret) = list.get(pos as usize - 1) { diff --git a/vm/src/sequence.rs b/vm/src/sequence.rs index 2855c711a6..51c2368f58 100644 --- a/vm/src/sequence.rs +++ b/vm/src/sequence.rs @@ -1,6 +1,7 @@ use crate::pyobject::{IdProtocol, PyObjectRef, PyResult}; use crate::vm::VirtualMachine; use std::ops::Deref; +use std::sync::{RwLockReadGuard, RwLockWriteGuard}; type DynPyIter<'a> = Box + 'a>; @@ -49,6 +50,30 @@ where } } +impl SimpleSeq for RwLockReadGuard<'_, T> +where + T: SimpleSeq, +{ + fn len(&self) -> usize { + self.deref().len() + } + fn iter(&self) -> DynPyIter { + self.deref().iter() + } +} + +impl SimpleSeq for RwLockWriteGuard<'_, T> +where + T: SimpleSeq, +{ + fn len(&self) -> usize { + self.deref().len() + } + fn iter(&self) -> DynPyIter { + self.deref().iter() + } +} + // impl<'a, I> pub(crate) fn eq( From a67c2875d429804c74fbacf99a519effada82625 Mon Sep 17 00:00:00 2001 From: Aviv Palivoda Date: Fri, 17 Apr 2020 15:16:59 +0300 Subject: [PATCH 4/4] Use std::mem to swap elements --- vm/src/obj/objlist.rs | 25 ++++++++----------------- vm/src/pyobject.rs | 4 ++-- 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/vm/src/obj/objlist.rs b/vm/src/obj/objlist.rs index 81d9cc42e1..433cf44d7c 100644 --- a/vm/src/obj/objlist.rs +++ b/vm/src/obj/objlist.rs @@ -1,6 +1,6 @@ use std::fmt; use std::mem::size_of; -use std::ops::Range; +use std::ops::{DerefMut, Range}; use std::sync::{RwLock, RwLockReadGuard, RwLockWriteGuard}; use crossbeam_utils::atomic::AtomicCell; @@ -114,7 +114,7 @@ impl PyList { vm: &VirtualMachine, ) -> PyResult { let mut elements = Vec::::with_capacity(self.borrow_elements().len()); - for elem in self.borrow_elements().clone().iter() { + for elem in self.borrow_elements().iter() { match PyIntRef::try_from_object(vm, elem.clone()) { Ok(result) => match result.as_bigint().to_u8() { Some(result) => elements.push(result), @@ -498,10 +498,10 @@ impl PyList { #[pymethod(name = "__imul__")] fn imul(zelf: PyRef, counter: isize) -> PyRef { - let elements = zelf.borrow_elements_mut(); - let new_elements: Vec = + let mut elements = zelf.borrow_elements_mut(); + let mut new_elements: Vec = sequence::seq_mul(&elements, counter).cloned().collect(); - PyList::_replace(elements, new_elements); + std::mem::swap(elements.deref_mut(), &mut new_elements); zelf.clone() } @@ -777,26 +777,17 @@ impl PyList { // replace list contents with [] for duration of sort. // this prevents keyfunc from messing with the list and makes it easy to // check if it tries to append elements to it. - let mut elements = PyList::_replace(self.borrow_elements_mut(), vec![]); + let mut elements = std::mem::take(self.borrow_elements_mut().deref_mut()); do_sort(vm, &mut elements, options.key, options.reverse)?; - let temp_elements = PyList::_replace(self.borrow_elements_mut(), elements); + std::mem::swap(self.borrow_elements_mut().deref_mut(), &mut elements); - if !temp_elements.is_empty() { + if !elements.is_empty() { return Err(vm.new_value_error("list modified during sort".to_owned())); } Ok(()) } - fn _replace( - mut elements: RwLockWriteGuard<'_, Vec>, - mut new: Vec, - ) -> Vec { - let old = elements.drain(..).collect(); - elements.append(&mut new); - old - } - #[pyslot] fn tp_new( cls: PyClassRef, diff --git a/vm/src/pyobject.rs b/vm/src/pyobject.rs index 33a06a3aa6..f3c819b1b0 100644 --- a/vm/src/pyobject.rs +++ b/vm/src/pyobject.rs @@ -1222,8 +1222,8 @@ 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. +// Temporary hack to help with converting object that contain PyObjectRef to ThreadSafe. +// Should be removed before threading is allowed. Do not try this at home!!! unsafe impl Send for PyObject {} unsafe impl Sync for PyObject {}