Skip to content

Commit d1d2184

Browse files
VFLashMVFLashM
committed
SsoHashSet/Map - genericiy over Q removed
Due to performance regression, see SsoHashMap comment.
1 parent 92a0668 commit d1d2184

File tree

2 files changed

+72
-89
lines changed

2 files changed

+72
-89
lines changed

‎compiler/rustc_data_structures/src/sso/map.rs‎

Lines changed: 54 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
use super::either_iter::EitherIter;
22
use crate::fx::FxHashMap;
33
use arrayvec::ArrayVec;
4-
use std::borrow::Borrow;
54
use std::fmt;
65
use std::hash::Hash;
76
use std::iter::FromIterator;
@@ -30,19 +29,45 @@ const SSO_ARRAY_SIZE: usize = 8;
3029
///
3130
/// Stores elements in a small array up to a certain length
3231
/// and switches to `HashMap` when that length is exceeded.
33-
///
34-
/// Implements subset of HashMap API.
35-
///
36-
/// Missing HashMap API:
37-
/// all hasher-related
38-
/// try_reserve (unstable)
39-
/// shrink_to (unstable)
40-
/// drain_filter (unstable)
41-
/// into_keys/into_values (unstable)
42-
/// all raw_entry-related
43-
/// PartialEq/Eq (requires sorting the array)
44-
/// Entry::or_insert_with_key (unstable)
45-
/// Vacant/Occupied entries and related
32+
//
33+
// FIXME: Implements subset of HashMap API.
34+
//
35+
// Missing HashMap API:
36+
// all hasher-related
37+
// try_reserve (unstable)
38+
// shrink_to (unstable)
39+
// drain_filter (unstable)
40+
// into_keys/into_values (unstable)
41+
// all raw_entry-related
42+
// PartialEq/Eq (requires sorting the array)
43+
// Entry::or_insert_with_key (unstable)
44+
// Vacant/Occupied entries and related
45+
//
46+
// FIXME: In HashMap most methods accepting key reference
47+
// accept reference to generic `Q` where `K: Borrow<Q>`.
48+
//
49+
// However, using this approach in `HashMap::get` apparently
50+
// breaks inlining and noticeably reduces performance.
51+
//
52+
// Performance *should* be the same given that borrow is
53+
// a NOP in most cases, but in practice that's not the case.
54+
//
55+
// Further investigation is required.
56+
//
57+
// Affected methods:
58+
// SsoHashMap::get
59+
// SsoHashMap::get_mut
60+
// SsoHashMap::get_entry
61+
// SsoHashMap::get_key_value
62+
// SsoHashMap::contains_key
63+
// SsoHashMap::remove
64+
// SsoHashMap::remove_entry
65+
// Index::index
66+
// SsoHashSet::take
67+
// SsoHashSet::get
68+
// SsoHashSet::remove
69+
// SsoHashSet::contains
70+
4671
#[derive(Clone)]
4772
pub enum SsoHashMap<K, V> {
4873
Array(ArrayVec<[(K, V); SSO_ARRAY_SIZE]>),
@@ -232,14 +257,10 @@ impl<K: Eq + Hash, V> SsoHashMap<K, V> {
232257

233258
/// Removes a key from the map, returning the value at the key if the key
234259
/// was previously in the map.
235-
pub fn remove<Q: ?Sized>(&mut self, key: &Q) -> Option<V>
236-
where
237-
K: Borrow<Q>,
238-
Q: Hash + Eq,
239-
{
260+
pub fn remove(&mut self, key: &K) -> Option<V> {
240261
match self {
241262
SsoHashMap::Array(array) => {
242-
if let Some(index) = array.iter().position(|(k, _v)| k.borrow() == key) {
263+
if let Some(index) = array.iter().position(|(k, _v)| k == key) {
243264
Some(array.swap_remove(index).1)
244265
} else {
245266
None
@@ -251,14 +272,10 @@ impl<K: Eq + Hash, V> SsoHashMap<K, V> {
251272

252273
/// Removes a key from the map, returning the stored key and value if the
253274
/// key was previously in the map.
254-
pub fn remove_entry<Q: ?Sized>(&mut self, key: &Q) -> Option<(K, V)>
255-
where
256-
K: Borrow<Q>,
257-
Q: Hash + Eq,
258-
{
275+
pub fn remove_entry(&mut self, key: &K) -> Option<(K, V)> {
259276
match self {
260277
SsoHashMap::Array(array) => {
261-
if let Some(index) = array.iter().position(|(k, _v)| k.borrow() == key) {
278+
if let Some(index) = array.iter().position(|(k, _v)| k == key) {
262279
Some(array.swap_remove(index))
263280
} else {
264281
None
@@ -269,15 +286,11 @@ impl<K: Eq + Hash, V> SsoHashMap<K, V> {
269286
}
270287

271288
/// Returns a reference to the value corresponding to the key.
272-
pub fn get<Q: ?Sized>(&self, key: &Q) -> Option<&V>
273-
where
274-
K: Borrow<Q>,
275-
Q: Hash + Eq,
276-
{
289+
pub fn get(&self, key: &K) -> Option<&V> {
277290
match self {
278291
SsoHashMap::Array(array) => {
279292
for (k, v) in array {
280-
if k.borrow() == key {
293+
if k == key {
281294
return Some(v);
282295
}
283296
}
@@ -288,15 +301,11 @@ impl<K: Eq + Hash, V> SsoHashMap<K, V> {
288301
}
289302

290303
/// Returns a mutable reference to the value corresponding to the key.
291-
pub fn get_mut<Q: ?Sized>(&mut self, key: &Q) -> Option<&mut V>
292-
where
293-
K: Borrow<Q>,
294-
Q: Hash + Eq,
295-
{
304+
pub fn get_mut(&mut self, key: &K) -> Option<&mut V> {
296305
match self {
297306
SsoHashMap::Array(array) => {
298307
for (k, v) in array {
299-
if (*k).borrow() == key {
308+
if k == key {
300309
return Some(v);
301310
}
302311
}
@@ -307,15 +316,11 @@ impl<K: Eq + Hash, V> SsoHashMap<K, V> {
307316
}
308317

309318
/// Returns the key-value pair corresponding to the supplied key.
310-
pub fn get_key_value<Q: ?Sized>(&self, key: &Q) -> Option<(&K, &V)>
311-
where
312-
K: Borrow<Q>,
313-
Q: Hash + Eq,
314-
{
319+
pub fn get_key_value(&self, key: &K) -> Option<(&K, &V)> {
315320
match self {
316321
SsoHashMap::Array(array) => {
317322
for (k, v) in array {
318-
if k.borrow() == key {
323+
if k == key {
319324
return Some((k, v));
320325
}
321326
}
@@ -326,13 +331,9 @@ impl<K: Eq + Hash, V> SsoHashMap<K, V> {
326331
}
327332

328333
/// Returns `true` if the map contains a value for the specified key.
329-
pub fn contains_key<Q: ?Sized>(&self, key: &Q) -> bool
330-
where
331-
K: Borrow<Q>,
332-
Q: Hash + Eq,
333-
{
334+
pub fn contains_key(&self, key: &K) -> bool {
334335
match self {
335-
SsoHashMap::Array(array) => array.iter().any(|(k, _v)| k.borrow() == key),
336+
SsoHashMap::Array(array) => array.iter().any(|(k, _v)| k == key),
336337
SsoHashMap::Map(map) => map.contains_key(key),
337338
}
338339
}
@@ -483,15 +484,14 @@ where
483484
}
484485
}
485486

486-
impl<'a, K, Q: ?Sized, V> Index<&'a Q> for SsoHashMap<K, V>
487+
impl<'a, K, V> Index<&'a K> for SsoHashMap<K, V>
487488
where
488-
K: Eq + Hash + Borrow<Q>,
489-
Q: Eq + Hash,
489+
K: Eq + Hash,
490490
{
491491
type Output = V;
492492

493493
#[inline]
494-
fn index(&self, key: &Q) -> &V {
494+
fn index(&self, key: &K) -> &V {
495495
self.get(key).expect("no entry found for key")
496496
}
497497
}

‎compiler/rustc_data_structures/src/sso/set.rs‎

Lines changed: 18 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
use std::borrow::Borrow;
21
use std::fmt;
32
use std::hash::Hash;
43
use std::iter::FromIterator;
@@ -9,20 +8,20 @@ use super::map::SsoHashMap;
98
///
109
/// Stores elements in a small array up to a certain length
1110
/// and switches to `HashSet` when that length is exceeded.
12-
///
13-
/// Implements subset of HashSet API.
14-
///
15-
/// Missing HashSet API:
16-
/// all hasher-related
17-
/// try_reserve (unstable)
18-
/// shrink_to (unstable)
19-
/// drain_filter (unstable)
20-
/// replace
21-
/// get_or_insert/get_or_insert_owned/get_or_insert_with (unstable)
22-
/// difference/symmetric_difference/intersection/union
23-
/// is_disjoint/is_subset/is_superset
24-
/// PartialEq/Eq (requires SsoHashMap implementation)
25-
/// BitOr/BitAnd/BitXor/Sub
11+
//
12+
// FIXME: Implements subset of HashSet API.
13+
//
14+
// Missing HashSet API:
15+
// all hasher-related
16+
// try_reserve (unstable)
17+
// shrink_to (unstable)
18+
// drain_filter (unstable)
19+
// replace
20+
// get_or_insert/get_or_insert_owned/get_or_insert_with (unstable)
21+
// difference/symmetric_difference/intersection/union
22+
// is_disjoint/is_subset/is_superset
23+
// PartialEq/Eq (requires SsoHashMap implementation)
24+
// BitOr/BitAnd/BitXor/Sub
2625
#[derive(Clone)]
2726
pub struct SsoHashSet<T> {
2827
map: SsoHashMap<T, ()>,
@@ -115,21 +114,13 @@ impl<T: Eq + Hash> SsoHashSet<T> {
115114

116115
/// Removes and returns the value in the set, if any, that is equal to the given one.
117116
#[inline]
118-
pub fn take<Q: ?Sized>(&mut self, value: &Q) -> Option<T>
119-
where
120-
T: Borrow<Q>,
121-
Q: Hash + Eq,
122-
{
117+
pub fn take(&mut self, value: &T) -> Option<T> {
123118
self.map.remove_entry(value).map(entry_to_key)
124119
}
125120

126121
/// Returns a reference to the value in the set, if any, that is equal to the given value.
127122
#[inline]
128-
pub fn get<Q: ?Sized>(&self, value: &Q) -> Option<&T>
129-
where
130-
T: Borrow<Q>,
131-
Q: Hash + Eq,
132-
{
123+
pub fn get(&self, value: &T) -> Option<&T> {
133124
self.map.get_key_value(value).map(entry_to_key)
134125
}
135126

@@ -146,21 +137,13 @@ impl<T: Eq + Hash> SsoHashSet<T> {
146137
/// Removes a value from the set. Returns whether the value was
147138
/// present in the set.
148139
#[inline]
149-
pub fn remove<Q: ?Sized>(&mut self, value: &Q) -> bool
150-
where
151-
T: Borrow<Q>,
152-
Q: Hash + Eq,
153-
{
140+
pub fn remove(&mut self, value: &T) -> bool {
154141
self.map.remove(value).is_some()
155142
}
156143

157144
/// Returns `true` if the set contains a value.
158145
#[inline]
159-
pub fn contains<Q: ?Sized>(&self, value: &Q) -> bool
160-
where
161-
T: Borrow<Q>,
162-
Q: Hash + Eq,
163-
{
146+
pub fn contains(&self, value: &T) -> bool {
164147
self.map.contains_key(value)
165148
}
166149
}

0 commit comments

Comments
 (0)