Replace internal maps with unsorted Vecs

signal-hook-registry/src/lib.rs

@@ -65,9 +65,8 @@
 extern crate libc;
 
 mod half_lock;
+mod vec_map;
 
-use std::collections::hash_map::Entry;
-use std::collections::{BTreeMap, HashMap};
 use std::io::Error;
 use std::mem;
 use std::ptr;
@@ -89,6 +88,7 @@ use libc::{SIGFPE, SIGILL, SIGKILL, SIGSEGV, SIGSTOP};
 use libc::{SIGFPE, SIGILL, SIGSEGV};
 
 use half_lock::HalfLock;
+use vec_map::VecMap;
 
 // These constants are not defined in the current version of libc, but it actually
 // exists in Windows CRT.
@@ -140,9 +140,8 @@ type Action = Fn(&siginfo_t) + Send + Sync;
 #[derive(Clone)]
 struct Slot {
     prev: Prev,
-    // We use BTreeMap here, because we want to run the actions in the order they were inserted.
-    // This works, because the ActionIds are assigned in an increasing order.
-    actions: BTreeMap<ActionId, Arc<Action>>,
+    // Actions are stored and executed in the order they were registered.
+    actions: VecMap<ActionId, Arc<Action>>,
 }
 
 impl Slot {
@@ -154,7 +153,7 @@ impl Slot {
         }
         Ok(Slot {
             prev: Prev { signal, info: old },
-            actions: BTreeMap::new(),
+            actions: VecMap::new(),
         })
     }
 
@@ -200,14 +199,14 @@ impl Slot {
         }
         Ok(Slot {
             prev: Prev { signal, info: old },
-            actions: BTreeMap::new(),
+            actions: VecMap::new(),
         })
     }
 }
 
 #[derive(Clone)]
 struct SignalData {
-    signals: HashMap<c_int, Slot>,
+    signals: VecMap<c_int, Slot>,
     next_id: u128,
 }
 
@@ -324,7 +323,7 @@ impl GlobalData {
         GLOBAL_INIT.call_once(|| {
             let data = Box::into_raw(Box::new(GlobalData {
                 data: HalfLock::new(SignalData {
-                    signals: HashMap::new(),
+                    signals: VecMap::new(),
                     next_id: 1,
                 }),
                 race_fallback: HalfLock::new(None),
@@ -631,34 +630,32 @@ unsafe fn register_unchecked_impl(signal: c_int, action: Arc<Action>) -> Result<
     let id = ActionId(sigdata.next_id);
     sigdata.next_id += 1;
 
-    match sigdata.signals.entry(signal) {
-        Entry::Occupied(mut occupied) => {
-            assert!(occupied.get_mut().actions.insert(id, action).is_none());
-        }
-        Entry::Vacant(place) => {
-            // While the sigaction/signal exchanges the old one atomically, we are not able to
-            // atomically store it somewhere a signal handler could read it. That poses a race
-            // condition where we could lose some signals delivered in between changing it and
-            // storing it.
-            //
-            // Therefore we first store the old one in the fallback storage. The fallback only
-            // covers the cases where the slot is not yet active and becomes "inert" after that,
-            // even if not removed (it may get overwritten by some other signal, but for that the
-            // mutex in globals.data must be unlocked here - and by that time we already stored the
-            // slot.
-            //
-            // And yes, this still leaves a short race condition when some other thread could
-            // replace the signal handler and we would be calling the outdated one for a short
-            // time, until we install the slot.
-            globals
-                .race_fallback
-                .write()
-                .store(Some(Prev::detect(signal)?));
-
-            let mut slot = Slot::new(signal)?;
-            slot.actions.insert(id, action);
-            place.insert(slot);
-        }
+    if sigdata.signals.contains(&signal) {
+        let slot = sigdata.signals.get_mut(&signal).unwrap();
+        assert!(slot.actions.insert(id, action).is_none());
+    } else {
+        // While the sigaction/signal exchanges the old one atomically, we are not able to
+        // atomically store it somewhere a signal handler could read it. That poses a race
+        // condition where we could lose some signals delivered in between changing it and
+        // storing it.
+        //
+        // Therefore we first store the old one in the fallback storage. The fallback only
+        // covers the cases where the slot is not yet active and becomes "inert" after that,
+        // even if not removed (it may get overwritten by some other signal, but for that the
+        // mutex in globals.data must be unlocked here - and by that time we already stored the
+        // slot.
+        //
+        // And yes, this still leaves a short race condition when some other thread could
+        // replace the signal handler and we would be calling the outdated one for a short
+        // time, until we install the slot.
+        globals
+            .race_fallback
+            .write()
+            .store(Some(Prev::detect(signal)?));
+
+        let mut slot = Slot::new(signal)?;
+        slot.actions.insert(id, action);
+        sigdata.signals.insert(signal, slot);
     }
 
     lock.store(sigdata);

signal-hook-registry/src/vec_map.rs

@@ -0,0 +1,171 @@
+use std::mem;
+
+/// A small map backed by an unsorted vector.
+///
+/// Maintains key uniqueness at cost of O(n) lookup/insert/remove. Maintains insertion order
+/// (`insert` calls that overwrite an existing value don't change order).
+#[derive(Clone, Default)]
+pub struct VecMap<K, V>(Vec<(K, V)>);
+
+impl<K: Eq, V> VecMap<K, V> {
+    pub fn new() -> Self {
+        VecMap(Vec::new())
+    }
+
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+
+    pub fn clear(&mut self) {
+        self.0.clear();
+    }
+
+    fn find(&self, key: &K) -> Option<usize> {
+        for (i, (k, _)) in self.0.iter().enumerate() {
+            if k == key {
+                return Some(i);
+            }
+        }
+        None
+    }
+
+    pub fn contains(&self, key: &K) -> bool {
+        self.find(key).is_some()
+    }
+
+    pub fn get(&self, key: &K) -> Option<&V> {
+        match self.find(key) {
+            Some(i) => Some(&self.0[i].1),
+            None => None,
+        }
+    }
+
+    pub fn get_mut(&mut self, key: &K) -> Option<&mut V> {
+        match self.find(key) {
+            Some(i) => Some(&mut self.0[i].1),
+            None => None,
+        }
+    }
+
+    pub fn insert(&mut self, key: K, value: V) -> Option<V> {
+        if let Some(old) = self.get_mut(&key) {
+            return Some(mem::replace(old, value));
+        }
+        self.0.push((key, value));
+        None
+    }
+
+    pub fn remove(&mut self, key: &K) -> Option<V> {
+        match self.find(key) {
+            Some(i) => Some(self.0.remove(i).1),
+            None => None,
+        }
+    }
+
+    pub fn values(&self) -> impl Iterator<Item = &V> {
+        self.0.iter().map(|kv| &kv.1)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn empty() {
+        let m: VecMap<char, u32> = VecMap::new();
+        assert!(m.is_empty());
+        assert!(!m.contains(&'a'));
+        assert!(m.values().next().is_none());
+    }
+
+    #[test]
+    fn insert_update_get() {
+        let mut m: VecMap<char, u32> = VecMap::new();
+        assert!(m.insert('a', 1).is_none());
+        assert!(m.insert('b', 2).is_none());
+        assert_eq!(m.get(&'a'), Some(&1));
+        assert_eq!(m.get(&'b'), Some(&2));
+        *m.get_mut(&'a').unwrap() += 10;
+        assert_eq!(m.get(&'a'), Some(&11));
+        assert_eq!(m.get(&'b'), Some(&2));
+    }
+
+    #[test]
+    fn insert_overwrite() {
+        let mut m = VecMap::new();
+        assert_eq!(m.insert('a', 1), None);
+        assert_eq!(m.insert('b', 2), None);
+        assert_eq!(m.insert('a', 3), Some(1));
+        assert_eq!(m.insert('a', 4), Some(3));
+        assert_eq!(m.get(&'a').copied(), Some(4));
+        assert_eq!(m.get(&'b').copied(), Some(2));
+        assert_eq!(m.insert('b', 5), Some(2));
+        assert_eq!(m.get(&'a').copied(), Some(4));
+        assert_eq!(m.get(&'b').copied(), Some(5));
+    }
+
+    #[test]
+    fn insert_remove() {
+        let mut m: VecMap<char, u32> = VecMap::new();
+        assert_eq!(m.remove(&'a'), None);
+        m.insert('a', 1);
+        m.insert('b', 2);
+        assert_eq!(m.remove(&'a'), Some(1));
+        assert_eq!(m.remove(&'a'), None);
+        assert_eq!(m.remove(&'b'), Some(2));
+        assert!(m.is_empty());
+    }
+
+    #[test]
+    fn insertion_order() {
+        let mut m: VecMap<char, u32> = VecMap::new();
+        let values = |m: &VecMap<char, u32>| -> Vec<u32> { m.values().copied().collect() };
+        m.insert('b', 2);
+        m.insert('a', 1);
+        m.insert('c', 3);
+        assert_eq!(values(&m), vec![2, 1, 3]);
+        m.insert('a', 11);
+        m.remove(&'b');
+        assert_eq!(values(&m), vec![11, 3]);
+        m.insert('b', 2);
+        assert_eq!(values(&m), vec![11, 3, 2]);
+    }
+
+    #[test]
+    fn containment_equivalences() {
+        let mut m = VecMap::new();
+        for i in 0u8..=255 {
+            if i % 10 < 3 {
+                m.insert(i, i);
+            }
+        }
+        for i in 0u8..=255 {
+            if m.contains(&i) {
+                assert_eq!(m.get(&i).copied(), Some(i));
+                assert_eq!(m.get_mut(&i).copied(), Some(i));
+                assert_eq!(m.insert(i, i), Some(i));
+                assert_eq!(m.remove(&i), Some(i));
+            } else {
+                assert!(m.get(&i).is_none());
+                assert!(m.get_mut(&i).is_none());
+                assert!(m.remove(&i).is_none());
+                assert!(m.insert(i, i).is_none());
+            }
+        }
+    }
+
+    #[test]
+    fn clear() {
+        let mut m = VecMap::new();
+        m.clear();
+        assert!(m.is_empty());
+        m.insert('a', 1);
+        m.insert('b', 2);
+        assert!(!m.is_empty());
+        m.clear();
+        assert!(m.is_empty());
+        m.insert('c', 3);
+        assert!(!m.is_empty());
+    }
+}