From e47bec1556af7c1708878fef01c7365d0f9ab306 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 7 Aug 2020 13:34:27 +1000 Subject: [PATCH] Avoid closures to improve compile times. `HashMap` and `HashSet` are used widely, and often instantiated many times. As a result, small differences in how the code is written can have a significant effect on how much LLVM IR is generated, which affects compile times. This commit avoids a lot of small closures by replacing calls to `Option::map`, `Option::ok_or_else`, `Option::unwrap_or_else` and `Result::unwrap_or_else` with `match` expressions. Although this makes the code less concise, it improves compile times. For example, several of the benchmarks in rustc-perf compile up to 3.5% faster after this change is incorporated into std. Every change is accompanied by a short comment to explain why a `match` is used. This may seem excessive, but without these comments it would be easy for a well-meaning person in the future to change some or all of these back to the original versions without understanding the consequences. --- src/map.rs | 98 +++++++++++++++++++++++++++++++++----------------- src/raw/mod.rs | 77 ++++++++++++++++++++++++++------------- src/set.rs | 24 ++++++++++--- 3 files changed, 139 insertions(+), 60 deletions(-) diff --git a/src/map.rs b/src/map.rs index a1fa6e88bc..5ac8528c72 100644 --- a/src/map.rs +++ b/src/map.rs @@ -802,7 +802,11 @@ where K: Borrow, Q: Hash + Eq, { - self.get_key_value(k).map(|(_, v)| v) + // Avoid `Option::map` because it bloats LLVM IR. + match self.get_key_value(k) { + Some((_, v)) => Some(v), + None => None, + } } /// Returns the key-value pair corresponding to the supplied key. @@ -831,12 +835,14 @@ where Q: Hash + Eq, { let hash = make_hash(&self.hash_builder, k); - self.table - .find(hash, |x| k.eq(x.0.borrow())) - .map(|item| unsafe { + // Avoid `Option::map` because it bloats LLVM IR. + match self.table.find(hash, |x| k.eq(x.0.borrow())) { + Some(item) => unsafe { let &(ref key, ref value) = item.as_ref(); - (key, value) - }) + Some((key, value)) + } + None => None, + } } /// Returns the key-value pair corresponding to the supplied key, with a mutable reference to value. @@ -869,12 +875,14 @@ where Q: Hash + Eq, { let hash = make_hash(&self.hash_builder, k); - self.table - .find(hash, |x| k.eq(x.0.borrow())) - .map(|item| unsafe { + // Avoid `Option::map` because it bloats LLVM IR. + match self.table.find(hash, |x| k.eq(x.0.borrow())) { + Some(item) => unsafe { let &mut (ref key, ref mut value) = item.as_mut(); - (key, value) - }) + Some((key, value)) + } + None => None, + } } /// Returns `true` if the map contains a value for the specified key. @@ -933,9 +941,11 @@ where Q: Hash + Eq, { let hash = make_hash(&self.hash_builder, k); - self.table - .find(hash, |x| k.eq(x.0.borrow())) - .map(|item| unsafe { &mut item.as_mut().1 }) + // Avoid `Option::map` because it bloats LLVM IR. + match self.table.find(hash, |x| k.eq(x.0.borrow())) { + Some(item) => Some(unsafe { &mut item.as_mut().1 }), + None => None, + } } /// Inserts a key-value pair into the map. @@ -1004,7 +1014,11 @@ where K: Borrow, Q: Hash + Eq, { - self.remove_entry(k).map(|(_, v)| v) + // Avoid `Option::map` because it bloats LLVM IR. + match self.remove_entry(k) { + Some((_, v)) => Some(v), + None => None, + } } /// Removes a key from the map, returning the stored key and value if the @@ -1561,13 +1575,13 @@ impl<'a, K, V, S> RawEntryBuilder<'a, K, V, S> { where F: FnMut(&K) -> bool, { - self.map - .table - .find(hash, |(k, _)| is_match(k)) - .map(|item| unsafe { + match self.map.table.find(hash, |(k, _)| is_match(k)) { + Some(item) => unsafe { let &(ref key, ref value) = item.as_ref(); - (key, value) - }) + Some((key, value)) + } + None => None, + } } /// Access an entry by hash. @@ -2030,10 +2044,14 @@ impl<'a, K, V> Iterator for Iter<'a, K, V> { #[cfg_attr(feature = "inline-more", inline)] fn next(&mut self) -> Option<(&'a K, &'a V)> { - self.inner.next().map(|x| unsafe { - let r = x.as_ref(); - (&r.0, &r.1) - }) + // Avoid `Option::map` because it bloats LLVM IR. + match self.inner.next() { + Some(x) => unsafe { + let r = x.as_ref(); + Some((&r.0, &r.1)) + } + None => None, + } } #[cfg_attr(feature = "inline-more", inline)] fn size_hint(&self) -> (usize, Option) { @@ -2054,10 +2072,14 @@ impl<'a, K, V> Iterator for IterMut<'a, K, V> { #[cfg_attr(feature = "inline-more", inline)] fn next(&mut self) -> Option<(&'a K, &'a mut V)> { - self.inner.next().map(|x| unsafe { - let r = x.as_mut(); - (&r.0, &mut r.1) - }) + // Avoid `Option::map` because it bloats LLVM IR. + match self.inner.next() { + Some(x) => unsafe { + let r = x.as_mut(); + Some((&r.0, &mut r.1)) + } + None => None, + } } #[cfg_attr(feature = "inline-more", inline)] fn size_hint(&self) -> (usize, Option) { @@ -2113,7 +2135,11 @@ impl<'a, K, V> Iterator for Keys<'a, K, V> { #[cfg_attr(feature = "inline-more", inline)] fn next(&mut self) -> Option<&'a K> { - self.inner.next().map(|(k, _)| k) + // Avoid `Option::map` because it bloats LLVM IR. + match self.inner.next() { + Some((k, _)) => Some(k), + None => None, + } } #[cfg_attr(feature = "inline-more", inline)] fn size_hint(&self) -> (usize, Option) { @@ -2133,7 +2159,11 @@ impl<'a, K, V> Iterator for Values<'a, K, V> { #[cfg_attr(feature = "inline-more", inline)] fn next(&mut self) -> Option<&'a V> { - self.inner.next().map(|(_, v)| v) + // Avoid `Option::map` because it bloats LLVM IR. + match self.inner.next() { + Some((_, v)) => Some(v), + None => None, + } } #[cfg_attr(feature = "inline-more", inline)] fn size_hint(&self) -> (usize, Option) { @@ -2153,7 +2183,11 @@ impl<'a, K, V> Iterator for ValuesMut<'a, K, V> { #[cfg_attr(feature = "inline-more", inline)] fn next(&mut self) -> Option<&'a mut V> { - self.inner.next().map(|(_, v)| v) + // Avoid `Option::map` because it bloats LLVM IR. + match self.inner.next() { + Some((_, v)) => Some(v), + None => None, + } } #[cfg_attr(feature = "inline-more", inline)] fn size_hint(&self) -> (usize, Option) { diff --git a/src/raw/mod.rs b/src/raw/mod.rs index 8fe520f084..d8deab5f50 100644 --- a/src/raw/mod.rs +++ b/src/raw/mod.rs @@ -402,9 +402,16 @@ impl RawTable { fallability: Fallibility, ) -> Result { debug_assert!(buckets.is_power_of_two()); - let (layout, ctrl_offset) = - calculate_layout::(buckets).ok_or_else(|| fallability.capacity_overflow())?; - let ptr = NonNull::new(alloc(layout)).ok_or_else(|| fallability.alloc_err(layout))?; + + // Avoid `Option::ok_or_else` because it bloats LLVM IR. + let (layout, ctrl_offset) = match calculate_layout::(buckets) { + Some(lco) => lco, + None => return Err(fallability.capacity_overflow()) + }; + let ptr = match NonNull::new(alloc(layout)) { + Some(ptr) => ptr, + None => return Err(fallability.alloc_err(layout)), + }; let ctrl = NonNull::new_unchecked(ptr.as_ptr().add(ctrl_offset)); Ok(Self { ctrl, @@ -425,8 +432,11 @@ impl RawTable { Ok(Self::new()) } else { unsafe { - let buckets = - capacity_to_buckets(capacity).ok_or_else(|| fallability.capacity_overflow())?; + // Avoid `Option::ok_or_else` because it bloats LLVM IR. + let buckets = match capacity_to_buckets(capacity) { + Some(buckets) => buckets, + None => return Err(fallability.capacity_overflow()), + }; let result = Self::new_uninitialized(buckets, fallability)?; result.ctrl(0).write_bytes(EMPTY, result.num_ctrl_bytes()); @@ -445,15 +455,21 @@ impl RawTable { /// Allocates a new hash table with at least enough capacity for inserting /// the given number of elements without reallocating. pub fn with_capacity(capacity: usize) -> Self { - Self::fallible_with_capacity(capacity, Fallibility::Infallible) - .unwrap_or_else(|_| unsafe { hint::unreachable_unchecked() }) + // Avoid `Result::unwrap_or_else` because it bloats LLVM IR. + match Self::fallible_with_capacity(capacity, Fallibility::Infallible) { + Ok(capacity) => capacity, + Err(_) => unsafe { hint::unreachable_unchecked() }, + } } /// Deallocates the table without dropping any entries. #[cfg_attr(feature = "inline-more", inline)] unsafe fn free_buckets(&mut self) { - let (layout, ctrl_offset) = - calculate_layout::(self.buckets()).unwrap_or_else(|| hint::unreachable_unchecked()); + // Avoid `Option::unwrap_or_else` because it bloats LLVM IR. + let (layout, ctrl_offset) = match calculate_layout::(self.buckets()) { + Some(lco) => lco, + None => hint::unreachable_unchecked(), + }; dealloc(self.ctrl.as_ptr().sub(ctrl_offset), layout); } @@ -671,8 +687,10 @@ impl RawTable { if self.items == 0 { *self = Self::with_capacity(min_size) } else { - self.resize(min_size, hasher, Fallibility::Infallible) - .unwrap_or_else(|_| unsafe { hint::unreachable_unchecked() }); + // Avoid `Result::unwrap_or_else` because it bloats LLVM IR. + if self.resize(min_size, hasher, Fallibility::Infallible).is_err() { + unsafe { hint::unreachable_unchecked() } + } } } } @@ -682,8 +700,10 @@ impl RawTable { #[cfg_attr(feature = "inline-more", inline)] pub fn reserve(&mut self, additional: usize, hasher: impl Fn(&T) -> u64) { if additional > self.growth_left { - self.reserve_rehash(additional, hasher, Fallibility::Infallible) - .unwrap_or_else(|_| unsafe { hint::unreachable_unchecked() }); + // Avoid `Result::unwrap_or_else` because it bloats LLVM IR. + if self.reserve_rehash(additional, hasher, Fallibility::Infallible).is_err() { + unsafe { hint::unreachable_unchecked() } + } } } @@ -711,11 +731,11 @@ impl RawTable { hasher: impl Fn(&T) -> u64, fallability: Fallibility, ) -> Result<(), TryReserveError> { - let new_items = self - .items - .checked_add(additional) - .ok_or_else(|| fallability.capacity_overflow())?; - + // Avoid `Option::ok_or_else` because it bloats LLVM IR. + let new_items = match self.items.checked_add(additional) { + Some(new_items) => new_items, + None => return Err(fallability.capacity_overflow()), + }; let full_capacity = bucket_mask_to_capacity(self.bucket_mask); if new_items <= full_capacity / 2 { // Rehash in-place without re-allocating if we have plenty of spare @@ -1065,8 +1085,11 @@ impl RawTable { let alloc = if self.is_empty_singleton() { None } else { - let (layout, ctrl_offset) = calculate_layout::(self.buckets()) - .unwrap_or_else(|| unsafe { hint::unreachable_unchecked() }); + // Avoid `Option::unwrap_or_else` because it bloats LLVM IR. + let (layout, ctrl_offset) = match calculate_layout::(self.buckets()) { + Some(lco) => lco, + None => unsafe { hint::unreachable_unchecked() }, + }; Some(( unsafe { NonNull::new_unchecked(self.ctrl.as_ptr().sub(ctrl_offset)) }, layout, @@ -1087,8 +1110,11 @@ impl Clone for RawTable { } else { unsafe { let mut new_table = ManuallyDrop::new( - Self::new_uninitialized(self.buckets(), Fallibility::Infallible) - .unwrap_or_else(|_| hint::unreachable_unchecked()), + // Avoid `Result::ok_or_else` because it bloats LLVM IR. + match Self::new_uninitialized(self.buckets(), Fallibility::Infallible) { + Ok(table) => table, + Err(_) => hint::unreachable_unchecked(), + } ); new_table.clone_from_spec(self, |new_table| { @@ -1121,8 +1147,11 @@ impl Clone for RawTable { self.free_buckets(); } (self as *mut Self).write( - Self::new_uninitialized(source.buckets(), Fallibility::Infallible) - .unwrap_or_else(|_| hint::unreachable_unchecked()), + // Avoid `Result::unwrap_or_else` because it bloats LLVM IR. + match Self::new_uninitialized(source.buckets(), Fallibility::Infallible) { + Ok(table) => table, + Err(_) => hint::unreachable_unchecked(), + } ); } diff --git a/src/set.rs b/src/set.rs index 9ba4c340ce..eaae82ce7d 100644 --- a/src/set.rs +++ b/src/set.rs @@ -687,7 +687,11 @@ where T: Borrow, Q: Hash + Eq, { - self.map.get_key_value(value).map(|(k, _)| k) + // Avoid `Option::map` because it bloats LLVM IR. + match self.map.get_key_value(value) { + Some((k, _)) => Some(k), + None => None, + } } /// Inserts the given `value` into the set if it is not present, then @@ -951,7 +955,11 @@ where T: Borrow, Q: Hash + Eq, { - self.map.remove_entry(value).map(|(k, _)| k) + // Avoid `Option::map` because it bloats LLVM IR. + match self.map.remove_entry(value) { + Some((k, _)) => Some(k), + None => None, + } } } @@ -1365,7 +1373,11 @@ impl Iterator for IntoIter { #[cfg_attr(feature = "inline-more", inline)] fn next(&mut self) -> Option { - self.iter.next().map(|(k, _)| k) + // Avoid `Option::map` because it bloats LLVM IR. + match self.iter.next() { + Some((k, _)) => Some(k), + None => None, + } } #[cfg_attr(feature = "inline-more", inline)] fn size_hint(&self) -> (usize, Option) { @@ -1392,7 +1404,11 @@ impl Iterator for Drain<'_, K> { #[cfg_attr(feature = "inline-more", inline)] fn next(&mut self) -> Option { - self.iter.next().map(|(k, _)| k) + // Avoid `Option::map` because it bloats LLVM IR. + match self.iter.next() { + Some((k, _)) => Some(k), + None => None, + } } #[cfg_attr(feature = "inline-more", inline)] fn size_hint(&self) -> (usize, Option) {