From 711e04dfc42535558c551d7596793a0afcfed763 Mon Sep 17 00:00:00 2001 From: joshwd36 Date: Thu, 22 Jul 2021 12:57:06 +0100 Subject: [PATCH] Use lock for map iteration (#1366) --- boa/src/builtins/map/map_iterator.rs | 21 +-- boa/src/builtins/map/mod.rs | 48 ++++--- boa/src/builtins/map/ordered_map.rs | 191 ++++++++++++++++++--------- boa/src/builtins/map/tests.rs | 50 +++++++ boa/src/object/mod.rs | 6 +- 5 files changed, 222 insertions(+), 94 deletions(-) diff --git a/boa/src/builtins/map/map_iterator.rs b/boa/src/builtins/map/map_iterator.rs index 566f421ec2..aba39df089 100644 --- a/boa/src/builtins/map/map_iterator.rs +++ b/boa/src/builtins/map/map_iterator.rs @@ -7,6 +7,8 @@ use crate::{ }; use gc::{Finalize, Trace}; +use super::{ordered_map::MapLock, Map}; + #[derive(Debug, Clone, Finalize, Trace)] pub enum MapIterationKind { Key, @@ -25,18 +27,21 @@ pub struct MapIterator { iterated_map: Value, map_next_index: usize, map_iteration_kind: MapIterationKind, + lock: MapLock, } impl MapIterator { pub(crate) const NAME: &'static str = "MapIterator"; /// Constructs a new `MapIterator`, that will iterate over `map`, starting at index 0 - fn new(map: Value, kind: MapIterationKind) -> Self { - MapIterator { + fn new(map: Value, kind: MapIterationKind, context: &mut Context) -> Result { + let lock = Map::lock(&map, context)?; + Ok(MapIterator { iterated_map: map, map_next_index: 0, map_iteration_kind: kind, - } + lock, + }) } /// Abstract operation CreateMapIterator( map, kind ) @@ -48,17 +53,17 @@ impl MapIterator { /// /// [spec]: https://www.ecma-international.org/ecma-262/11.0/index.html#sec-createmapiterator pub(crate) fn create_map_iterator( - context: &Context, + context: &mut Context, map: Value, kind: MapIterationKind, - ) -> Value { + ) -> Result { let map_iterator = Value::new_object(context); - map_iterator.set_data(ObjectData::MapIterator(Self::new(map, kind))); + map_iterator.set_data(ObjectData::MapIterator(Self::new(map, kind, context)?)); map_iterator .as_object() .expect("map iterator object") .set_prototype_instance(context.iterator_prototypes().map_iterator().into()); - map_iterator + Ok(map_iterator) } /// %MapIteratorPrototype%.next( ) @@ -83,7 +88,7 @@ impl MapIterator { if let Value::Object(ref object) = m { if let Some(entries) = object.borrow().as_map_ref() { - let num_entries = entries.len(); + let num_entries = entries.full_len(); while index < num_entries { let e = entries.get_index(index); index += 1; diff --git a/boa/src/builtins/map/mod.rs b/boa/src/builtins/map/mod.rs index f5b3c0c847..31aee09466 100644 --- a/boa/src/builtins/map/mod.rs +++ b/boa/src/builtins/map/mod.rs @@ -24,12 +24,14 @@ use ordered_map::OrderedMap; pub mod map_iterator; use map_iterator::{MapIterationKind, MapIterator}; +use self::ordered_map::MapLock; + pub mod ordered_map; #[cfg(test)] mod tests; #[derive(Debug, Clone)] -pub(crate) struct Map(OrderedMap); +pub(crate) struct Map(OrderedMap); impl BuiltIn for Map { const NAME: &'static str = "Map"; @@ -198,11 +200,7 @@ impl Map { /// [spec]: https://www.ecma-international.org/ecma-262/11.0/index.html#sec-map.prototype.entries /// [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/entries pub(crate) fn entries(this: &Value, _: &[Value], context: &mut Context) -> Result { - Ok(MapIterator::create_map_iterator( - context, - this.clone(), - MapIterationKind::KeyAndValue, - )) + MapIterator::create_map_iterator(context, this.clone(), MapIterationKind::KeyAndValue) } /// `Map.prototype.keys()` @@ -216,11 +214,7 @@ impl Map { /// [spec]: https://tc39.es/ecma262/#sec-map.prototype.keys /// [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/keys pub(crate) fn keys(this: &Value, _: &[Value], context: &mut Context) -> Result { - Ok(MapIterator::create_map_iterator( - context, - this.clone(), - MapIterationKind::Key, - )) + MapIterator::create_map_iterator(context, this.clone(), MapIterationKind::Key) } /// Helper function to set the size property. @@ -392,7 +386,9 @@ impl Map { let mut index = 0; - while index < Map::get_size(this, context)? { + let lock = Map::lock(this, context)?; + + while index < Map::get_full_len(this, context)? { let arguments = if let Value::Object(ref object) = this { let object = object.borrow(); if let Some(map) = object.as_map_ref() { @@ -415,15 +411,31 @@ impl Map { index += 1; } + drop(lock); + Ok(Value::Undefined) } - /// Helper function to get the size of the map. - fn get_size(map: &Value, context: &mut Context) -> Result { + /// Helper function to get the full size of the map. + fn get_full_len(map: &Value, context: &mut Context) -> Result { if let Value::Object(ref object) = map { let object = object.borrow(); if let Some(map) = object.as_map_ref() { - Ok(map.len()) + Ok(map.full_len()) + } else { + Err(context.construct_type_error("'this' is not a Map")) + } + } else { + Err(context.construct_type_error("'this' is not a Map")) + } + } + + /// Helper function to lock the map. + fn lock(map: &Value, context: &mut Context) -> Result { + if let Value::Object(ref object) = map { + let mut map = object.borrow_mut(); + if let Some(map) = map.as_map_mut() { + Ok(map.lock(object.clone())) } else { Err(context.construct_type_error("'this' is not a Map")) } @@ -443,11 +455,7 @@ impl Map { /// [spec]: https://tc39.es/ecma262/#sec-map.prototype.values /// [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/values pub(crate) fn values(this: &Value, _: &[Value], context: &mut Context) -> Result { - Ok(MapIterator::create_map_iterator( - context, - this.clone(), - MapIterationKind::Value, - )) + MapIterator::create_map_iterator(context, this.clone(), MapIterationKind::Value) } /// Helper function to get a key-value pair from an array. diff --git a/boa/src/builtins/map/ordered_map.rs b/boa/src/builtins/map/ordered_map.rs index 195acee55f..4a92e5edee 100644 --- a/boa/src/builtins/map/ordered_map.rs +++ b/boa/src/builtins/map/ordered_map.rs @@ -1,63 +1,109 @@ -use crate::gc::{custom_trace, Finalize, Trace}; -use indexmap::{map::IntoIter, map::Iter, map::IterMut, IndexMap}; +use crate::{ + gc::{custom_trace, Finalize, Trace}, + object::GcObject, + Value, +}; +use indexmap::{Equivalent, IndexMap}; use std::{ collections::hash_map::RandomState, fmt::Debug, - hash::{BuildHasher, Hash}, + hash::{BuildHasher, Hash, Hasher}, }; +#[derive(PartialEq, Eq, Clone, Debug)] +enum MapKey { + Key(Value), + Empty(usize), // Necessary to ensure empty keys are still unique. +} + +// This ensures that a MapKey::Key(value) hashes to the same as value. The derived PartialEq implementation still holds. +#[allow(clippy::derive_hash_xor_eq)] +impl Hash for MapKey { + fn hash(&self, state: &mut H) { + match self { + MapKey::Key(v) => v.hash(state), + MapKey::Empty(e) => e.hash(state), + } + } +} + +impl Equivalent for Value { + fn equivalent(&self, key: &MapKey) -> bool { + match key { + MapKey::Key(v) => v == self, + _ => false, + } + } +} + /// A newtype wrapping indexmap::IndexMap #[derive(Clone)] -pub struct OrderedMap(IndexMap) -where - K: Hash + Eq; +pub struct OrderedMap { + map: IndexMap, S>, + lock: u32, + empty_count: usize, +} -impl Finalize for OrderedMap {} -unsafe impl Trace for OrderedMap { +impl Finalize for OrderedMap {} +unsafe impl Trace for OrderedMap { custom_trace!(this, { - for (k, v) in this.0.iter() { - mark(k); + for (k, v) in this.map.iter() { + if let MapKey::Key(key) = k { + mark(key); + } mark(v); } }); } -impl Debug for OrderedMap { +impl Debug for OrderedMap { fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { - self.0.fmt(formatter) + self.map.fmt(formatter) } } -impl Default for OrderedMap { +impl Default for OrderedMap { fn default() -> Self { Self::new() } } -impl OrderedMap -where - K: Hash + Eq, -{ +impl OrderedMap { pub fn new() -> Self { - OrderedMap(IndexMap::new()) + OrderedMap { + map: IndexMap::new(), + lock: 0, + empty_count: 0, + } } pub fn with_capacity(capacity: usize) -> Self { - OrderedMap(IndexMap::with_capacity(capacity)) + OrderedMap { + map: IndexMap::with_capacity(capacity), + lock: 0, + empty_count: 0, + } } - /// Return the number of key-value pairs in the map. + /// Return the number of key-value pairs in the map, including empty values. + /// + /// Computes in **O(1)** time. + pub fn full_len(&self) -> usize { + self.map.len() + } + + /// Gets the number of key-value pairs in the map, not including empty values. /// /// Computes in **O(1)** time. pub fn len(&self) -> usize { - self.0.len() + self.map.len() - self.empty_count } /// Returns true if the map contains no elements. /// /// Computes in **O(1)** time. pub fn is_empty(&self) -> bool { - self.0.len() == 0 + self.len() == 0 } /// Insert a key-value pair in the map. @@ -70,8 +116,8 @@ where /// inserted, last in order, and `None` is returned. /// /// Computes in **O(1)** time (amortized average). - pub fn insert(&mut self, key: K, value: V) -> Option { - self.0.insert(key, value) + pub fn insert(&mut self, key: Value, value: V) -> Option { + self.map.insert(MapKey::Key(key), Some(value)).flatten() } /// Remove the key-value pair equivalent to `key` and return @@ -84,70 +130,89 @@ where /// Return `None` if `key` is not in map. /// /// Computes in **O(n)** time (average). - pub fn remove(&mut self, key: &K) -> Option { - self.0.shift_remove(key) + pub fn remove(&mut self, key: &Value) -> Option { + if self.lock == 0 { + self.map.shift_remove(key).flatten() + } else if self.map.contains_key(key) { + self.map.insert(MapKey::Empty(self.empty_count), None); + self.empty_count += 1; + self.map.swap_remove(key).flatten() + } else { + None + } } /// Return a reference to the value stored for `key`, if it is present, /// else `None`. /// /// Computes in **O(1)** time (average). - pub fn get(&self, key: &K) -> Option<&V> { - self.0.get(key) + pub fn get(&self, key: &Value) -> Option<&V> { + self.map.get(key).map(Option::as_ref).flatten() } /// Get a key-value pair by index - /// Valid indices are 0 <= index < self.len() + /// Valid indices are 0 <= index < self.full_len() /// Computes in O(1) time. - pub fn get_index(&self, index: usize) -> Option<(&K, &V)> { - self.0.get_index(index) + pub fn get_index(&self, index: usize) -> Option<(&Value, &V)> { + if let (MapKey::Key(key), Some(value)) = self.map.get_index(index)? { + Some((key, value)) + } else { + None + } } /// Return an iterator over the key-value pairs of the map, in their order - pub fn iter(&self) -> Iter<'_, K, V> { - self.0.iter() + pub fn iter(&self) -> impl Iterator { + self.map.iter().filter_map(|o| { + if let (MapKey::Key(key), Some(value)) = o { + Some((key, value)) + } else { + None + } + }) } /// Return `true` if an equivalent to `key` exists in the map. /// /// Computes in **O(1)** time (average). - pub fn contains_key(&self, key: &K) -> bool { - self.0.contains_key(key) + pub fn contains_key(&self, key: &Value) -> bool { + self.map.contains_key(key) + } + + /// Increases the lock counter and returns a lock object that will decrement the counter when dropped. + /// + /// This allows objects to be removed from the map during iteration without affecting the indexes until the iteration has completed. + pub(crate) fn lock(&mut self, map: GcObject) -> MapLock { + self.lock += 1; + MapLock(map) } -} -impl<'a, K, V, S> IntoIterator for &'a OrderedMap -where - K: Hash + Eq, - S: BuildHasher, -{ - type Item = (&'a K, &'a V); - type IntoIter = Iter<'a, K, V>; - fn into_iter(self) -> Self::IntoIter { - self.0.iter() + /// Decreases the lock counter and, if 0, removes all empty entries. + fn unlock(&mut self) { + self.lock -= 1; + if self.lock == 0 { + self.map.retain(|k, _| matches!(k, MapKey::Key(_))); + self.empty_count = 0; + } } } -impl<'a, K, V, S> IntoIterator for &'a mut OrderedMap -where - K: Hash + Eq, - S: BuildHasher, -{ - type Item = (&'a K, &'a mut V); - type IntoIter = IterMut<'a, K, V>; - fn into_iter(self) -> Self::IntoIter { - self.0.iter_mut() +/// Increases the lock count of the map for the lifetime of the guard. This should not be dropped until iteration has completed. +#[derive(Debug, Trace)] +pub(crate) struct MapLock(GcObject); + +impl Clone for MapLock { + fn clone(&self) -> Self { + let mut map = self.0.borrow_mut(); + let map = map.as_map_mut().expect("MapLock does not point to a map"); + map.lock(self.0.clone()) } } -impl IntoIterator for OrderedMap -where - K: Hash + Eq, - S: BuildHasher, -{ - type Item = (K, V); - type IntoIter = IntoIter; - fn into_iter(self) -> IntoIter { - self.0.into_iter() +impl Finalize for MapLock { + fn finalize(&self) { + let mut map = self.0.borrow_mut(); + let map = map.as_map_mut().expect("MapLock does not point to a map"); + map.unlock(); } } diff --git a/boa/src/builtins/map/tests.rs b/boa/src/builtins/map/tests.rs index 1e482b1744..eba98d24ca 100644 --- a/boa/src/builtins/map/tests.rs +++ b/boa/src/builtins/map/tests.rs @@ -354,3 +354,53 @@ fn not_a_function() { "\"TypeError: calling a builtin Map constructor without new is forbidden\"" ); } + +#[test] +fn for_each_delete() { + let mut context = Context::new(); + let init = r#" + let map = new Map([[0, "a"], [1, "b"], [2, "c"]]); + let result = []; + map.forEach(function(value, key) { + if (key === 0) { + map.delete(0); + map.set(3, "d"); + } + result.push([key, value]); + }) + "#; + forward(&mut context, init); + assert_eq!(forward(&mut context, "result[0][0]"), "0"); + assert_eq!(forward(&mut context, "result[0][1]"), "\"a\""); + assert_eq!(forward(&mut context, "result[1][0]"), "1"); + assert_eq!(forward(&mut context, "result[1][1]"), "\"b\""); + assert_eq!(forward(&mut context, "result[2][0]"), "2"); + assert_eq!(forward(&mut context, "result[2][1]"), "\"c\""); + assert_eq!(forward(&mut context, "result[3][0]"), "3"); + assert_eq!(forward(&mut context, "result[3][1]"), "\"d\""); +} + +#[test] +fn for_of_delete() { + let mut context = Context::new(); + let init = r#" + let map = new Map([[0, "a"], [1, "b"], [2, "c"]]); + let result = []; + for (a of map) { + if (a[0] === 0) { + map.delete(0); + map.set(3, "d"); + } + result.push([a[0], a[1]]); + } + "#; + forward(&mut context, init); + assert_eq!(forward(&mut context, "result[0][0]"), "0"); + assert_eq!(forward(&mut context, "result[0][1]"), "\"a\""); + assert_eq!(forward(&mut context, "result[1][0]"), "1"); + assert_eq!(forward(&mut context, "result[1][1]"), "\"b\""); + assert_eq!(forward(&mut context, "result[2][0]"), "2"); + assert_eq!(forward(&mut context, "result[2][1]"), "\"c\""); + assert_eq!(forward(&mut context, "result[3][0]"), "3"); + assert_eq!(forward(&mut context, "result[3][1]"), "\"d\""); +} diff --git a/boa/src/object/mod.rs b/boa/src/object/mod.rs index 88efd42b4e..140a1df917 100644 --- a/boa/src/object/mod.rs +++ b/boa/src/object/mod.rs @@ -83,7 +83,7 @@ pub struct Object { pub enum ObjectData { Array, ArrayIterator(ArrayIterator), - Map(OrderedMap), + Map(OrderedMap), MapIterator(MapIterator), RegExp(Box), RegExpStringIterator(RegExpStringIterator), @@ -354,7 +354,7 @@ impl Object { } #[inline] - pub fn as_map_ref(&self) -> Option<&OrderedMap> { + pub fn as_map_ref(&self) -> Option<&OrderedMap> { match self.data { ObjectData::Map(ref map) => Some(map), _ => None, @@ -362,7 +362,7 @@ impl Object { } #[inline] - pub fn as_map_mut(&mut self) -> Option<&mut OrderedMap> { + pub fn as_map_mut(&mut self) -> Option<&mut OrderedMap> { match &mut self.data { ObjectData::Map(map) => Some(map), _ => None,