Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove OrderedMap in favor of IndexMap #22656

Merged
merged 1 commit into from Jan 10, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Remove OrderedMap in favor of IndexMap

  • Loading branch information
shanavas786 committed Jan 10, 2019
commit b763d6737cf25b94dd94a37ed360cdd61d275a8f

Some generated files are not rendered by default. Learn more.

@@ -39,6 +39,7 @@ fallible = { path = "../fallible" }
fxhash = "0.2"
hashglobe = { path = "../hashglobe" }
html5ever = {version = "0.22", optional = true}
indexmap = "1.0.2"
itertools = "0.8"
itoa = "0.4"
lazy_static = "1"
@@ -8,17 +8,17 @@

use crate::hash::map::Entry;
use crate::properties::{CSSWideKeyword, CustomDeclarationValue};
use crate::selector_map::{PrecomputedHashMap, PrecomputedHashSet};
use crate::selector_map::{PrecomputedHashMap, PrecomputedHashSet, PrecomputedHasher};
use crate::Atom;
use cssparser::{Delimiter, Parser, ParserInput, SourcePosition, Token, TokenSerializationType};
use precomputed_hash::PrecomputedHash;
use indexmap::IndexMap;
use selectors::parser::SelectorParseErrorKind;
use servo_arc::Arc;
use smallvec::SmallVec;
use std::borrow::{Borrow, Cow};
use std::borrow::Cow;
use std::cmp;
use std::fmt::{self, Write};
use std::hash::Hash;
use std::hash::BuildHasherDefault;
use style_traits::{CssWriter, ParseError, StyleParseErrorKind, ToCss};

/// The environment from which to get `env` function values.
@@ -131,7 +131,8 @@ impl ToCss for SpecifiedValue {
///
/// The variable values are guaranteed to not have references to other
/// properties.
pub type CustomPropertiesMap = OrderedMap<Name, Arc<VariableValue>>;
pub type CustomPropertiesMap =
IndexMap<Name, Arc<VariableValue>, BuildHasherDefault<PrecomputedHasher>>;

/// Both specified and computed values are VariableValues, the difference is
/// whether var() functions are expanded.
@@ -140,130 +141,6 @@ pub type SpecifiedValue = VariableValue;
/// whether var() functions are expanded.
pub type ComputedValue = VariableValue;

/// A map that preserves order for the keys, and that is easily indexable.
#[derive(Clone, Debug, Eq, PartialEq)]
pub struct OrderedMap<K, V>
where
K: PrecomputedHash + Hash + Eq + Clone,
{
/// Key index.
index: Vec<K>,
/// Key-value map.
values: PrecomputedHashMap<K, V>,
}

impl<K, V> OrderedMap<K, V>
where
K: Eq + PrecomputedHash + Hash + Clone,
{
/// Creates a new ordered map.
pub fn new() -> Self {
OrderedMap {
index: Vec::new(),
values: PrecomputedHashMap::default(),
}
}

/// Insert a new key-value pair.
///
/// TODO(emilio): Remove unused_mut when Gecko and Servo agree in whether
/// it's necessary.
#[allow(unused_mut)]
pub fn insert(&mut self, key: K, value: V) {
let OrderedMap {
ref mut index,
ref mut values,
} = *self;
match values.entry(key) {
Entry::Vacant(mut entry) => {
index.push(entry.key().clone());
entry.insert(value);
},
Entry::Occupied(mut entry) => {
entry.insert(value);
},
}
}

/// Get a value given its key.
pub fn get(&self, key: &K) -> Option<&V> {
let value = self.values.get(key);
debug_assert_eq!(value.is_some(), self.index.contains(key));
value
}

/// Get whether there's a value on the map for `key`.
pub fn contains_key(&self, key: &K) -> bool {
self.values.contains_key(key)
}

/// Get the key located at the given index.
pub fn get_key_at(&self, index: u32) -> Option<&K> {
self.index.get(index as usize)
}

/// Get an ordered map iterator.
pub fn iter<'a>(&'a self) -> OrderedMapIterator<'a, K, V> {
OrderedMapIterator {
inner: self,
pos: 0,
}
}

/// Get the count of items in the map.
pub fn len(&self) -> usize {
debug_assert_eq!(self.values.len(), self.index.len());
self.values.len()
}

/// Returns whether this map is empty.
pub fn is_empty(&self) -> bool {
self.len() == 0
}

/// Remove an item given its key.
fn remove<Q: ?Sized>(&mut self, key: &Q) -> Option<V>
where
K: Borrow<Q>,
Q: PrecomputedHash + Hash + Eq,
{
let index = self.index.iter().position(|k| k.borrow() == key)?;
self.index.remove(index);
self.values.remove(key)
}
}

/// An iterator for OrderedMap.
///
/// The iteration order is determined by the order that the values are
/// added to the key-value map.
pub struct OrderedMapIterator<'a, K, V>
where
K: 'a + Eq + PrecomputedHash + Hash + Clone,
V: 'a,
{
/// The OrderedMap itself.
inner: &'a OrderedMap<K, V>,
/// The position of the iterator.
pos: usize,
}

impl<'a, K, V> Iterator for OrderedMapIterator<'a, K, V>
where
K: Eq + PrecomputedHash + Hash + Clone,
{
type Item = (&'a K, &'a V);

fn next(&mut self) -> Option<Self::Item> {
let key = self.inner.index.get(self.pos)?;

self.pos += 1;
let value = &self.inner.values[key];

Some((key, value))
}
}

/// A struct holding information about the external references to that a custom
/// property value may have.
#[derive(Default)]
@@ -648,7 +525,7 @@ impl<'a> CustomPropertiesBuilder<'a> {
if self.custom_properties.is_none() {
self.custom_properties = Some(match self.inherited {
Some(inherited) => (**inherited).clone(),
None => CustomPropertiesMap::new(),
None => CustomPropertiesMap::default(),
});
}

@@ -933,7 +810,7 @@ fn substitute_all(custom_properties_map: &mut CustomPropertiesMap, environment:

// We have to clone the names so that we can mutably borrow the map
// in the context we create for traversal.
let names = custom_properties_map.index.clone();
let names: Vec<_> = custom_properties_map.keys().cloned().collect();
for name in names.into_iter() {
This conversation was marked as resolved by CYBAI

This comment has been minimized.

@CYBAI

CYBAI Jan 9, 2019

Collaborator

Could we just iterate with keys() or maybe keys().into_iter() ?

- for name in names.into_iter() {
+ for name in custom_properties_map.keys() {

This comment has been minimized.

@shanavas786

shanavas786 Jan 9, 2019

Author Contributor

keys() iterates over reference.
keys().cloned() gives value but both keeps an immutable reference to custom_properties_map which prevents from borrowing it mutably at

            map: custom_properties_map,

This comment has been minimized.

@CYBAI

CYBAI Jan 9, 2019

Collaborator

Oh, understood! But maybe we don't need to collect() it and then into_iter()? 🤔

Ex.

for name in custom_properties_map.keys().cloned() {

This comment has been minimized.

@KiChjang

KiChjang Jan 9, 2019

Member

If cloned() doesn't work, perhaps map.keys().map(|name| name.clone()) would?

This comment has been minimized.

@emilio

emilio Jan 10, 2019

Member

collect() is fine for now. If we wanted to avoid the heap allocation we could collect into a SmallVec or such, but that should probably be a separate PR, and probably with some measurement, indication that it can actually help.

let mut context = Context {
count: 0,
@@ -1112,7 +989,7 @@ pub fn substitute<'i>(
let mut input = ParserInput::new(input);
let mut input = Parser::new(&mut input);
let mut position = (input.position(), first_token_type);
let empty_map = CustomPropertiesMap::new();
let empty_map = CustomPropertiesMap::default();
let custom_properties = match computed_values_map {
Some(m) => &**m,
None => &empty_map,
@@ -52,6 +52,7 @@ extern crate hashglobe;
#[cfg(feature = "servo")]
#[macro_use]
extern crate html5ever;
extern crate indexmap;
extern crate itertools;
extern crate itoa;
#[macro_use]
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.