Skip to content

Commit

Permalink
Bug 1633410 - Revert "Bug 1631721 - Use hashbrown instead of hashglob…
Browse files Browse the repository at this point in the history
…e". r=manishearth a=pascalc

This reverts commit 64ad40e.

Differential Revision: https://phabricator.services.mozilla.com/D75488

Depends on D75487
  • Loading branch information
SimonSapin committed May 15, 2020
1 parent d8eeb42 commit f923374
Show file tree
Hide file tree
Showing 11 changed files with 66 additions and 92 deletions.
2 changes: 0 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion servo/components/fallible/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ path = "lib.rs"

[dependencies]
smallvec = "1.0"
hashbrown = "0.7"
hashglobe = { path = "../hashglobe" }

# This crate effectively does nothing except if the `known_system_malloc`
Expand Down
78 changes: 17 additions & 61 deletions servo/components/fallible/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,70 +2,28 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/. */

extern crate hashbrown;
extern crate hashglobe;
extern crate smallvec;

use hashbrown::hash_map::Entry;
use hashbrown::CollectionAllocErr;
#[cfg(feature = "known_system_malloc")]
use hashglobe::alloc;
use hashglobe::FailedAllocationError;
use smallvec::Array;
use smallvec::SmallVec;
use std::alloc::Layout;
use std::vec::Vec;

pub trait FallibleVec<T> {
/// Append |val| to the end of |vec|. Returns Ok(()) on success,
/// Err(reason) if it fails, with |reason| describing the failure.
fn try_push(&mut self, value: T) -> Result<(), CollectionAllocErr>;
}

pub trait FallibleHashMap<K, V, H> {
fn try_insert(&mut self, k: K, v: V) -> Result<Option<V>, CollectionAllocErr>;
fn try_entry(&mut self, k: K) -> Result<Entry<K, V, H>, CollectionAllocErr>;
}

pub trait FallibleHashSet<T, H> {
fn try_insert(&mut self, x: T) -> Result<bool, CollectionAllocErr>;
}

impl<K, V, H> FallibleHashMap<K, V, H> for hashbrown::HashMap<K, V, H>
where
K: Eq + std::hash::Hash,
H: std::hash::BuildHasher,
{
#[inline]
fn try_insert(&mut self, k: K, v: V) -> Result<Option<V>, CollectionAllocErr> {
self.try_reserve(1)?;
Ok(self.insert(k, v))
}

#[inline]
fn try_entry(&mut self, k: K) -> Result<Entry<K, V, H>, CollectionAllocErr> {
self.try_reserve(1)?;
Ok(self.entry(k))
}
}

impl<T, H> FallibleHashSet<T, H> for hashbrown::HashSet<T, H>
where
T: Eq + std::hash::Hash,
H: std::hash::BuildHasher,
{
#[inline]
fn try_insert(&mut self, x: T) -> Result<bool, CollectionAllocErr> {
self.try_reserve(1)?;
Ok(self.insert(x))
}
fn try_push(&mut self, value: T) -> Result<(), FailedAllocationError>;
}

/////////////////////////////////////////////////////////////////
// Vec

impl<T> FallibleVec<T> for Vec<T> {
#[inline(always)]
fn try_push(&mut self, val: T) -> Result<(), CollectionAllocErr> {
fn try_push(&mut self, val: T) -> Result<(), FailedAllocationError> {
#[cfg(feature = "known_system_malloc")]
{
if self.capacity() == self.len() {
Expand All @@ -83,7 +41,7 @@ impl<T> FallibleVec<T> for Vec<T> {
#[cfg(feature = "known_system_malloc")]
#[inline(never)]
#[cold]
fn try_double_vec<T>(vec: &mut Vec<T>) -> Result<(), CollectionAllocErr> {
fn try_double_vec<T>(vec: &mut Vec<T>) -> Result<(), FailedAllocationError> {
use std::mem;

let old_ptr = vec.as_mut_ptr();
Expand All @@ -95,15 +53,12 @@ fn try_double_vec<T>(vec: &mut Vec<T>) -> Result<(), CollectionAllocErr> {
} else {
old_cap
.checked_mul(2)
.ok_or(CollectionAllocErr::CapacityOverflow)?
.ok_or(FailedAllocationError::new("capacity overflow for Vec"))?
};

let new_size_bytes = new_cap
.checked_mul(mem::size_of::<T>())
.ok_or(CollectionAllocErr::CapacityOverflow)?;

let layout = Layout::from_size_align(new_size_bytes, std::mem::align_of::<T>())
.map_err(|_| CollectionAllocErr::CapacityOverflow)?;
.ok_or(FailedAllocationError::new("capacity overflow for Vec"))?;

let new_ptr = unsafe {
if old_cap == 0 {
Expand All @@ -114,7 +69,9 @@ fn try_double_vec<T>(vec: &mut Vec<T>) -> Result<(), CollectionAllocErr> {
};

if new_ptr.is_null() {
return Err(CollectionAllocErr::AllocErr { layout });
return Err(FailedAllocationError::new(
"out of memory when allocating Vec",
));
}

let new_vec = unsafe { Vec::from_raw_parts(new_ptr as *mut T, old_len, new_cap) };
Expand All @@ -128,7 +85,7 @@ fn try_double_vec<T>(vec: &mut Vec<T>) -> Result<(), CollectionAllocErr> {

impl<T: Array> FallibleVec<T::Item> for SmallVec<T> {
#[inline(always)]
fn try_push(&mut self, val: T::Item) -> Result<(), CollectionAllocErr> {
fn try_push(&mut self, val: T::Item) -> Result<(), FailedAllocationError> {
#[cfg(feature = "known_system_malloc")]
{
if self.capacity() == self.len() {
Expand All @@ -146,7 +103,7 @@ impl<T: Array> FallibleVec<T::Item> for SmallVec<T> {
#[cfg(feature = "known_system_malloc")]
#[inline(never)]
#[cold]
fn try_double_small_vec<T>(svec: &mut SmallVec<T>) -> Result<(), CollectionAllocErr>
fn try_double_small_vec<T>(svec: &mut SmallVec<T>) -> Result<(), FailedAllocationError>
where
T: Array,
{
Expand All @@ -162,21 +119,18 @@ where
} else {
old_cap
.checked_mul(2)
.ok_or(CollectionAllocErr::CapacityOverflow)?
.ok_or(FailedAllocationError::new("capacity overflow for SmallVec"))?
};

// This surely shouldn't fail, if |old_cap| was previously accepted as a
// valid value. But err on the side of caution.
let old_size_bytes = old_cap
.checked_mul(mem::size_of::<T>())
.ok_or(CollectionAllocErr::CapacityOverflow)?;
.ok_or(FailedAllocationError::new("capacity overflow for SmallVec"))?;

let new_size_bytes = new_cap
.checked_mul(mem::size_of::<T>())
.ok_or(CollectionAllocErr::CapacityOverflow)?;

let layout = Layout::from_size_align(new_size_bytes, std::mem::align_of::<T>())
.map_err(|_| CollectionAllocErr::CapacityOverflow)?;
.ok_or(FailedAllocationError::new("capacity overflow for SmallVec"))?;

let new_ptr;
if svec.spilled() {
Expand All @@ -196,7 +150,9 @@ where
}

if new_ptr.is_null() {
return Err(CollectionAllocErr::AllocErr { layout });
return Err(FailedAllocationError::new(
"out of memory when allocating SmallVec",
));
}

let new_vec = unsafe { Vec::from_raw_parts(new_ptr as *mut T::Item, old_len, new_cap) };
Expand Down
1 change: 0 additions & 1 deletion servo/components/malloc_size_of/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ content-security-policy = {version = "0.3.0", features = ["serde"], optional = t
crossbeam-channel = { version = "0.3", optional = true }
cssparser = "0.27"
euclid = "0.20"
hashbrown = "0.7"
hashglobe = { path = "../hashglobe" }
hyper = { version = "0.12", optional = true }
hyper_serde = { version = "0.11", optional = true }
Expand Down
3 changes: 0 additions & 3 deletions servo/components/malloc_size_of/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ extern crate content_security_policy;
extern crate crossbeam_channel;
extern crate cssparser;
extern crate euclid;
extern crate hashbrown;
extern crate hashglobe;
#[cfg(feature = "servo")]
extern crate hyper;
Expand Down Expand Up @@ -488,7 +487,6 @@ macro_rules! malloc_size_of_hash_set {
}

malloc_size_of_hash_set!(std::collections::HashSet<T, S>);
malloc_size_of_hash_set!(hashbrown::HashSet<T, S>);
malloc_size_of_hash_set!(hashglobe::hash_set::HashSet<T, S>);
malloc_size_of_hash_set!(hashglobe::fake::HashSet<T, S>);

Expand Down Expand Up @@ -530,7 +528,6 @@ macro_rules! malloc_size_of_hash_map {
}

malloc_size_of_hash_map!(std::collections::HashMap<K, V, S>);
malloc_size_of_hash_map!(hashbrown::HashMap<K, V, S>);
malloc_size_of_hash_map!(hashglobe::hash_map::HashMap<K, V, S>);
malloc_size_of_hash_map!(hashglobe::fake::HashMap<K, V, S>);

Expand Down
21 changes: 19 additions & 2 deletions servo/components/style/hash.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,28 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/. */

//! Reexports of hashbrown, without and with FxHash
//! Reexports of hashglobe types in Gecko mode, and stdlib hashmap shims in Servo mode
//!
//! Can go away when the stdlib gets fallible collections
//! https://github.com/rust-lang/rfcs/pull/2116
use fxhash;

pub use hashbrown::{hash_map as map, HashMap, HashSet};
#[cfg(feature = "gecko")]
pub use hashglobe::hash_map::HashMap;
#[cfg(feature = "gecko")]
pub use hashglobe::hash_set::HashSet;

#[cfg(feature = "servo")]
pub use hashglobe::fake::{HashMap, HashSet};

/// Appropriate reexports of hash_map types
pub mod map {
#[cfg(feature = "gecko")]
pub use hashglobe::hash_map::{Entry, Iter};
#[cfg(feature = "servo")]
pub use std::collections::hash_map::{Entry, Iter};
}

/// Hash map that uses the Fx hasher
pub type FxHashMap<K, V> = HashMap<K, V, fxhash::FxBuildHasher>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use crate::selector_map::{MaybeCaseInsensitiveHashMap, SelectorMap, SelectorMapE
use crate::selector_parser::SelectorImpl;
use crate::{Atom, LocalName, Namespace};
use fallible::FallibleVec;
use hashbrown::CollectionAllocErr;
use hashglobe::FailedAllocationError;
use selectors::attr::NamespaceConstraint;
use selectors::parser::{Combinator, Component};
use selectors::parser::{Selector, SelectorIter};
Expand All @@ -33,7 +33,7 @@ use smallvec::SmallVec;
/// We generate a Dependency for both |a _ b:X _| and |a _ b:X _ c _ d:Y _|,
/// even though those selectors may not appear on their own in any stylesheet.
/// This allows us to quickly scan through the dependency sites of all style
/// rules and determine the maximum effect that a given state or attribute
/// rules and determine the maximum effect that a given state or attributef
/// change may have on the style of elements in the document.
#[derive(Clone, Debug, MallocSizeOf)]
pub struct Dependency {
Expand Down Expand Up @@ -257,7 +257,7 @@ impl InvalidationMap {
&mut self,
selector: &Selector<SelectorImpl>,
quirks_mode: QuirksMode,
) -> Result<(), CollectionAllocErr> {
) -> Result<(), FailedAllocationError> {
debug!("InvalidationMap::note_selector({:?})", selector);

let mut document_state = DocumentState::empty();
Expand Down Expand Up @@ -342,7 +342,7 @@ struct SelectorDependencyCollector<'a> {
compound_state: PerCompoundState,

/// The allocation error, if we OOM.
alloc_error: &'a mut Option<CollectionAllocErr>,
alloc_error: &'a mut Option<FailedAllocationError>,
}

impl<'a> SelectorDependencyCollector<'a> {
Expand Down
1 change: 0 additions & 1 deletion servo/components/style/invalidation/stylesheets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ use crate::stylesheets::{CssRule, StylesheetInDocument};
use crate::Atom;
use crate::CaseSensitivityExt;
use crate::LocalName as SelectorLocalName;
use fallible::FallibleHashSet;
use fxhash::FxHasher;
use selectors::attr::CaseSensitivity;
use selectors::parser::{Component, LocalName, Selector};
Expand Down
2 changes: 1 addition & 1 deletion servo/components/style/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ extern crate fxhash;
#[cfg(feature = "gecko")]
#[macro_use]
pub mod gecko_string_cache;
extern crate hashbrown;
extern crate hashglobe;
#[cfg(feature = "servo")]
#[macro_use]
extern crate html5ever;
Expand Down
29 changes: 19 additions & 10 deletions servo/components/style/selector_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ use crate::rule_tree::CascadeLevel;
use crate::selector_parser::SelectorImpl;
use crate::stylist::Rule;
use crate::{Atom, LocalName, Namespace, WeakAtom};
use fallible::{FallibleHashMap, FallibleVec};
use hashbrown::CollectionAllocErr;
use fallible::FallibleVec;
use hashglobe::FailedAllocationError;
use precomputed_hash::PrecomputedHash;
use selectors::matching::{matches_selector, ElementSelectorFlags, MatchingContext};
use selectors::parser::{Combinator, Component, SelectorIter};
Expand Down Expand Up @@ -95,7 +95,7 @@ pub trait SelectorMapEntry: Sized + Clone {
///
/// TODO: Tune the initial capacity of the HashMap
#[derive(Debug, MallocSizeOf)]
pub struct SelectorMap<T> {
pub struct SelectorMap<T: 'static> {
/// Rules that have `:root` selectors.
pub root: SmallVec<[T; 1]>,
/// A hash from an ID to rules which contain that ID selector.
Expand All @@ -112,14 +112,17 @@ pub struct SelectorMap<T> {
pub count: usize,
}

impl<T> Default for SelectorMap<T> {
impl<T: 'static> Default for SelectorMap<T> {
#[inline]
fn default() -> Self {
Self::new()
}
}

impl<T> SelectorMap<T> {
// FIXME(Manishearth) the 'static bound can be removed when
// our HashMap fork (hashglobe) is able to use NonZero,
// or when stdlib gets fallible collections
impl<T: 'static> SelectorMap<T> {
/// Trivially constructs an empty `SelectorMap`.
pub fn new() -> Self {
SelectorMap {
Expand Down Expand Up @@ -276,7 +279,11 @@ impl SelectorMap<Rule> {

impl<T: SelectorMapEntry> SelectorMap<T> {
/// Inserts an entry into the correct bucket(s).
pub fn insert(&mut self, entry: T, quirks_mode: QuirksMode) -> Result<(), CollectionAllocErr> {
pub fn insert(
&mut self,
entry: T,
quirks_mode: QuirksMode,
) -> Result<(), FailedAllocationError> {
self.count += 1;

// NOTE(emilio): It'd be nice for this to be a separate function, but
Expand Down Expand Up @@ -604,11 +611,14 @@ fn find_bucket<'a>(

/// Wrapper for PrecomputedHashMap that does ASCII-case-insensitive lookup in quirks mode.
#[derive(Debug, MallocSizeOf)]
pub struct MaybeCaseInsensitiveHashMap<K: PrecomputedHash + Hash + Eq, V>(
pub struct MaybeCaseInsensitiveHashMap<K: PrecomputedHash + Hash + Eq, V: 'static>(
PrecomputedHashMap<K, V>,
);

impl<V> MaybeCaseInsensitiveHashMap<Atom, V> {
// FIXME(Manishearth) the 'static bound can be removed when
// our HashMap fork (hashglobe) is able to use NonZero,
// or when stdlib gets fallible collections
impl<V: 'static> MaybeCaseInsensitiveHashMap<Atom, V> {
/// Empty map
pub fn new() -> Self {
MaybeCaseInsensitiveHashMap(PrecomputedHashMap::default())
Expand All @@ -619,8 +629,7 @@ impl<V> MaybeCaseInsensitiveHashMap<Atom, V> {
&mut self,
mut key: Atom,
quirks_mode: QuirksMode,
) -> Result<hash_map::Entry<Atom, V, BuildHasherDefault<PrecomputedHasher>>, CollectionAllocErr>
{
) -> Result<hash_map::Entry<Atom, V>, FailedAllocationError> {
if quirks_mode == QuirksMode::Quirks {
key = key.to_ascii_lowercase()
}
Expand Down
Loading

0 comments on commit f923374

Please sign in to comment.