Make dict repr more efficient

common/src/cell.rs

@@ -119,7 +119,31 @@ cfg_if::cfg_if! {
         type RwLockInner<T> = RefCell<T>;
         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<T>, RefMut<'a, T>);
+        impl<'a, T: ?Sized> RwLockWriteInner<'a, T> {
+            fn map<U: ?Sized, F>(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<T>(value: T) -> RwLockInner<T> {
             RefCell::new(value)
@@ -128,7 +152,7 @@ cfg_if::cfg_if! {
             m.borrow()
         }
         fn write_rwlock<T: ?Sized>(m: &RwLockInner<T>) -> RwLockWriteInner<T> {
-            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]

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<T = PyObjectRef> {
     inner: PyRwLock<DictInner<T>>,
 }
 
-struct DictInner<T> {
-    size: usize,
-    indices: HashMap<HashIndex, EntryIndex>,
-    entries: Vec<Option<DictEntry<T>>>,
+#[derive(Debug, Copy, Clone)]
+enum IndexEntry {
+    Dummy,
+    Free,
+    Index(usize),
 }
-
-impl<T: Clone> Clone for DictInner<T> {
-    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<i64> 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<IndexEntry> 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<T> {
+    used: usize,
+    filled: usize,
+    indices: Vec<i64>,
+    entries: Vec<DictEntry<T>>,
+}
 
 impl<T: Clone> Clone for Dict<T> {
     fn clone(&self) -> Self {
@@ -52,34 +74,110 @@ impl<T> Default for Dict<T> {
     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<T> {
     hash: HashValue,
     key: PyObjectRef,
+    index: IndexIndex,
     value: T,
 }
 
-impl<T: Clone> Clone for DictEntry<T> {
-    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<T> DictInner<T> {
+    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<T: Clone> Dict<T> {
@@ -91,116 +189,82 @@ impl<T: Clone> Dict<T> {
         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<K>(&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<K: DictKey>(&self, vm: &VirtualMachine, key: &K) -> PyResult<bool> {
-        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<K: DictKey>(&self, vm: &VirtualMachine, key: &K) -> PyResult<Option<T>> {
-        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<T: Clone> Dict<T> {
     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<T: Clone> Dict<T> {
         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<T: Clone> Dict<T> {
     }
 
     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<PyObjectRef> {
         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<K: DictKey>(&self, vm: &VirtualMachine, key: &K) -> PyResult<LookupResult> {
-        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<K: DictKey>(
+        &self,
+        vm: &VirtualMachine,
+        key: &K,
+        hash_value: HashValue,
+        mut lock: Option<PyRwLockReadGuard<DictInner<T>>>,
+    ) -> PyResult<LookupResult> {
+        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<Option<DictEntry<T>>, ()> {
+        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<K: DictKey>(&self, vm: &VirtualMachine, key: &K) -> PyResult<Option<T>> {
+        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>() + self.borrow_value().size * size_of::<DictEntry<T>>()
+        let inner = self.borrow_value();
+        size_of::<Self>()
+            + size_of::<DictInner<T>>()
+            + inner.indices.len() * size_of::<i64>()
+            + inner.entries.len() * size_of::<DictEntry<T>>()
     }
 }
 
-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:

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

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