Skip to content

Commit 017e9b5

Browse files
authored
Merge pull request #192 from hanna-kruppe/dumb-down-data-structures
Replace internal maps with unsorted Vecs
2 parents 990feef + 0cae452 commit 017e9b5

File tree

2 files changed

+205
-37
lines changed

2 files changed

+205
-37
lines changed

signal-hook-registry/src/lib.rs

Lines changed: 34 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -65,9 +65,8 @@
6565
extern crate libc;
6666

6767
mod half_lock;
68+
mod vec_map;
6869

69-
use std::collections::hash_map::Entry;
70-
use std::collections::{BTreeMap, HashMap};
7170
use std::io::Error;
7271
use std::mem;
7372
use std::ptr;
@@ -89,6 +88,7 @@ use libc::{SIGFPE, SIGILL, SIGKILL, SIGSEGV, SIGSTOP};
8988
use libc::{SIGFPE, SIGILL, SIGSEGV};
9089

9190
use half_lock::HalfLock;
91+
use vec_map::VecMap;
9292

9393
// These constants are not defined in the current version of libc, but it actually
9494
// exists in Windows CRT.
@@ -140,9 +140,8 @@ type Action = Fn(&siginfo_t) + Send + Sync;
140140
#[derive(Clone)]
141141
struct Slot {
142142
prev: Prev,
143-
// We use BTreeMap here, because we want to run the actions in the order they were inserted.
144-
// This works, because the ActionIds are assigned in an increasing order.
145-
actions: BTreeMap<ActionId, Arc<Action>>,
143+
// Actions are stored and executed in the order they were registered.
144+
actions: VecMap<ActionId, Arc<Action>>,
146145
}
147146

148147
impl Slot {
@@ -154,7 +153,7 @@ impl Slot {
154153
}
155154
Ok(Slot {
156155
prev: Prev { signal, info: old },
157-
actions: BTreeMap::new(),
156+
actions: VecMap::new(),
158157
})
159158
}
160159

@@ -200,14 +199,14 @@ impl Slot {
200199
}
201200
Ok(Slot {
202201
prev: Prev { signal, info: old },
203-
actions: BTreeMap::new(),
202+
actions: VecMap::new(),
204203
})
205204
}
206205
}
207206

208207
#[derive(Clone)]
209208
struct SignalData {
210-
signals: HashMap<c_int, Slot>,
209+
signals: VecMap<c_int, Slot>,
211210
next_id: u128,
212211
}
213212

@@ -324,7 +323,7 @@ impl GlobalData {
324323
GLOBAL_INIT.call_once(|| {
325324
let data = Box::into_raw(Box::new(GlobalData {
326325
data: HalfLock::new(SignalData {
327-
signals: HashMap::new(),
326+
signals: VecMap::new(),
328327
next_id: 1,
329328
}),
330329
race_fallback: HalfLock::new(None),
@@ -631,34 +630,32 @@ unsafe fn register_unchecked_impl(signal: c_int, action: Arc<Action>) -> Result<
631630
let id = ActionId(sigdata.next_id);
632631
sigdata.next_id += 1;
633632

634-
match sigdata.signals.entry(signal) {
635-
Entry::Occupied(mut occupied) => {
636-
assert!(occupied.get_mut().actions.insert(id, action).is_none());
637-
}
638-
Entry::Vacant(place) => {
639-
// While the sigaction/signal exchanges the old one atomically, we are not able to
640-
// atomically store it somewhere a signal handler could read it. That poses a race
641-
// condition where we could lose some signals delivered in between changing it and
642-
// storing it.
643-
//
644-
// Therefore we first store the old one in the fallback storage. The fallback only
645-
// covers the cases where the slot is not yet active and becomes "inert" after that,
646-
// even if not removed (it may get overwritten by some other signal, but for that the
647-
// mutex in globals.data must be unlocked here - and by that time we already stored the
648-
// slot.
649-
//
650-
// And yes, this still leaves a short race condition when some other thread could
651-
// replace the signal handler and we would be calling the outdated one for a short
652-
// time, until we install the slot.
653-
globals
654-
.race_fallback
655-
.write()
656-
.store(Some(Prev::detect(signal)?));
657-
658-
let mut slot = Slot::new(signal)?;
659-
slot.actions.insert(id, action);
660-
place.insert(slot);
661-
}
633+
if sigdata.signals.contains(&signal) {
634+
let slot = sigdata.signals.get_mut(&signal).unwrap();
635+
assert!(slot.actions.insert(id, action).is_none());
636+
} else {
637+
// While the sigaction/signal exchanges the old one atomically, we are not able to
638+
// atomically store it somewhere a signal handler could read it. That poses a race
639+
// condition where we could lose some signals delivered in between changing it and
640+
// storing it.
641+
//
642+
// Therefore we first store the old one in the fallback storage. The fallback only
643+
// covers the cases where the slot is not yet active and becomes "inert" after that,
644+
// even if not removed (it may get overwritten by some other signal, but for that the
645+
// mutex in globals.data must be unlocked here - and by that time we already stored the
646+
// slot.
647+
//
648+
// And yes, this still leaves a short race condition when some other thread could
649+
// replace the signal handler and we would be calling the outdated one for a short
650+
// time, until we install the slot.
651+
globals
652+
.race_fallback
653+
.write()
654+
.store(Some(Prev::detect(signal)?));
655+
656+
let mut slot = Slot::new(signal)?;
657+
slot.actions.insert(id, action);
658+
sigdata.signals.insert(signal, slot);
662659
}
663660

664661
lock.store(sigdata);
Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
use std::mem;
2+
3+
/// A small map backed by an unsorted vector.
4+
///
5+
/// Maintains key uniqueness at cost of O(n) lookup/insert/remove. Maintains insertion order
6+
/// (`insert` calls that overwrite an existing value don't change order).
7+
#[derive(Clone, Default)]
8+
pub struct VecMap<K, V>(Vec<(K, V)>);
9+
10+
impl<K: Eq, V> VecMap<K, V> {
11+
pub fn new() -> Self {
12+
VecMap(Vec::new())
13+
}
14+
15+
pub fn is_empty(&self) -> bool {
16+
self.0.is_empty()
17+
}
18+
19+
pub fn clear(&mut self) {
20+
self.0.clear();
21+
}
22+
23+
fn find(&self, key: &K) -> Option<usize> {
24+
for (i, (k, _)) in self.0.iter().enumerate() {
25+
if k == key {
26+
return Some(i);
27+
}
28+
}
29+
None
30+
}
31+
32+
pub fn contains(&self, key: &K) -> bool {
33+
self.find(key).is_some()
34+
}
35+
36+
pub fn get(&self, key: &K) -> Option<&V> {
37+
match self.find(key) {
38+
Some(i) => Some(&self.0[i].1),
39+
None => None,
40+
}
41+
}
42+
43+
pub fn get_mut(&mut self, key: &K) -> Option<&mut V> {
44+
match self.find(key) {
45+
Some(i) => Some(&mut self.0[i].1),
46+
None => None,
47+
}
48+
}
49+
50+
pub fn insert(&mut self, key: K, value: V) -> Option<V> {
51+
if let Some(old) = self.get_mut(&key) {
52+
return Some(mem::replace(old, value));
53+
}
54+
self.0.push((key, value));
55+
None
56+
}
57+
58+
pub fn remove(&mut self, key: &K) -> Option<V> {
59+
match self.find(key) {
60+
Some(i) => Some(self.0.remove(i).1),
61+
None => None,
62+
}
63+
}
64+
65+
pub fn values(&self) -> impl Iterator<Item = &V> {
66+
self.0.iter().map(|kv| &kv.1)
67+
}
68+
}
69+
70+
#[cfg(test)]
71+
mod tests {
72+
use super::*;
73+
74+
#[test]
75+
fn empty() {
76+
let m: VecMap<char, u32> = VecMap::new();
77+
assert!(m.is_empty());
78+
assert!(!m.contains(&'a'));
79+
assert!(m.values().next().is_none());
80+
}
81+
82+
#[test]
83+
fn insert_update_get() {
84+
let mut m: VecMap<char, u32> = VecMap::new();
85+
assert!(m.insert('a', 1).is_none());
86+
assert!(m.insert('b', 2).is_none());
87+
assert_eq!(m.get(&'a'), Some(&1));
88+
assert_eq!(m.get(&'b'), Some(&2));
89+
*m.get_mut(&'a').unwrap() += 10;
90+
assert_eq!(m.get(&'a'), Some(&11));
91+
assert_eq!(m.get(&'b'), Some(&2));
92+
}
93+
94+
#[test]
95+
fn insert_overwrite() {
96+
let mut m = VecMap::new();
97+
assert_eq!(m.insert('a', 1), None);
98+
assert_eq!(m.insert('b', 2), None);
99+
assert_eq!(m.insert('a', 3), Some(1));
100+
assert_eq!(m.insert('a', 4), Some(3));
101+
assert_eq!(m.get(&'a').copied(), Some(4));
102+
assert_eq!(m.get(&'b').copied(), Some(2));
103+
assert_eq!(m.insert('b', 5), Some(2));
104+
assert_eq!(m.get(&'a').copied(), Some(4));
105+
assert_eq!(m.get(&'b').copied(), Some(5));
106+
}
107+
108+
#[test]
109+
fn insert_remove() {
110+
let mut m: VecMap<char, u32> = VecMap::new();
111+
assert_eq!(m.remove(&'a'), None);
112+
m.insert('a', 1);
113+
m.insert('b', 2);
114+
assert_eq!(m.remove(&'a'), Some(1));
115+
assert_eq!(m.remove(&'a'), None);
116+
assert_eq!(m.remove(&'b'), Some(2));
117+
assert!(m.is_empty());
118+
}
119+
120+
#[test]
121+
fn insertion_order() {
122+
let mut m: VecMap<char, u32> = VecMap::new();
123+
let values = |m: &VecMap<char, u32>| -> Vec<u32> { m.values().copied().collect() };
124+
m.insert('b', 2);
125+
m.insert('a', 1);
126+
m.insert('c', 3);
127+
assert_eq!(values(&m), vec![2, 1, 3]);
128+
m.insert('a', 11);
129+
m.remove(&'b');
130+
assert_eq!(values(&m), vec![11, 3]);
131+
m.insert('b', 2);
132+
assert_eq!(values(&m), vec![11, 3, 2]);
133+
}
134+
135+
#[test]
136+
fn containment_equivalences() {
137+
let mut m = VecMap::new();
138+
for i in 0u8..=255 {
139+
if i % 10 < 3 {
140+
m.insert(i, i);
141+
}
142+
}
143+
for i in 0u8..=255 {
144+
if m.contains(&i) {
145+
assert_eq!(m.get(&i).copied(), Some(i));
146+
assert_eq!(m.get_mut(&i).copied(), Some(i));
147+
assert_eq!(m.insert(i, i), Some(i));
148+
assert_eq!(m.remove(&i), Some(i));
149+
} else {
150+
assert!(m.get(&i).is_none());
151+
assert!(m.get_mut(&i).is_none());
152+
assert!(m.remove(&i).is_none());
153+
assert!(m.insert(i, i).is_none());
154+
}
155+
}
156+
}
157+
158+
#[test]
159+
fn clear() {
160+
let mut m = VecMap::new();
161+
m.clear();
162+
assert!(m.is_empty());
163+
m.insert('a', 1);
164+
m.insert('b', 2);
165+
assert!(!m.is_empty());
166+
m.clear();
167+
assert!(m.is_empty());
168+
m.insert('c', 3);
169+
assert!(!m.is_empty());
170+
}
171+
}

0 commit comments

Comments
 (0)