Skip to content

Commit 65cde9a

Browse files
coolreader18coolreader18
committed
Optimize size of WeakRefList
The outer struct is only 1 word now, and inside the box it's a byte for the mutex (which gets padded to a word) + a word for the linked list (we don't use the tail) + a word for the pointer to the object.
1 parent e51cb82 commit 65cde9a

File tree

2 files changed

+72
-61
lines changed

2 files changed

+72
-61
lines changed

‎common/src/linked_list.rs‎

Lines changed: 38 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -51,9 +51,8 @@ pub struct LinkedList<L, T> {
5151
/// Linked list head
5252
head: Option<NonNull<T>>,
5353

54-
/// Linked list tail
55-
tail: Option<NonNull<T>>,
56-
54+
// /// Linked list tail
55+
// tail: Option<NonNull<T>>,
5756
/// Node type marker.
5857
_marker: PhantomData<*const L>,
5958
}
@@ -139,7 +138,7 @@ impl<L, T> LinkedList<L, T> {
139138
pub const fn new() -> LinkedList<L, T> {
140139
LinkedList {
141140
head: None,
142-
tail: None,
141+
// tail: None,
143142
_marker: PhantomData,
144143
}
145144
}
@@ -162,40 +161,41 @@ impl<L: Link> LinkedList<L, L::Target> {
162161

163162
self.head = Some(ptr);
164163

165-
if self.tail.is_none() {
166-
self.tail = Some(ptr);
167-
}
164+
// if self.tail.is_none() {
165+
// self.tail = Some(ptr);
166+
// }
168167
}
169168
}
170169

171-
/// Removes the last element from a list and returns it, or None if it is
172-
/// empty.
173-
pub fn pop_back(&mut self) -> Option<L::Handle> {
174-
unsafe {
175-
let last = self.tail?;
176-
self.tail = L::pointers(last).as_ref().get_prev();
170+
// /// Removes the last element from a list and returns it, or None if it is
171+
// /// empty.
172+
// pub fn pop_back(&mut self) -> Option<L::Handle> {
173+
// unsafe {
174+
// let last = self.tail?;
175+
// self.tail = L::pointers(last).as_ref().get_prev();
177176

178-
if let Some(prev) = L::pointers(last).as_ref().get_prev() {
179-
L::pointers(prev).as_mut().set_next(None);
180-
} else {
181-
self.head = None
182-
}
177+
// if let Some(prev) = L::pointers(last).as_ref().get_prev() {
178+
// L::pointers(prev).as_mut().set_next(None);
179+
// } else {
180+
// self.head = None
181+
// }
183182

184-
L::pointers(last).as_mut().set_prev(None);
185-
L::pointers(last).as_mut().set_next(None);
183+
// L::pointers(last).as_mut().set_prev(None);
184+
// L::pointers(last).as_mut().set_next(None);
186185

187-
Some(L::from_raw(last))
188-
}
189-
}
186+
// Some(L::from_raw(last))
187+
// }
188+
// }
190189

191190
/// Returns whether the linked list does not contain any node
192191
pub fn is_empty(&self) -> bool {
193-
if self.head.is_some() {
194-
return false;
195-
}
192+
self.head.is_none()
193+
// if self.head.is_some() {
194+
// return false;
195+
// }
196196

197-
assert!(self.tail.is_none());
198-
true
197+
// assert!(self.tail.is_none());
198+
// true
199199
}
200200

201201
/// Removes the specified node from the list
@@ -224,12 +224,12 @@ impl<L: Link> LinkedList<L, L::Target> {
224224
.as_mut()
225225
.set_prev(L::pointers(node).as_ref().get_prev());
226226
} else {
227-
// This might be the last item in the list
228-
if self.tail != Some(node) {
229-
return None;
230-
}
227+
// // This might be the last item in the list
228+
// if self.tail != Some(node) {
229+
// return None;
230+
// }
231231

232-
self.tail = L::pointers(node).as_ref().get_prev();
232+
// self.tail = L::pointers(node).as_ref().get_prev();
233233
}
234234

235235
L::pointers(node).as_mut().set_next(None);
@@ -238,10 +238,10 @@ impl<L: Link> LinkedList<L, L::Target> {
238238
Some(L::from_raw(node))
239239
}
240240

241-
pub fn last(&self) -> Option<&L::Target> {
242-
let tail = self.tail.as_ref()?;
243-
unsafe { Some(&*tail.as_ptr()) }
244-
}
241+
// pub fn last(&self) -> Option<&L::Target> {
242+
// let tail = self.tail.as_ref()?;
243+
// unsafe { Some(&*tail.as_ptr()) }
244+
// }
245245

246246
pub fn count(&self) -> usize {
247247
std::iter::successors(self.head, |node| unsafe {
@@ -255,7 +255,7 @@ impl<L: Link> fmt::Debug for LinkedList<L, L::Target> {
255255
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
256256
f.debug_struct("LinkedList")
257257
.field("head", &self.head)
258-
.field("tail", &self.tail)
258+
// .field("tail", &self.tail)
259259
.finish()
260260
}
261261
}

‎vm/src/pyobjectrc.rs‎

Lines changed: 34 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,11 @@ use std::mem::ManuallyDrop;
1515
use std::ops::Deref;
1616
use std::ptr::{self, NonNull};
1717

18+
#[cfg(feature = "threading")]
19+
use once_cell::race::OnceBox;
20+
#[cfg(not(feature = "threading"))]
21+
type OnceBox<T> = once_cell::unsync::OnceCell<Box<T>>;
22+
1823
// so, PyObjectRef is basically equivalent to `PyRc<PyGenericObject<dyn PyObjectPayload>>`, except it's
1924
// only one pointer in width rather than 2. We do that by manually creating a vtable, and putting
2025
// a &'static reference to it inside the `PyRc` rather than adjacent to it, like trait objects do.
@@ -103,7 +108,7 @@ impl<T: fmt::Debug> fmt::Debug for PyInner<T> {
103108
}
104109

105110
struct WeakRefList {
106-
weak_lock: PyMutex<WeakListInner>,
111+
inner: OnceBox<PyMutex<WeakListInner>>,
107112
}
108113

109114
impl fmt::Debug for WeakRefList {
@@ -112,36 +117,45 @@ impl fmt::Debug for WeakRefList {
112117
}
113118
}
114119

115-
cfg_if::cfg_if! {
116-
if #[cfg(feature = "threading")] {
117-
unsafe impl Send for WeakRefList {}
118-
unsafe impl Sync for WeakRefList {}
119-
}
120-
}
121-
120+
#[derive(Default)]
122121
struct WeakListInner {
123122
list: LinkedList<WeakLink, PyObjectView<PyWeak>>,
124123
obj: Option<NonNull<PyObject>>,
125124
}
126125

126+
cfg_if::cfg_if! {
127+
if #[cfg(feature = "threading")] {
128+
unsafe impl Send for WeakListInner {}
129+
unsafe impl Sync for WeakListInner {}
130+
}
131+
}
132+
127133
impl WeakRefList {
128134
pub fn new() -> Self {
129135
WeakRefList {
130-
weak_lock: PyMutex::new(WeakListInner {
131-
list: LinkedList::new(),
132-
obj: None,
133-
}),
136+
inner: OnceBox::new(),
134137
}
135138
}
136139

140+
/// returns None if there have never been any weakrefs in this list
141+
fn try_lock(&self) -> Option<PyMutexGuard<'_, WeakListInner>> {
142+
self.inner.get().map(|mu| mu.lock())
143+
}
144+
fn lock(&self) -> PyMutexGuard<'_, WeakListInner> {
145+
self.inner
146+
.get()
147+
.expect("weakref should be initialized")
148+
.lock()
149+
}
150+
137151
fn add(
138152
&self,
139153
obj: &PyObject,
140154
cls: PyTypeRef,
141155
callback: Option<PyObjectRef>,
142156
dict: Option<PyDictRef>,
143157
) -> PyObjectWeak {
144-
let mut inner = self.weak_lock.lock();
158+
let mut inner = self.inner.get_or_init(Default::default).lock();
145159
if inner.obj.is_none() {
146160
inner.obj = Some(NonNull::from(obj));
147161
}
@@ -159,7 +173,10 @@ impl WeakRefList {
159173
}
160174

161175
fn clear(&self) {
162-
let mut inner = self.weak_lock.lock();
176+
let mut inner = match self.try_lock() {
177+
Some(inner) => inner,
178+
None => return,
179+
};
163180
inner.obj = None;
164181
// TODO: can be an arrayvec
165182
let mut v = Vec::with_capacity(16);
@@ -201,7 +218,7 @@ impl WeakRefList {
201218
}
202219

203220
fn count(&self) -> usize {
204-
self.weak_lock.lock().list.count()
221+
self.try_lock().map_or(0, |inner| inner.list.count())
205222
}
206223
}
207224

@@ -249,13 +266,7 @@ cfg_if::cfg_if! {
249266
impl PyWeak {
250267
pub(crate) fn upgrade(&self) -> Option<PyObjectRef> {
251268
// TODO: figure out orderings for here, in drop, and the store in WeakRefList::clear
252-
let guard = unsafe {
253-
self.parent
254-
.load(Ordering::SeqCst)
255-
.as_ref()?
256-
.weak_lock
257-
.lock()
258-
};
269+
let guard = unsafe { self.parent.load(Ordering::SeqCst).as_ref()?.lock() };
259270
let obj_ptr = guard.obj?;
260271
unsafe {
261272
obj_ptr.as_ref().0.refcount.incref();
@@ -267,7 +278,7 @@ impl PyWeak {
267278
fn drop_inner(&self) {
268279
let parent_ptr = self.parent.load(Ordering::SeqCst);
269280
let mut guard = match unsafe { parent_ptr.as_ref() } {
270-
Some(parent) => parent.weak_lock.lock(),
281+
Some(parent) => parent.lock(),
271282
// the object is dead - we don't have to worry about removing ourselves from the list
272283
None => return,
273284
};

0 commit comments

Comments
 (0)