Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use linked list in ReleasePool #281

Merged
merged 7 commits into from
Nov 24, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion benches/bench_dict.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(|| {
Expand Down
110 changes: 77 additions & 33 deletions src/pythonrun.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,8 @@ impl Drop for GILGuard {

/// Release pool
struct ReleasePool {
owned: Vec<NonNull<ffi::PyObject>>,
borrowed: Vec<NonNull<ffi::PyObject>>,
owned: ArrayList<NonNull<ffi::PyObject>>,
borrowed: ArrayList<NonNull<ffi::PyObject>>,
pointers: *mut Vec<NonNull<ffi::PyObject>>,
obj: Vec<Box<any::Any>>,
p: spin::Mutex<*mut Vec<NonNull<ffi::PyObject>>>,
Expand All @@ -118,8 +118,8 @@ struct ReleasePool {
impl ReleasePool {
fn new() -> ReleasePool {
ReleasePool {
owned: Vec::with_capacity(256),
borrowed: Vec::with_capacity(256),
owned: ArrayList::new(),
borrowed: ArrayList::new(),
pointers: Box::into_raw(Box::new(Vec::with_capacity(256))),
obj: Vec::with_capacity(8),
p: spin::Mutex::new(Box::into_raw(Box::new(Vec::with_capacity(256)))),
Expand All @@ -128,39 +128,30 @@ 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
// release PyObjects
for ptr in vec.iter_mut() {
ffi::Py_DECREF(ptr.as_ptr());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #282 because this is lurking UB

}
vec.set_len(0);
}

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);
}

let len = self.borrowed.len();
if borrowed < len {
self.borrowed.set_len(borrowed);
// Release owned objects(call decref)
while owned < self.owned.len() {
let last = self.owned.pop_back().unwrap();
ffi::Py_DECREF(last.as_ptr());
}
// Release borrowed objects(don't call decref)
self.borrowed.truncate(borrowed);

if pointers {
self.release_pointers();
Expand Down Expand Up @@ -232,23 +223,18 @@ pub unsafe fn register_any<'p, T: 'static>(obj: T) -> &'p T {
}

pub unsafe fn register_pointer(obj: NonNull<ffi::PyObject>) {
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<ffi::PyObject>) -> &PyObjectRef {
let pool: &'static mut ReleasePool = &mut *POOL;
pool.owned.push(obj);
&*(&pool.owned[pool.owned.len() - 1] as *const _ as *const PyObjectRef)
let pool = &mut *POOL;
&*(pool.owned.push_back(obj) as *const _ as *const PyObjectRef)
}

pub unsafe fn register_borrowed(_py: Python, obj: NonNull<ffi::PyObject>) -> &PyObjectRef {
let pool: &'static mut ReleasePool = &mut *POOL;
pool.borrowed.push(obj);
&*(&pool.borrowed[pool.borrowed.len() - 1] as *const _ as *const PyObjectRef)
let pool = &mut *POOL;
&*(pool.borrowed.push_back(obj) as *const _ as *const PyObjectRef)
}

impl GILGuard {
Expand Down Expand Up @@ -278,6 +264,64 @@ impl GILGuard {
}
}

use self::array_list::ArrayList;

mod array_list {
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<T> {
inner: LinkedList<[T; BLOCK_SIZE]>,
length: usize,
}

impl<T: Clone> ArrayList<T> {
pub fn new() -> Self {
ArrayList {
inner: LinkedList::new(),
length: 0,
}
}
pub fn push_back(&mut self, item: T) -> &T {
let next_idx = self.next_idx();
if next_idx == 0 {
self.inner.push_back(unsafe { mem::uninitialized() });
}
self.inner.back_mut().unwrap()[next_idx] = item;
self.length += 1;
&self.inner.back().unwrap()[next_idx]
}
pub fn pop_back(&mut self) -> Option<T> {
self.length -= 1;
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
}
pub fn truncate(&mut self, new_len: usize) {
if self.length <= new_len {
return;
}
while self.inner.len() > (new_len + BLOCK_SIZE - 1) / BLOCK_SIZE {
self.inner.pop_back();
}
self.length = new_len;
}
fn next_idx(&self) -> usize {
self.length % BLOCK_SIZE
}
}
}

#[cfg(test)]
mod test {
use super::{GILPool, NonNull, ReleasePool, POOL};
Expand Down
17 changes: 17 additions & 0 deletions tests/test_dict_iter.rs
Original file line number Diff line number Diff line change
@@ -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);
}