From 7c2856d449e49071e700302f98037bfa94dd1dcf Mon Sep 17 00:00:00 2001 From: Noah <33094578+coolreader18@users.noreply.github.com> Date: Thu, 24 Sep 2020 22:25:32 -0500 Subject: [PATCH] Make dict repr more efficient --- common/src/cell.rs | 31 ++- vm/src/dictdatatype.rs | 509 +++++++++++++++++++++++------------------ vm/src/obj/objdict.rs | 2 +- vm/src/obj/objset.rs | 3 +- 4 files changed, 314 insertions(+), 231 deletions(-) diff --git a/common/src/cell.rs b/common/src/cell.rs index 5277cf8a0..5be98de32 100644 --- a/common/src/cell.rs +++ b/common/src/cell.rs @@ -119,7 +119,31 @@ cfg_if::cfg_if! { type RwLockInner = RefCell; type RwLockReadInner<'a, T> = Ref<'a, T>; type MappedRwLockReadInner<'a, T> = Ref<'a, T>; - type RwLockWriteInner<'a, T> = RefMut<'a, T>; + #[derive(Debug)] + struct RwLockWriteInner<'a, T: ?Sized>(&'a RefCell, RefMut<'a, T>); + impl<'a, T: ?Sized> RwLockWriteInner<'a, T> { + fn map(s: Self, f: F) -> MappedRwLockWriteInner<'a, U> + where + F: FnOnce(&mut T) -> &mut U, + { + RefMut::map(s.1, f) + } + fn downgrade(s: Self) -> RwLockReadInner<'a, T> { + drop(s.1); + read_rwlock(s.0) + } + } + impl<'a, T: ?Sized> Deref for RwLockWriteInner<'a, T> { + type Target = T; + fn deref(&self) -> &T { + self.1.deref() + } + } + impl<'a, T: ?Sized> DerefMut for RwLockWriteInner<'a, T> { + fn deref_mut(&mut self) -> &mut T { + self.1.deref_mut() + } + } type MappedRwLockWriteInner<'a, T> = RefMut<'a, T>; const fn new_rwlock(value: T) -> RwLockInner { RefCell::new(value) @@ -128,7 +152,7 @@ cfg_if::cfg_if! { m.borrow() } fn write_rwlock(m: &RwLockInner) -> RwLockWriteInner { - m.borrow_mut() + RwLockWriteInner(m, m.borrow_mut()) } } } @@ -229,6 +253,9 @@ impl<'a, T: ?Sized> PyRwLockWriteGuard<'a, T> { { PyMappedRwLockWriteGuard(RwLockWriteInner::map(s.0, f)) } + pub fn downgrade(s: Self) -> PyRwLockReadGuard<'a, T> { + PyRwLockReadGuard(RwLockWriteInner::downgrade(s.0)) + } } impl<'a, T: ?Sized> PyMappedRwLockWriteGuard<'a, T> { #[inline] diff --git a/vm/src/dictdatatype.rs b/vm/src/dictdatatype.rs index 4a33646f3..687a66259 100644 --- a/vm/src/dictdatatype.rs +++ b/vm/src/dictdatatype.rs @@ -7,7 +7,6 @@ use crate::obj::objstr::{PyString, PyStringRef}; use crate::pyobject::{BorrowValue, IdProtocol, IntoPyObject, PyObjectRef, PyResult}; use crate::vm::VirtualMachine; use rustpython_common::hash; -use std::collections::HashMap; use std::mem::size_of; // HashIndex is intended to be same size with hash::PyHash @@ -17,28 +16,51 @@ use std::mem::size_of; type HashValue = hash::PyHash; /// index calculated by resolving collision type HashIndex = hash::PyHash; -/// entry index mapped in indices +/// index into dict.indices +type IndexIndex = usize; +/// index into dict.entries type EntryIndex = usize; pub struct Dict { inner: PyRwLock>, } -struct DictInner { - size: usize, - indices: HashMap, - entries: Vec>>, +#[derive(Debug, Copy, Clone)] +enum IndexEntry { + Dummy, + Free, + Index(usize), } - -impl Clone for DictInner { - fn clone(&self) -> Self { - DictInner { - size: self.size, - indices: self.indices.clone(), - entries: self.entries.clone(), +impl IndexEntry { + const FREE: i64 = -1; + const DUMMY: i64 = -2; +} +impl From for IndexEntry { + fn from(idx: i64) -> Self { + match idx { + IndexEntry::FREE => IndexEntry::Free, + IndexEntry::DUMMY => IndexEntry::Dummy, + x => IndexEntry::Index(x as usize), } } } +impl From for i64 { + fn from(idx: IndexEntry) -> Self { + match idx { + IndexEntry::Free => IndexEntry::FREE, + IndexEntry::Dummy => IndexEntry::DUMMY, + IndexEntry::Index(i) => i as i64, + } + } +} + +#[derive(Clone)] +struct DictInner { + used: usize, + filled: usize, + indices: Vec, + entries: Vec>, +} impl Clone for Dict { fn clone(&self) -> Self { @@ -52,34 +74,110 @@ impl Default for Dict { fn default() -> Self { Dict { inner: PyRwLock::new(DictInner { - size: 0, - indices: HashMap::new(), + used: 0, + filled: 0, + indices: vec![IndexEntry::FREE; 8], entries: Vec::new(), }), } } } +#[derive(Clone)] struct DictEntry { hash: HashValue, key: PyObjectRef, + index: IndexIndex, value: T, } -impl Clone for DictEntry { - fn clone(&self) -> Self { - DictEntry { - hash: self.hash, - key: self.key.clone(), - value: self.value.clone(), +#[derive(Debug, PartialEq)] +pub struct DictSize { + indices_size: usize, + entries_size: usize, + used: usize, + filled: usize, +} + +struct GenIndexes { + idx: HashIndex, + perturb: HashValue, + mask: HashIndex, +} + +impl GenIndexes { + fn new(hash: HashValue, mask: HashIndex) -> Self { + let hash = hash.abs(); + Self { + idx: hash, + perturb: hash, + mask, } } + fn next(&mut self) -> usize { + let prev = self.idx; + self.idx = prev + .wrapping_mul(5) + .wrapping_add(self.perturb) + .wrapping_add(1); + self.perturb >>= 5; + (prev & self.mask) as usize + } } -#[derive(Debug)] -pub struct DictSize { - size: usize, - entries_size: usize, +impl DictInner { + fn resize(&mut self, new_size: usize) { + let new_size = { + let mut i = 1; + while i < new_size { + i <<= 1; + } + i + }; + self.indices = vec![IndexEntry::FREE; new_size]; + let mask = (new_size - 1) as i64; + for (entry_idx, entry) in self.entries.iter_mut().enumerate() { + let mut idxs = GenIndexes::new(entry.hash, mask); + loop { + let index_index = idxs.next(); + let idx = &mut self.indices[index_index]; + if *idx == IndexEntry::FREE { + *idx = entry_idx as i64; + entry.index = index_index; + break; + } + } + } + self.filled = self.used; + } + + fn unchecked_push( + &mut self, + index: IndexIndex, + hash_value: HashValue, + key: PyObjectRef, + value: T, + ) { + let entry = DictEntry { + hash: hash_value, + key, + value, + index, + }; + let entry_index = self.entries.len(); + self.entries.push(entry); + self.indices[index] = entry_index as i64; + self.used += 1; + } + + fn size(&self) -> DictSize { + DictSize { + indices_size: self.indices.len(), + entries_size: self.entries.len(), + used: self.used, + filled: self.filled, + } + } } impl Dict { @@ -91,116 +189,82 @@ impl Dict { self.inner.write() } - fn resize(&self) { - let mut inner = self.borrow_value_mut(); - let mut new_indices = HashMap::with_capacity(inner.size); - let mut new_entries = Vec::with_capacity(inner.size); - for maybe_entry in inner.entries.drain(0..) { - if let Some(entry) = maybe_entry { - let mut hash_index = entry.hash; - // Faster version of lookup. No equality checks here. - // We assume dict doesn't contatins any duplicate keys - while new_indices.contains_key(&hash_index) { - hash_index = Self::next_index(entry.hash, hash_index); - } - new_indices.insert(hash_index, new_entries.len()); - new_entries.push(Some(entry)); - } - } - inner.indices = new_indices; - inner.entries = new_entries; - } - - fn unchecked_push( - &self, - hash_index: HashIndex, - hash_value: HashValue, - key: PyObjectRef, - value: T, - ) { - let entry = DictEntry { - hash: hash_value, - key, - value, - }; - let mut inner = self.borrow_value_mut(); - let entry_index = inner.entries.len(); - inner.entries.push(Some(entry)); - inner.indices.insert(hash_index, entry_index); - inner.size += 1; - } - /// Store a key pub fn insert(&self, vm: &VirtualMachine, key: K, value: T) -> PyResult<()> where K: DictKey, { - // This does not need to be accurate so we can take the lock mutiple times. - let (indices_len, size) = { - let borrowed = self.borrow_value(); - (borrowed.indices.len(), borrowed.size) - }; - if indices_len > 2 * size { - self.resize(); - } + let hash = key.key_hash(vm)?; let _removed = loop { - match self.lookup(vm, &key)? { - LookupResult::Existing(index) => { - let mut inner = self.borrow_value_mut(); - // Update existing key - if let Some(ref mut entry) = inner.entries[index] { - // They entry might have changed since we did lookup. Should we update the key? + let (entry_index, index_index) = self.lookup(vm, &key, hash, None)?; + if let IndexEntry::Index(index) = entry_index { + let mut inner = self.borrow_value_mut(); + // Update existing key + if let Some(entry) = inner.entries.get_mut(index) { + if entry.index == index_index { let removed = std::mem::replace(&mut entry.value, value); // defer dec RC break Some(removed); } else { - // The dict was changed since we did lookup. Let's try again. - continue; + // stuff shifted around, let's try again + } + } else { + // The dict was changed since we did lookup. Let's try again. + } + } else { + // New key: + let mut inner = self.borrow_value_mut(); + inner.unchecked_push(index_index, hash, key.into_pyobject(vm), value); + if let IndexEntry::Free = entry_index { + inner.filled += 1; + if inner.filled * 3 > inner.indices.len() * 2 { + let new_size = inner.used * 4; + inner.resize(new_size) } } - LookupResult::NewIndex { - hash_index, - hash_value, - } => { - // New key: - self.unchecked_push(hash_index, hash_value, key.into_pyobject(vm), value); - break None; - } + break None; } }; Ok(()) } pub fn contains(&self, vm: &VirtualMachine, key: &K) -> PyResult { - if let LookupResult::Existing(_) = self.lookup(vm, key)? { - Ok(true) - } else { - Ok(false) - } + let (entry, _) = self.lookup(vm, key, key.key_hash(vm)?, None)?; + Ok(matches!(entry, IndexEntry::Index(_))) } /// Retrieve a key #[cfg_attr(feature = "flame-it", flame("Dict"))] pub fn get(&self, vm: &VirtualMachine, key: &K) -> PyResult> { - loop { - if let LookupResult::Existing(index) = self.lookup(vm, key)? { - if let Some(entry) = &self.borrow_value().entries[index] { - break Ok(Some(entry.value.clone())); + let hash = key.key_hash(vm)?; + let ret = loop { + let (entry, index_index) = self.lookup(vm, key, hash, None)?; + if let IndexEntry::Index(index) = entry { + let inner = self.borrow_value(); + if let Some(entry) = inner.entries.get(index) { + if entry.index == index_index { + break Some(entry.value.clone()); + } else { + // stuff shifted around, let's try again + } } else { // The dict was changed since we did lookup. Let's try again. continue; } } else { - break Ok(None); + break None; } - } + }; + Ok(ret) } pub fn clear(&self) { let _removed = { let mut inner = self.borrow_value_mut(); inner.indices.clear(); - inner.size = 0; + inner.indices.resize(8, IndexEntry::FREE); + inner.used = 0; + inner.filled = 0; // defer dec rc std::mem::replace(&mut inner.entries, Vec::new()) }; @@ -222,18 +286,14 @@ impl Dict { where K: DictKey, { + let hash = key.key_hash(vm)?; let deleted = loop { - if let LookupResult::Existing(entry_index) = self.lookup(vm, key)? { - let mut inner = self.borrow_value_mut(); - let entry = inner.entries.get_mut(entry_index).unwrap(); - if entry.is_some() { - // defer rc out of borrow - let deleted = std::mem::take(entry); - inner.size -= 1; - break deleted; + let lookup = self.lookup(vm, key, hash, None)?; + if let IndexEntry::Index(_) = lookup.0 { + if let Ok(Some(entry)) = self.pop_inner(lookup) { + break Some(entry); } else { // The dict was changed since we did lookup. Let's try again. - continue; } } else { break None; @@ -248,35 +308,27 @@ impl Dict { key: &PyObjectRef, value: T, ) -> PyResult<()> { + let hash = key.key_hash(vm)?; let _removed = loop { - match self.lookup(vm, key)? { - LookupResult::Existing(entry_index) => { - let mut inner = self.borrow_value_mut(); - let entry = inner.entries.get_mut(entry_index).unwrap(); - if entry.is_some() { - // defer dec RC - let entry = std::mem::take(entry); - inner.size -= 1; - break entry; - } else { - // The dict was changed since we did lookup. Let's try again. - continue; - } + let lookup = self.lookup(vm, key, hash, None)?; + let (entry, index_index) = lookup; + if let IndexEntry::Index(_) = entry { + if let Ok(Some(entry)) = self.pop_inner(lookup) { + break Some(entry); + } else { + // The dict was changed since we did lookup. Let's try again. } - LookupResult::NewIndex { - hash_value, - hash_index, - } => { - self.unchecked_push(hash_index, hash_value, key.clone(), value); - break None; - } - }; + } else { + let mut inner = self.borrow_value_mut(); + inner.unchecked_push(index_index, hash, key.clone(), value); + break None; + } }; Ok(()) } pub fn len(&self) -> usize { - self.borrow_value().size + self.borrow_value().used } pub fn is_empty(&self) -> bool { @@ -284,150 +336,153 @@ impl Dict { } pub fn size(&self) -> DictSize { - let inner = self.borrow_value(); - DictSize { - size: inner.size, - entries_size: inner.entries.len(), - } + self.borrow_value().size() } pub fn next_entry(&self, position: &mut EntryIndex) -> Option<(PyObjectRef, T)> { - self.borrow_value().entries[*position..] - .iter() - .find_map(|entry| { - *position += 1; - entry - .as_ref() - .map(|DictEntry { key, value, .. }| (key.clone(), value.clone())) - }) + self.borrow_value().entries.get(*position).map(|entry| { + *position += 1; + (entry.key.clone(), entry.value.clone()) + }) } pub fn len_from_entry_index(&self, position: EntryIndex) -> usize { - self.borrow_value().entries[position..] - .iter() - .flatten() - .count() + self.borrow_value().entries.len() - position } - pub fn has_changed_size(&self, position: &DictSize) -> bool { - let inner = self.borrow_value(); - position.size != inner.size || inner.entries.len() != position.entries_size + pub fn has_changed_size(&self, old: &DictSize) -> bool { + let current = self.borrow_value().size(); + current != *old } pub fn keys(&self) -> Vec { self.borrow_value() .entries .iter() - .filter_map(|v| v.as_ref().map(|v| v.key.clone())) + .map(|v| v.key.clone()) .collect() } /// Lookup the index for the given key. #[cfg_attr(feature = "flame-it", flame("Dict"))] - fn lookup(&self, vm: &VirtualMachine, key: &K) -> PyResult { - let hash_value = key.key_hash(vm)?; - let perturb = hash_value; - let mut hash_index: HashIndex = hash_value; - 'outer: loop { - let (entry, index) = loop { - let inner = self.borrow_value(); - if inner.indices.contains_key(&hash_index) { - // Now we have an index, lets check the key. - let index = inner.indices[&hash_index]; - if let Some(entry) = &inner.entries[index] { - // Okay, we have an entry at this place - if key.key_is(&entry.key) { - // Literally the same object - break 'outer Ok(LookupResult::Existing(index)); - } else if entry.hash == hash_value { - break (entry.clone(), index); - } else { - // entry mismatch. + fn lookup( + &self, + vm: &VirtualMachine, + key: &K, + hash_value: HashValue, + mut lock: Option>>, + ) -> PyResult { + let mut idxs = None; + let mut freeslot = None; + let ret = 'outer: loop { + let (entry_key, ret) = { + let inner = lock.take().unwrap_or_else(|| self.borrow_value()); + let idxs = idxs.get_or_insert_with(|| { + GenIndexes::new(hash_value, (inner.indices.len() - 1) as i64) + }); + loop { + let index_index = idxs.next(); + match IndexEntry::from(inner.indices[index_index]) { + IndexEntry::Dummy => { + if freeslot.is_none() { + freeslot = Some(index_index); + } + } + IndexEntry::Free => { + let idxs = match freeslot { + Some(free) => (IndexEntry::Dummy, free), + None => (IndexEntry::Free, index_index), + }; + return Ok(idxs); + } + IndexEntry::Index(i) => { + let entry = &inner.entries[i]; + let ret = (IndexEntry::Index(i), index_index); + if key.key_is(&entry.key) { + break 'outer ret; + } else if entry.hash == hash_value { + break (entry.key.clone(), ret); + } else { + // entry mismatch + } } - } else { - // Removed entry, continue search... } - } else { - // Hash not in table, we are at free slot now. - break 'outer Ok(LookupResult::NewIndex { - hash_value, - hash_index, - }); + // warn!("Perturb value: {}", i); } - // Update i to next probe location: - hash_index = Self::next_index(perturb, hash_index) - // warn!("Perturb value: {}", i); }; // This comparison needs to be done outside the lock. - if key.key_eq(vm, &entry.key)? { - break Ok(LookupResult::Existing(index)); + if key.key_eq(vm, &entry_key)? { + break 'outer ret; } else { - // entry mismatch. + // hash collision } - // Update i to next probe location: - hash_index = Self::next_index(perturb, hash_index) // warn!("Perturb value: {}", i); - } + }; + Ok(ret) } - fn next_index(perturb: HashValue, hash_index: HashIndex) -> HashIndex { - hash_index - .wrapping_mul(5) - .wrapping_add(perturb) - .wrapping_add(1) + // returns Err(()) if changed since lookup + fn pop_inner(&self, lookup: LookupResult) -> Result>, ()> { + let (entry_index, index_index) = lookup; + let entry_index = if let IndexEntry::Index(entry_index) = entry_index { + entry_index + } else { + return Ok(None); + }; + let mut inner = self.borrow_value_mut(); + if matches!(inner.entries.get(entry_index), Some(entry) if entry.index == index_index) { + // all good + } else { + // The dict was changed since we did lookup. Let's try again. + return Err(()); + }; + inner.indices[index_index] = IndexEntry::DUMMY; + inner.used -= 1; + let removed = if entry_index == inner.used { + inner.entries.pop().unwrap() + } else { + let last_index = inner.entries.last().unwrap().index; + let removed = inner.entries.swap_remove(entry_index); + inner.indices[last_index] = entry_index as i64; + removed + }; + Ok(Some(removed)) } /// Retrieve and delete a key pub fn pop(&self, vm: &VirtualMachine, key: &K) -> PyResult> { + let hash_value = key.key_hash(vm)?; let removed = loop { - if let LookupResult::Existing(index) = self.lookup(vm, key)? { - let mut inner = self.borrow_value_mut(); - if let Some(entry) = inner.entries.get_mut(index) { - let popped = std::mem::take(entry); - inner.size -= 1; - break Some(popped.unwrap().value); - } else { - // The dict was changed since we did lookup. Let's try again. - continue; - } + let lookup = self.lookup(vm, key, hash_value, None)?; + if let Ok(ret) = self.pop_inner(lookup) { + break ret.map(|e| e.value); } else { - break None; + // changed since lookup, loop again } }; Ok(removed) } - pub fn pop_front(&self) -> Option<(PyObjectRef, T)> { - let mut position = 0; + pub fn pop_back(&self) -> Option<(PyObjectRef, T)> { let mut inner = self.borrow_value_mut(); - let first_item = inner.entries.iter().find_map(|entry| { - position += 1; - entry - .as_ref() - .map(|DictEntry { key, value, .. }| (key.clone(), value.clone())) - }); - if let Some(item) = first_item { - inner.entries[position - 1] = None; - inner.size -= 1; - Some(item) - } else { - None - } + inner.entries.pop().map(|entry| { + inner.used -= 1; + inner.indices[entry.index] = IndexEntry::DUMMY; + (entry.key, entry.value) + }) } pub fn sizeof(&self) -> usize { - size_of::() + self.borrow_value().size * size_of::>() + let inner = self.borrow_value(); + size_of::() + + size_of::>() + + inner.indices.len() * size_of::() + + inner.entries.len() * size_of::>() } } -enum LookupResult { - NewIndex { - hash_value: HashValue, - hash_index: HashIndex, - }, // return not found, index into indices - Existing(EntryIndex), // Existing record, index into entries -} +type LookupResult = (IndexEntry, IndexIndex); /// Types implementing this trait can be used to index /// the dictionary. Typical usecases are: diff --git a/vm/src/obj/objdict.rs b/vm/src/obj/objdict.rs index 7023015f7..d703836ae 100644 --- a/vm/src/obj/objdict.rs +++ b/vm/src/obj/objdict.rs @@ -376,7 +376,7 @@ impl PyDict { #[pymethod] fn popitem(&self, vm: &VirtualMachine) -> PyResult { - if let Some((key, value)) = self.entries.pop_front() { + if let Some((key, value)) = self.entries.pop_back() { Ok(vm.ctx.new_tuple(vec![key, value])) } else { let err_msg = vm.ctx.new_str("popitem(): dictionary is empty"); diff --git a/vm/src/obj/objset.rs b/vm/src/obj/objset.rs index d0df0b693..e680c1f98 100644 --- a/vm/src/obj/objset.rs +++ b/vm/src/obj/objset.rs @@ -234,7 +234,8 @@ impl PySetInner { } fn pop(&self, vm: &VirtualMachine) -> PyResult { - if let Some((key, _)) = self.content.pop_front() { + // TODO: should be pop_front, but that requires rearranging every index + if let Some((key, _)) = self.content.pop_back() { Ok(key) } else { let err_msg = vm.ctx.new_str("pop from an empty set");