From 25a5b74ba63b2d0b9e077e7d690b0a4e1330ed2e Mon Sep 17 00:00:00 2001 From: kngwyu Date: Sun, 18 Nov 2018 12:13:20 +0900 Subject: [PATCH 1/7] Use linked list in ReleasePool --- src/pythonrun.rs | 40 ++++++++++++++++++---------------------- tests/test_dict_iter.rs | 17 +++++++++++++++++ 2 files changed, 35 insertions(+), 22 deletions(-) create mode 100644 tests/test_dict_iter.rs diff --git a/src/pythonrun.rs b/src/pythonrun.rs index 3b98827d1e5..3fea68118d3 100644 --- a/src/pythonrun.rs +++ b/src/pythonrun.rs @@ -5,6 +5,7 @@ use crate::types::PyObjectRef; use spin; use std::ptr::NonNull; use std::{any, marker, rc, sync}; +use std::collections::LinkedList; static START: sync::Once = sync::ONCE_INIT; static START_PYO3: sync::Once = sync::ONCE_INIT; @@ -108,20 +109,20 @@ impl Drop for GILGuard { /// Release pool struct ReleasePool { - owned: Vec>, - borrowed: Vec>, + owned: LinkedList>, + borrowed: LinkedList>, pointers: *mut Vec>, - obj: Vec>, + obj: LinkedList>, p: spin::Mutex<*mut Vec>>, } impl ReleasePool { fn new() -> ReleasePool { ReleasePool { - owned: Vec::with_capacity(256), - borrowed: Vec::with_capacity(256), + owned: LinkedList::new(), + borrowed: LinkedList::new(), pointers: Box::into_raw(Box::new(Vec::with_capacity(256))), - obj: Vec::with_capacity(8), + obj: LinkedList::new(), p: spin::Mutex::new(Box::into_raw(Box::new(Vec::with_capacity(256)))), } } @@ -149,17 +150,12 @@ impl ReleasePool { } pub unsafe fn drain(&mut self, owned: usize, borrowed: usize, pointers: bool) { - let len = self.owned.len(); - if owned < len { - for ptr in &mut self.owned[owned..len] { - ffi::Py_DECREF(ptr.as_ptr()); - } - self.owned.set_len(owned); + while owned < self.owned.len() { + let last = self.owned.pop_back().unwrap(); + ffi::Py_DECREF(last.as_ptr()); } - - let len = self.borrowed.len(); - if borrowed < len { - self.borrowed.set_len(borrowed); + while borrowed < self.borrowed.len() { + self.borrowed.pop_back(); } if pointers { @@ -222,9 +218,9 @@ impl Drop for GILPool { pub unsafe fn register_any<'p, T: 'static>(obj: T) -> &'p T { let pool: &'static mut ReleasePool = &mut *POOL; - pool.obj.push(Box::new(obj)); + pool.obj.push_back(Box::new(obj)); pool.obj - .last() + .back() .unwrap() .as_ref() .downcast_ref::() @@ -241,14 +237,14 @@ pub unsafe fn register_pointer(obj: NonNull) { pub unsafe fn register_owned(_py: Python, obj: NonNull) -> &PyObjectRef { let pool: &'static mut ReleasePool = &mut *POOL; - pool.owned.push(obj); - &*(&pool.owned[pool.owned.len() - 1] as *const _ as *const PyObjectRef) + pool.owned.push_back(obj); + &*(pool.owned.back().unwrap() as *const _ as *const PyObjectRef) } pub unsafe fn register_borrowed(_py: Python, obj: NonNull) -> &PyObjectRef { let pool: &'static mut ReleasePool = &mut *POOL; - pool.borrowed.push(obj); - &*(&pool.borrowed[pool.borrowed.len() - 1] as *const _ as *const PyObjectRef) + pool.borrowed.push_back(obj); + &*(pool.borrowed.back().unwrap() as *const _ as *const PyObjectRef) } impl GILGuard { diff --git a/tests/test_dict_iter.rs b/tests/test_dict_iter.rs new file mode 100644 index 00000000000..f4e36017b02 --- /dev/null +++ b/tests/test_dict_iter.rs @@ -0,0 +1,17 @@ +extern crate pyo3; + +use pyo3::{prelude::*, types::IntoPyDict}; + +#[test] +fn iter_dict_nosegv() { + let gil = Python::acquire_gil(); + let py = gil.python(); + const LEN: usize = 10_000_000; + let dict = (0..LEN as u64).map(|i| (i, i * 2)).into_py_dict(py); + let mut sum = 0; + for (k, _v) in dict.iter() { + let i: u64 = k.extract().unwrap(); + sum += i; + } + assert_eq!(sum, 49999995000000); +} From be7784697930a3c0174d4b08a53463fefe778bc5 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Sun, 18 Nov 2018 19:15:40 +0900 Subject: [PATCH 2/7] Use LinkedList<[T; 256]> in ReleasePool --- benches/bench_dict.rs | 2 +- src/pythonrun.rs | 74 +++++++++++++++++++++++++++++++++++-------- 2 files changed, 62 insertions(+), 14 deletions(-) diff --git a/benches/bench_dict.rs b/benches/bench_dict.rs index 620550da2fe..114f38eb0f2 100644 --- a/benches/bench_dict.rs +++ b/benches/bench_dict.rs @@ -9,7 +9,7 @@ use pyo3::{prelude::*, types::IntoPyDict}; fn iter_dict(b: &mut Bencher) { let gil = Python::acquire_gil(); let py = gil.python(); - const LEN: usize = 1_000_000; + const LEN: usize = 1_000_00; let dict = (0..LEN as u64).map(|i| (i, i * 2)).into_py_dict(py); let mut sum = 0; b.iter(|| { diff --git a/src/pythonrun.rs b/src/pythonrun.rs index 3fea68118d3..e99db0c4d1f 100644 --- a/src/pythonrun.rs +++ b/src/pythonrun.rs @@ -5,7 +5,6 @@ use crate::types::PyObjectRef; use spin; use std::ptr::NonNull; use std::{any, marker, rc, sync}; -use std::collections::LinkedList; static START: sync::Once = sync::ONCE_INIT; static START_PYO3: sync::Once = sync::ONCE_INIT; @@ -109,20 +108,20 @@ impl Drop for GILGuard { /// Release pool struct ReleasePool { - owned: LinkedList>, - borrowed: LinkedList>, + owned: ArrayList>, + borrowed: ArrayList>, pointers: *mut Vec>, - obj: LinkedList>, + obj: Vec>, p: spin::Mutex<*mut Vec>>, } impl ReleasePool { fn new() -> ReleasePool { ReleasePool { - owned: LinkedList::new(), - borrowed: LinkedList::new(), + owned: ArrayList::new(), + borrowed: ArrayList::new(), pointers: Box::into_raw(Box::new(Vec::with_capacity(256))), - obj: LinkedList::new(), + obj: Vec::new(), p: spin::Mutex::new(Box::into_raw(Box::new(Vec::with_capacity(256)))), } } @@ -218,9 +217,9 @@ impl Drop for GILPool { pub unsafe fn register_any<'p, T: 'static>(obj: T) -> &'p T { let pool: &'static mut ReleasePool = &mut *POOL; - pool.obj.push_back(Box::new(obj)); + pool.obj.push(Box::new(obj)); pool.obj - .back() + .last() .unwrap() .as_ref() .downcast_ref::() @@ -237,14 +236,12 @@ pub unsafe fn register_pointer(obj: NonNull) { pub unsafe fn register_owned(_py: Python, obj: NonNull) -> &PyObjectRef { let pool: &'static mut ReleasePool = &mut *POOL; - pool.owned.push_back(obj); - &*(pool.owned.back().unwrap() as *const _ as *const PyObjectRef) + &*(pool.owned.push_back(obj) as *const _ as *const PyObjectRef) } pub unsafe fn register_borrowed(_py: Python, obj: NonNull) -> &PyObjectRef { let pool: &'static mut ReleasePool = &mut *POOL; - pool.borrowed.push_back(obj); - &*(pool.borrowed.back().unwrap() as *const _ as *const PyObjectRef) + &*(pool.borrowed.push_back(obj) as *const _ as *const PyObjectRef) } impl GILGuard { @@ -274,6 +271,57 @@ impl GILGuard { } } +use self::array_list::ArrayList; + +mod array_list { + use std::mem; + use std::collections::LinkedList; + + const BLOCK_SIZE: usize = 256; + pub(super) struct ArrayList { + inner: LinkedList<[T; BLOCK_SIZE]>, + last_length: usize, + length: usize, + } + + impl ArrayList { + pub fn new() -> Self { + let init: [T; BLOCK_SIZE] = unsafe { mem::uninitialized() }; + let mut inner = LinkedList::new(); + inner.push_back(init); + ArrayList { + inner, + last_length: 0, + length: 0, + } + } + pub fn push_back(&mut self, item: T) -> &T { + if self.last_length == BLOCK_SIZE { + self.last_length = 0; + self.inner.push_back(unsafe { mem::uninitialized() }); + } + self.inner.back_mut().unwrap()[self.last_length] = item; + self.last_length += 1; + self.length += 1; + self.inner.back().unwrap().last().unwrap() + } + pub fn pop_back(&mut self) -> Option<&T> { + if self.last_length == 0 { + let _ = self.inner.pop_back()?; + self.last_length = BLOCK_SIZE; + } + self.last_length -= 1; + self.length -= 1; + self.inner.back().map(|arr| { + &arr[self.last_length] + }) + } + pub fn len(&self) -> usize { + self.length + } + } +} + #[cfg(test)] mod test { use super::{GILPool, NonNull, ReleasePool, POOL}; From 4c38b21e49b94d294d3042577a4fc8a76897d8c2 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Sun, 18 Nov 2018 19:56:16 +0900 Subject: [PATCH 3/7] Fix ArrayList::push_back --- src/pythonrun.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pythonrun.rs b/src/pythonrun.rs index e99db0c4d1f..019aa6cb4d5 100644 --- a/src/pythonrun.rs +++ b/src/pythonrun.rs @@ -303,7 +303,7 @@ mod array_list { self.inner.back_mut().unwrap()[self.last_length] = item; self.last_length += 1; self.length += 1; - self.inner.back().unwrap().last().unwrap() + &self.inner.back().unwrap()[self.last_length - 1] } pub fn pop_back(&mut self) -> Option<&T> { if self.last_length == 0 { From acbab2b3f0300a5144d6809d0406ebbd6b48ed7a Mon Sep 17 00:00:00 2001 From: kngwyu Date: Sun, 18 Nov 2018 22:10:13 +0900 Subject: [PATCH 4/7] Use ArrayList::truncate for borrowed pointers --- src/pythonrun.rs | 49 ++++++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 22 deletions(-) diff --git a/src/pythonrun.rs b/src/pythonrun.rs index 019aa6cb4d5..21ef5d1b2c4 100644 --- a/src/pythonrun.rs +++ b/src/pythonrun.rs @@ -149,13 +149,13 @@ impl ReleasePool { } pub unsafe fn drain(&mut self, owned: usize, borrowed: usize, pointers: bool) { + // Release owned objects(call decref) while owned < self.owned.len() { let last = self.owned.pop_back().unwrap(); ffi::Py_DECREF(last.as_ptr()); } - while borrowed < self.borrowed.len() { - self.borrowed.pop_back(); - } + // Release borrowed objects(don't call decref) + self.borrowed.truncate(borrowed); if pointers { self.release_pointers(); @@ -274,51 +274,56 @@ impl GILGuard { use self::array_list::ArrayList; mod array_list { - use std::mem; use std::collections::LinkedList; + use std::mem; const BLOCK_SIZE: usize = 256; + + /// A container type for Release Pool + /// See #271 for why this is crated pub(super) struct ArrayList { inner: LinkedList<[T; BLOCK_SIZE]>, - last_length: usize, length: usize, } impl ArrayList { pub fn new() -> Self { - let init: [T; BLOCK_SIZE] = unsafe { mem::uninitialized() }; - let mut inner = LinkedList::new(); - inner.push_back(init); ArrayList { - inner, - last_length: 0, + inner: LinkedList::new(), length: 0, } } pub fn push_back(&mut self, item: T) -> &T { - if self.last_length == BLOCK_SIZE { - self.last_length = 0; + let last_idx = self.last_idx(); + if last_idx == 0 { self.inner.push_back(unsafe { mem::uninitialized() }); } - self.inner.back_mut().unwrap()[self.last_length] = item; - self.last_length += 1; + self.inner.back_mut().unwrap()[last_idx] = item; self.length += 1; - &self.inner.back().unwrap()[self.last_length - 1] + &self.inner.back().unwrap()[last_idx] } pub fn pop_back(&mut self) -> Option<&T> { - if self.last_length == 0 { - let _ = self.inner.pop_back()?; - self.last_length = BLOCK_SIZE; + if self.last_idx() == 0 { + self.inner.pop_back()?; } - self.last_length -= 1; self.length -= 1; - self.inner.back().map(|arr| { - &arr[self.last_length] - }) + self.inner.back().map(|arr| &arr[self.last_idx()]) } pub fn len(&self) -> usize { self.length } + pub fn truncate(&mut self, new_len: usize) { + if self.length <= new_len { + return; + } + while self.inner.len() > new_len / BLOCK_SIZE + 1 { + self.inner.pop_back(); + } + self.length = new_len; + } + fn last_idx(&self) -> usize { + self.length % BLOCK_SIZE + } } } From 63ec65d5ac17dc4f4468fedf6ece7b344f0b2a7d Mon Sep 17 00:00:00 2001 From: kngwyu Date: Sun, 18 Nov 2018 23:14:00 +0900 Subject: [PATCH 5/7] Refactor ReleasePool --- src/pythonrun.rs | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/src/pythonrun.rs b/src/pythonrun.rs index 21ef5d1b2c4..7890e7cd909 100644 --- a/src/pythonrun.rs +++ b/src/pythonrun.rs @@ -128,21 +128,17 @@ impl ReleasePool { unsafe fn release_pointers(&mut self) { let mut v = self.p.lock(); - - // vec of pointers - let ptr = *v; - let vec: &'static mut Vec<_> = &mut *ptr; + let vec = &mut **v; if vec.is_empty() { return; } // switch vectors - *v = self.pointers; - self.pointers = ptr; + std::mem::swap(&mut self.pointers, &mut *v); drop(v); - // release py objects - for ptr in vec.iter_mut() { + // release PyObjects + for ptr in vec.iter() { ffi::Py_DECREF(ptr.as_ptr()); } vec.set_len(0); @@ -227,20 +223,17 @@ pub unsafe fn register_any<'p, T: 'static>(obj: T) -> &'p T { } pub unsafe fn register_pointer(obj: NonNull) { - let pool: &'static mut ReleasePool = &mut *POOL; - - let mut v = pool.p.lock(); - let pool: &'static mut Vec<_> = &mut *(*v); - pool.push(obj); + let pool = &mut *POOL; + (**pool.p.lock()).push(obj); } pub unsafe fn register_owned(_py: Python, obj: NonNull) -> &PyObjectRef { - let pool: &'static mut ReleasePool = &mut *POOL; + let pool = &mut *POOL; &*(pool.owned.push_back(obj) as *const _ as *const PyObjectRef) } pub unsafe fn register_borrowed(_py: Python, obj: NonNull) -> &PyObjectRef { - let pool: &'static mut ReleasePool = &mut *POOL; + let pool = &mut *POOL; &*(pool.borrowed.push_back(obj) as *const _ as *const PyObjectRef) } From 949097874ff4fab47ff789cf15db92c331e7b9ec Mon Sep 17 00:00:00 2001 From: kngwyu Date: Mon, 19 Nov 2018 00:26:02 +0900 Subject: [PATCH 6/7] Fix bug in ArrayList::pop_back --- src/pythonrun.rs | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/pythonrun.rs b/src/pythonrun.rs index 7890e7cd909..1e8764850a2 100644 --- a/src/pythonrun.rs +++ b/src/pythonrun.rs @@ -279,7 +279,7 @@ mod array_list { length: usize, } - impl ArrayList { + impl ArrayList { pub fn new() -> Self { ArrayList { inner: LinkedList::new(), @@ -287,20 +287,22 @@ mod array_list { } } pub fn push_back(&mut self, item: T) -> &T { - let last_idx = self.last_idx(); - if last_idx == 0 { + let next_idx = self.next_idx(); + if next_idx == 0 { self.inner.push_back(unsafe { mem::uninitialized() }); } - self.inner.back_mut().unwrap()[last_idx] = item; + self.inner.back_mut().unwrap()[next_idx] = item; self.length += 1; - &self.inner.back().unwrap()[last_idx] + &self.inner.back().unwrap()[next_idx] } - pub fn pop_back(&mut self) -> Option<&T> { - if self.last_idx() == 0 { - self.inner.pop_back()?; - } + pub fn pop_back(&mut self) -> Option { self.length -= 1; - self.inner.back().map(|arr| &arr[self.last_idx()]) + let current_idx = self.next_idx(); + if self.length >= BLOCK_SIZE && current_idx == 0 { + let last_list = self.inner.pop_back()?; + return Some(last_list[0].clone()); + } + self.inner.back().map(|arr| arr[current_idx].clone()) } pub fn len(&self) -> usize { self.length @@ -314,7 +316,7 @@ mod array_list { } self.length = new_len; } - fn last_idx(&self) -> usize { + fn next_idx(&self) -> usize { self.length % BLOCK_SIZE } } From 119e0abde2292ada8724a278ed37e5d731ca6149 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Tue, 20 Nov 2018 16:21:36 +0900 Subject: [PATCH 7/7] Fix ArrayList::truncate --- src/pythonrun.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/pythonrun.rs b/src/pythonrun.rs index 1e8764850a2..357641ce954 100644 --- a/src/pythonrun.rs +++ b/src/pythonrun.rs @@ -121,7 +121,7 @@ impl ReleasePool { owned: ArrayList::new(), borrowed: ArrayList::new(), pointers: Box::into_raw(Box::new(Vec::with_capacity(256))), - obj: Vec::new(), + obj: Vec::with_capacity(8), p: spin::Mutex::new(Box::into_raw(Box::new(Vec::with_capacity(256)))), } } @@ -138,7 +138,7 @@ impl ReleasePool { drop(v); // release PyObjects - for ptr in vec.iter() { + for ptr in vec.iter_mut() { ffi::Py_DECREF(ptr.as_ptr()); } vec.set_len(0); @@ -311,7 +311,7 @@ mod array_list { if self.length <= new_len { return; } - while self.inner.len() > new_len / BLOCK_SIZE + 1 { + while self.inner.len() > (new_len + BLOCK_SIZE - 1) / BLOCK_SIZE { self.inner.pop_back(); } self.length = new_len;