From 4e8a61490eb7933ac6a726f9b7245ef0c22095eb Mon Sep 17 00:00:00 2001 From: yossydev Date: Thu, 6 Feb 2025 22:54:54 +0900 Subject: [PATCH 01/10] feat(ecmascript): WeakMap Constructor --- .../weak_map_objects/weak_map_constructor.rs | 294 +++++++++++++++++- .../weak_map_objects/weak_map_prototype.rs | 26 +- nova_vm/src/ecmascript/builtins/weak_map.rs | 11 + .../src/ecmascript/builtins/weak_map/data.rs | 277 +++++++++++++++-- 4 files changed, 572 insertions(+), 36 deletions(-) diff --git a/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_constructor.rs b/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_constructor.rs index e5e8b44b5..f7947c224 100644 --- a/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_constructor.rs +++ b/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_constructor.rs @@ -2,7 +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/. +use std::hash::Hasher; + +use ahash::AHasher; + +use crate::ecmascript::abstract_operations::operations_on_iterator_objects::{ + get_iterator, if_abrupt_close_iterator, iterator_close, iterator_step_value, +}; +use crate::ecmascript::abstract_operations::operations_on_objects::{ + call_function, get, get_method, try_get, +}; +use crate::ecmascript::abstract_operations::testing_and_comparison::is_callable; +use crate::ecmascript::builtins::array::ArrayHeap; +use crate::ecmascript::builtins::keyed_collections::weak_map_objects::weak_map_prototype::canonicalize_keyed_collection_key; +use crate::ecmascript::builtins::ordinary::ordinary_create_from_constructor; +use crate::ecmascript::builtins::weak_map::data::WeakMapData; +use crate::ecmascript::builtins::weak_map::WeakMap; +use crate::ecmascript::execution::agent::ExceptionType; +use crate::ecmascript::execution::ProtoIntrinsics; +use crate::ecmascript::types::{Function, IntoFunction, IntoValue}; use crate::engine::context::GcScope; +use crate::engine::TryResult; +use crate::heap::{Heap, PrimitiveHeap, WellKnownSymbolIndexes}; use crate::{ ecmascript::{ builders::builtin_function_builder::BuiltinFunctionBuilder, @@ -13,6 +34,8 @@ use crate::{ heap::IntrinsicConstructorIndexes, }; +use super::weak_map_prototype::WeakMapPrototypeSet; + pub(crate) struct WeakMapConstructor; impl Builtin for WeakMapConstructor { const NAME: String<'static> = BUILTIN_STRING_MEMORY.WeakMap; @@ -21,19 +44,89 @@ impl Builtin for WeakMapConstructor { const BEHAVIOUR: Behaviour = Behaviour::Constructor(Self::constructor); } + impl BuiltinIntrinsicConstructor for WeakMapConstructor { const INDEX: IntrinsicConstructorIndexes = IntrinsicConstructorIndexes::WeakMap; } impl WeakMapConstructor { fn constructor( - _agent: &mut Agent, - _this_value: Value, - _arguments: ArgumentsList, - _new_target: Option, - _gc: GcScope, + agent: &mut Agent, + _: Value, + arguments: ArgumentsList, + new_target: Option, + mut gc: GcScope, ) -> JsResult { - todo!() + // If NewTarget is undefined, throw a TypeError exception. + let Some(new_target) = new_target else { + return Err(agent.throw_exception_with_static_message( + ExceptionType::TypeError, + "Constructor WeakMap requires 'new'", + gc.nogc(), + )); + }; + let new_target = Function::try_from(new_target).unwrap(); + // 2. Let map be ? OrdinaryCreateFromConstructor(NewTarget, "%WeakMap.prototype%", « [[WeakMapData]] »). + let mut map = WeakMap::try_from(ordinary_create_from_constructor( + agent, + new_target, + ProtoIntrinsics::WeakMap, + gc.reborrow(), + )?) + .unwrap() + .unbind() + .bind(gc.nogc()); + // 3. Set map.[[WeakMapData]] to a new empty List. + let iterable = arguments.get(0); + // 4. If iterable is either undefined or null, return map. + if iterable.is_undefined() || iterable.is_null() { + Ok(map.into_value()) + } else { + // Note + // If the parameter iterable is present, it is expected to be an + // object that implements an @@iterator method that returns an + // iterator object that produces a two element array-like object + // whose first element is a value that will be used as a WeakMap key + // and whose second element is the value to associate with that + // key. + + // 5. Let adder be ? Get(map, "set"). + let adder = if let TryResult::Continue(adder) = try_get( + agent, + map.into_object().unbind(), + BUILTIN_STRING_MEMORY.set.to_property_key(), + gc.nogc(), + ) { + adder + } else { + let scoped_map = map.scope(agent, gc.nogc()); + let adder = get( + agent, + map.into_object().unbind(), + BUILTIN_STRING_MEMORY.set.to_property_key(), + gc.reborrow(), + )?; + map = scoped_map.get(agent).bind(gc.nogc()); + adder + }; + // 6. If IsCallable(adder) is false, throw a TypeError exception. + let Some(adder) = is_callable(adder, gc.nogc()) else { + return Err(agent.throw_exception_with_static_message( + ExceptionType::TypeError, + "WeakMap.prototype.set is not callable", + gc.nogc(), + )); + }; + // 7. Return ? AddEntriesFromIterable(map, iterable, adder). + add_entries_from_iterable_weak_map_constructor( + agent, + map.unbind(), + iterable, + adder.unbind(), + gc.reborrow(), + ) + .map(|result| result.into_value()) + } } pub(crate) fn create_intrinsic(agent: &mut Agent, realm: RealmIdentifier) { @@ -46,3 +139,192 @@ impl WeakMapConstructor { .build(); } } + +/// ### [24.1.1.2 AddEntriesFromIterable ( target, iterable, adder )](https://tc39.es/ecma262/#sec-add-entries-from-iterable) +/// +/// #### Unspecified specialization +/// +/// This is a specialization for the `new WeakMap()` use case. +pub fn add_entries_from_iterable_weak_map_constructor<'a>( + agent: &mut Agent, + target: WeakMap, + iterable: Value, + adder: Function, + mut gc: GcScope<'a, '_>, +) -> JsResult> { + let mut target = target.bind(gc.nogc()); + let mut adder = adder.bind(gc.nogc()); + if let Function::BuiltinFunction(bf) = adder { + if agent[bf].behaviour == WeakMapPrototypeSet::BEHAVIOUR { + // Normal WeakMap.prototype.set + if let Value::Array(iterable) = iterable { + let scoped_adder = bf.scope(agent, gc.nogc()); + let scoped_target = target.scope(agent, gc.nogc()); + let using_iterator = get_method( + agent, + iterable.into_value(), + WellKnownSymbolIndexes::Iterator.into(), + gc.reborrow(), + )? + .map(|f| f.unbind()) + .map(|f| f.bind(gc.nogc())); + target = scoped_target.get(agent).bind(gc.nogc()); + if using_iterator + == Some( + agent + .current_realm() + .intrinsics() + .array_prototype_values() + .into_function(), + ) + { + let Heap { + elements, + arrays, + bigints, + numbers, + strings, + weak_maps, + .. + } = &mut agent.heap; + let array_heap = ArrayHeap::new(elements, arrays); + let primitive_heap = PrimitiveHeap::new(bigints, numbers, strings); + + // Iterable uses the normal Array iterator of this realm. + if iterable.len(&array_heap) == 0 { + // Array iterator does not iterate empty arrays. + return Ok(scoped_target.get(agent).bind(gc.into_nogc())); + } + if iterable.is_trivial(&array_heap) + && iterable.as_slice(&array_heap).iter().all(|entry| { + if let Some(Value::Array(entry)) = *entry { + entry.len(&array_heap) == 2 + && entry.is_trivial(&array_heap) + && entry.is_dense(&array_heap) + } else { + false + } + }) + { + // Trivial, dense array of trivial, dense arrays of two elements. + let length = iterable.len(&array_heap); + let WeakMapData { + keys, + values, + weak_map_data, + .. + } = weak_maps[target].borrow_mut(&primitive_heap); + let map_data = weak_map_data.get_mut(); + + let length = length as usize; + keys.reserve(length); + values.reserve(length); + // Note: The WeakMap is empty at this point, we don't need the hasher function. + assert!(map_data.is_empty()); + map_data.reserve(length, |_| 0); + let hasher = |value: Value| { + let mut hasher = AHasher::default(); + value.hash(&primitive_heap, &mut hasher); + hasher.finish() + }; + for entry in iterable.as_slice(&array_heap).iter() { + let Some(Value::Array(entry)) = *entry else { + unreachable!() + }; + let slice = entry.as_slice(&array_heap); + let key = canonicalize_keyed_collection_key(numbers, slice[0].unwrap()); + let key_hash = hasher(key); + let value = slice[1].unwrap(); + let next_index = keys.len() as u32; + let entry = map_data.entry( + key_hash, + |hash_equal_index| keys[*hash_equal_index as usize].unwrap() == key, + |index_to_hash| hasher(keys[*index_to_hash as usize].unwrap()), + ); + match entry { + hashbrown::hash_table::Entry::Occupied(occupied) => { + // We have duplicates in the array. Latter + // ones overwrite earlier ones. + let index = *occupied.get(); + values[index as usize] = Some(value); + } + hashbrown::hash_table::Entry::Vacant(vacant) => { + vacant.insert(next_index); + keys.push(Some(key)); + values.push(Some(value)); + } + } + } + return Ok(scoped_target.get(agent).bind(gc.into_nogc())); + } + } + adder = scoped_adder.get(agent).bind(gc.nogc()).into_function(); + } + } + } + + add_entries_from_iterable(agent, target.unbind(), iterable, adder.unbind(), gc) +} + +/// ### [24.1.1.2 AddEntriesFromIterable ( target, iterable, adder )](https://tc39.es/ecma262/#sec-add-entries-from-iterable) +/// +/// The abstract operation AddEntriesFromIterable takes arguments target (an +/// Object), iterable (an ECMAScript language value, but not undefined or +/// null), and adder (a function object) and returns either a normal completion +/// containing an ECMAScript language value or a throw completion. adder will +/// be invoked, with target as the receiver. +/// +/// > NOTE: The parameter iterable is expected to be an object that implements +/// > an @@iterator method that returns an iterator object that produces a two +/// > element array-like object whose first element is a value that will be used +/// > as a WeakMap key and whose second element is the value to associate with that +/// > key. +pub(crate) fn add_entries_from_iterable<'a>( + agent: &mut Agent, + target: WeakMap, + iterable: Value, + adder: Function, + mut gc: GcScope<'a, '_>, +) -> JsResult> { + let target = target.bind(gc.nogc()).scope(agent, gc.nogc()); + let adder = adder.bind(gc.nogc()).scope(agent, gc.nogc()); + // 1. Let iteratorRecord be ? GetIterator(iterable, SYNC). + let mut iterator_record = get_iterator(agent, iterable, false, gc.reborrow())?; + // 2. Repeat, + loop { + // a. Let next be ? IteratorStepValue(iteratorRecord). + let next = iterator_step_value(agent, &mut iterator_record, gc.reborrow())?; + // b. If next is DONE, return target. + let Some(next) = next else { + return Ok(target.get(agent).bind(gc.into_nogc())); + }; + // c. If next is not an Object, then + let Ok(next) = Object::try_from(next) else { + // i. Let error be ThrowCompletion(a newly created TypeError object). + let error = agent.throw_exception_with_static_message( + ExceptionType::TypeError, + "Invalid iterator next return value", + gc.nogc(), + ); + // ii. Return ? IteratorClose(iteratorRecord, error). + return iterator_close(agent, &iterator_record, Err(error), gc.reborrow()); + }; + // d. Let k be Completion(Get(next, "0")). + let k = get(agent, next, 0.into(), gc.reborrow()); + // e. IfAbruptCloseIterator(k, iteratorRecord). + let k = if_abrupt_close_iterator(agent, k, &iterator_record, gc.reborrow())?; + // f. Let v be Completion(Get(next, "1")). + let v = get(agent, next, 1.into(), gc.reborrow()); + // g. IfAbruptCloseIterator(v, iteratorRecord). + let v = if_abrupt_close_iterator(agent, v, &iterator_record, gc.reborrow())?; + // h. Let status be Completion(Call(adder, target, « k, v »)). + let status = call_function( + agent, + adder.get(agent), + target.get(agent).into_value(), + Some(ArgumentsList(&[k, v])), + gc.reborrow(), + ); + let _ = if_abrupt_close_iterator(agent, status, &iterator_record, gc.reborrow())?; + } +} diff --git a/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_prototype.rs b/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_prototype.rs index 4699205ef..086416bb4 100644 --- a/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_prototype.rs +++ b/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_prototype.rs @@ -2,6 +2,9 @@ // 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/. +use std::ops::Index; + +use crate::ecmascript::types::HeapNumber; use crate::engine::context::GcScope; use crate::{ ecmascript::{ @@ -33,7 +36,7 @@ impl Builtin for WeakMapPrototypeHas { const LENGTH: u8 = 1; const BEHAVIOUR: Behaviour = Behaviour::Regular(WeakMapPrototype::has); } -struct WeakMapPrototypeSet; +pub(super) struct WeakMapPrototypeSet; impl Builtin for WeakMapPrototypeSet { const NAME: String<'static> = BUILTIN_STRING_MEMORY.set; const LENGTH: u8 = 2; @@ -102,3 +105,24 @@ impl WeakMapPrototype { .build(); } } + +#[inline(always)] +/// ### [24.5.1 CanonicalizeKeyedCollectionKey ( key )](https://tc39.es/ecma262/#sec-canonicalizekeyedcollectionkey) +/// The abstract operation CanonicalizeKeyedCollectionKey takes argument key +/// (an ECMAScript language value) and returns an ECMAScript language value. +pub(crate) fn canonicalize_keyed_collection_key( + agent: &impl Index, Output = f64>, + key: Value, +) -> Value { + // 1. If key is -0𝔽, return +0𝔽. + if let Value::SmallF64(key) = key { + // Note: Only f32 should hold -0. + if key.into_f64() == -0.0 { + return 0.into(); + } + } else if let Value::Number(key) = key { + debug_assert_ne!(agent[key], -0.0, "HeapNumber should never be -0.0"); + } + // 2. Return key. + key +} diff --git a/nova_vm/src/ecmascript/builtins/weak_map.rs b/nova_vm/src/ecmascript/builtins/weak_map.rs index c91571216..e6ccab0af 100644 --- a/nova_vm/src/ecmascript/builtins/weak_map.rs +++ b/nova_vm/src/ecmascript/builtins/weak_map.rs @@ -89,6 +89,17 @@ impl<'a> From> for Object<'a> { } } +impl<'a> TryFrom> for WeakMap<'a> { + type Error = (); + + fn try_from(value: Object<'a>) -> Result { + match value { + Object::WeakMap(data) => Ok(data), + _ => Err(()), + } + } +} + impl<'a> InternalSlots<'a> for WeakMap<'a> { const DEFAULT_PROTOTYPE: ProtoIntrinsics = ProtoIntrinsics::WeakMap; diff --git a/nova_vm/src/ecmascript/builtins/weak_map/data.rs b/nova_vm/src/ecmascript/builtins/weak_map/data.rs index dc56f9d12..8c8251cb8 100644 --- a/nova_vm/src/ecmascript/builtins/weak_map/data.rs +++ b/nova_vm/src/ecmascript/builtins/weak_map/data.rs @@ -3,56 +3,275 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use crate::{ - ecmascript::types::{OrdinaryObject, Value}, - heap::{CompactionLists, HeapMarkAndSweep, WorkQueues}, + ecmascript::types::{ + bigint::HeapBigInt, HeapNumber, HeapPrimitive, HeapString, OrdinaryObject, Value, + BIGINT_DISCRIMINANT, NUMBER_DISCRIMINANT, STRING_DISCRIMINANT, + }, + heap::{CompactionLists, HeapMarkAndSweep, PrimitiveHeapIndexable, WorkQueues}, +}; +use ahash::AHasher; +use hashbrown::{hash_table::Entry, HashTable}; +use std::{ + cell::RefCell, + hash::{Hash, Hasher}, + sync::atomic::{AtomicBool, Ordering}, }; -#[derive(Debug, Clone, Default)] +#[derive(Debug, Default)] pub struct WeakMapHeapData { pub(crate) object_index: Option>, - // TODO: This isn't even close to a hashmap; HashMap won't allow inserting - // Value as key; f32 isn't hashable. And our f64s are found on the Heap and - // require fetching; What we actually should do is more like: - // pub(crate) map: HashMap + weak_map_data: WeakMapData, +} + +#[derive(Debug, Default)] +pub(crate) struct WeakMapData { + // TODO: Use a ParallelVec to remove one unnecessary allocation. // pub(crate) key_values: ParallelVec, Option> - // ValueHash is created using a Value.hash(agent) function and connects to - // an index; the index points to a key and value in parallel vector / Vec2. - // Note that empty slots are deleted values in the ParallelVec. - pub(crate) keys: Vec, - pub(crate) values: Vec, - // TODO: When an non-terminal (start or end) iterator exists for the Map, - // the items in the map cannot be compacted. - // pub(crate) observed: bool; + pub(crate) keys: Vec>, + pub(crate) values: Vec>, + /// Low-level hash table pointing to keys-values indexes. + pub(crate) weak_map_data: RefCell>, + pub(crate) needs_primitive_rehashing: AtomicBool, +} + +impl WeakMapHeapData { + /// ### [24.2.1.5 WeakMapDataSize ( setData )](https://tc39.es/ecma262/#sec-setdatasize) + /// + /// The abstract operation MapDataSize takes argument setData (a List of either + /// ECMAScript language values or EMPTY) and returns a non-negative integer. + #[inline(always)] + pub fn size(&self) -> u32 { + // 1. Let count be 0. + // 2. For each element e of setData, do + // a. If e is not EMPTY, set count to count + 1. + // 3. Return count. + self.weak_map_data.weak_map_data.borrow().len() as u32 + } + + pub fn keys(&self) -> &[Option] { + &self.weak_map_data.keys + } + + pub fn values(&self) -> &[Option] { + &self.weak_map_data.values + } + + pub fn clear(&mut self) { + // 3. For each Record { [[Key]], [[Value]] } p of M.[[MapData]], do + // a. Set p.[[Key]] to EMPTY. + // b. Set p.[[Value]] to EMPTY. + self.weak_map_data.weak_map_data.get_mut().clear(); + self.weak_map_data.values.fill(None); + self.weak_map_data.keys.fill(None); + } + + pub(crate) fn borrow(&self, arena: &impl PrimitiveHeapIndexable) -> &WeakMapData { + self.weak_map_data.rehash_if_needed(arena); + &self.weak_map_data + } + + pub(crate) fn borrow_mut(&mut self, arena: &impl PrimitiveHeapIndexable) -> &mut WeakMapData { + self.weak_map_data.rehash_if_needed_mut(arena); + &mut self.weak_map_data + } +} + +impl WeakMapData { + fn rehash_if_needed_mut(&mut self, arena: &impl PrimitiveHeapIndexable) { + if !*self.needs_primitive_rehashing.get_mut() { + return; + } + + rehash_map_data(&self.keys, self.weak_map_data.get_mut(), arena); + self.needs_primitive_rehashing + .store(false, Ordering::Relaxed); + } + + fn rehash_if_needed(&self, arena: &impl PrimitiveHeapIndexable) { + if !self.needs_primitive_rehashing.load(Ordering::Relaxed) { + return; + } + + rehash_map_data(&self.keys, &mut self.weak_map_data.borrow_mut(), arena); + self.needs_primitive_rehashing + .store(false, Ordering::Relaxed); + } +} + +fn rehash_map_data( + keys: &[Option], + map_data: &mut HashTable, + arena: &impl PrimitiveHeapIndexable, +) { + let hasher = |value: Value| { + let mut hasher = AHasher::default(); + value.hash(arena, &mut hasher); + hasher.finish() + }; + let hashes = { + let hasher = |discriminant: u8| { + let mut hasher = AHasher::default(); + discriminant.hash(&mut hasher); + hasher.finish() + }; + [ + (0u8, hasher(STRING_DISCRIMINANT)), + (1u8, hasher(NUMBER_DISCRIMINANT)), + (2u8, hasher(BIGINT_DISCRIMINANT)), + ] + }; + for (id, hash) in hashes { + let eq = |equal_hash_index: &u32| { + let value = keys[*equal_hash_index as usize].unwrap(); + match id { + 0 => HeapString::try_from(value).is_ok(), + 1 => HeapNumber::try_from(value).is_ok(), + 2 => HeapBigInt::try_from(value).is_ok(), + _ => unreachable!(), + } + }; + let mut entries = Vec::new(); + while let Ok(entry) = map_data.find_entry(hash, eq) { + entries.push(*entry.get()); + entry.remove(); + } + entries.iter().for_each(|entry| { + let key = keys[*entry as usize].unwrap(); + let key_hash = hasher(key); + let result = map_data.entry( + key_hash, + |equal_hash_index| { + // It should not be possible for there to be an equal item + // in the WeakMap already. + debug_assert_ne!(keys[*equal_hash_index as usize].unwrap(), key); + false + }, + |index_to_hash| hasher(keys[*index_to_hash as usize].unwrap()), + ); + + let Entry::Vacant(result) = result else { + unreachable!(); + }; + result.insert(*entry); + }); + } } impl HeapMarkAndSweep for WeakMapHeapData { fn mark_values(&self, queues: &mut WorkQueues) { let Self { object_index, - keys, - values, + weak_map_data, } = self; object_index.mark_values(queues); - for ele in keys { - ele.mark_values(queues); - } - for ele in values { - ele.mark_values(queues); - } + weak_map_data + .keys + .iter() + .for_each(|value| value.mark_values(queues)); + weak_map_data + .values + .iter() + .for_each(|value| value.mark_values(queues)); } fn sweep_values(&mut self, compactions: &CompactionLists) { let Self { object_index, + weak_map_data, + } = self; + let WeakMapData { keys, values, - } = self; + needs_primitive_rehashing, + weak_map_data, + } = weak_map_data; + let weak_map_data = weak_map_data.get_mut(); object_index.sweep_values(compactions); - for ele in keys { - ele.sweep_values(compactions); - } - for ele in values { - ele.sweep_values(compactions); + + let hasher = |value: Value| { + let mut hasher = AHasher::default(); + if value.try_hash(&mut hasher).is_err() { + // A heap String, Number, or BigInt required rehashing as part + // of moving an Object key inside the HashTable. The heap + // vectors for those data points are currently being sweeped on + // another thread, so we cannot access them right now even if + // we wanted to. This situation should be fairly improbable as + // it requires mixing eg. long string and object values in the + // same WeakMap, so it's not pressing right now. Still, it must be + // handled eventually. We essentially mark the heap hash data + // as "dirty". Any lookup shall then later check this boolean + // and rehash primitive keys if necessary. + needs_primitive_rehashing.store(true, Ordering::Relaxed); + let value = HeapPrimitive::try_from(value).unwrap(); + // Return a hash based on the discriminant. This we are likely + // to cause hash collisions but we avoid having to rehash all + // keys; we can just rehash the primitive keys that match the + // discriminant hash. + core::mem::discriminant(&value).hash(&mut hasher); + } + hasher.finish() + }; + assert_eq!(keys.len(), values.len()); + for index in 0..keys.len() as u32 { + let key = &mut keys[index as usize]; + let Some(key) = key else { + // Skip empty slots. + continue; + }; + // Sweep value without any concerns. + values[index as usize].sweep_values(compactions); + + let old_key = *key; + key.sweep_values(compactions); + let new_key = *key; + + if old_key == new_key { + // No identity change, no hash change. + continue; + } + + if !new_key.is_object() { + // Non-objects do not change their hash even if their identity + // changes. + continue; + } + // Object changed identity; it must be moved in the map_data. + let old_hash = hasher(old_key); + let new_hash = hasher(new_key); + if let Ok(old_entry) = + weak_map_data.find_entry(old_hash, |equal_hash_index| *equal_hash_index == index) + { + // We do not always find an entry if a previous item has + // shifted ontop of our old hash. + old_entry.remove(); + } + let new_entry = weak_map_data.entry( + new_hash, + |equal_hash_index| { + // It's not possible for there to be another hash index that + // holds our item. But! It's possible that we're eg. shifting + // 32 to 30, and then 30 to 29. Thus 32 will happily override + // 30. + values[*equal_hash_index as usize].unwrap() == new_key + }, + |index_to_hash| { + let value = values[*index_to_hash as usize].unwrap(); + hasher(value) + }, + ); + match new_entry { + hashbrown::hash_table::Entry::Occupied(mut occupied) => { + // We found an existing entry that points to a + // (necesssarily) different slot that contains the same + // value. This value will necessarily be removed later; we + // can just reuse this slot. + *occupied.get_mut() = index; + } + hashbrown::hash_table::Entry::Vacant(vacant) => { + // This is the expected case: We're not overwriting a slot. + vacant.insert(index); + } + } } } } From 04b7f6b9a73ac6783d4b40a0e1c2d88c17e92951 Mon Sep 17 00:00:00 2001 From: yossydev Date: Thu, 6 Feb 2025 22:55:07 +0900 Subject: [PATCH 02/10] chore(ecmascript): update metrics --- tests/metrics.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/metrics.json b/tests/metrics.json index 501f92996..09f8834fc 100644 --- a/tests/metrics.json +++ b/tests/metrics.json @@ -1,8 +1,8 @@ { "results": { - "crash": 13145, - "fail": 9052, - "pass": 24539, + "crash": 13142, + "fail": 9060, + "pass": 24534, "skip": 65, "timeout": 0, "unresolved": 0 From 0d611a9a0bce91dab6c9f6f9acd6fabf8d3b25b7 Mon Sep 17 00:00:00 2001 From: yossydev Date: Thu, 6 Feb 2025 22:55:24 +0900 Subject: [PATCH 03/10] chore(ecmascript): update expectations --- tests/expectations.json | 35 +---------------------------------- 1 file changed, 1 insertion(+), 34 deletions(-) diff --git a/tests/expectations.json b/tests/expectations.json index d308ce396..edf1f30a4 100644 --- a/tests/expectations.json +++ b/tests/expectations.json @@ -2039,16 +2039,6 @@ "built-ins/Map/groupBy/string.js": "CRASH", "built-ins/Map/groupBy/toPropertyKey.js": "CRASH", "built-ins/Map/proto-from-ctor-realm.js": "FAIL", - "built-ins/Map/prototype/clear/context-is-weakmap-object-throws.js": "CRASH", - "built-ins/Map/prototype/delete/context-is-weakmap-object-throws.js": "CRASH", - "built-ins/Map/prototype/entries/does-not-have-mapdata-internal-slot-weakmap.js": "CRASH", - "built-ins/Map/prototype/forEach/does-not-have-mapdata-internal-slot-weakmap.js": "CRASH", - "built-ins/Map/prototype/get/does-not-have-mapdata-internal-slot-weakmap.js": "CRASH", - "built-ins/Map/prototype/has/does-not-have-mapdata-internal-slot-weakmap.js": "CRASH", - "built-ins/Map/prototype/keys/does-not-have-mapdata-internal-slot-weakmap.js": "CRASH", - "built-ins/Map/prototype/set/does-not-have-mapdata-internal-slot-weakmap.js": "CRASH", - "built-ins/Map/prototype/size/does-not-have-mapdata-internal-slot-weakmap.js": "CRASH", - "built-ins/Map/prototype/values/does-not-have-mapdata-internal-slot-weakmap.js": "CRASH", "built-ins/Map/valid-keys.js": "CRASH", "built-ins/Math/f16round/length.js": "FAIL", "built-ins/Math/f16round/name.js": "FAIL", @@ -2487,7 +2477,6 @@ "built-ins/Object/prototype/toString/symbol-tag-override-primitives.js": "FAIL", "built-ins/Object/prototype/toString/symbol-tag-set-builtin.js": "CRASH", "built-ins/Object/prototype/toString/symbol-tag-string-builtin.js": "CRASH", - "built-ins/Object/prototype/toString/symbol-tag-weakmap-builtin.js": "CRASH", "built-ins/Object/prototype/toString/symbol-tag-weakset-builtin.js": "CRASH", "built-ins/Object/prototype/valueOf/S15.2.4.4_A14.js": "CRASH", "built-ins/Object/seal/object-seal-o-is-a-date-object.js": "CRASH", @@ -2498,7 +2487,6 @@ "built-ins/Object/seal/seal-finalizationregistry.js": "CRASH", "built-ins/Object/seal/seal-regexp.js": "CRASH", "built-ins/Object/seal/seal-sharedarraybuffer.js": "CRASH", - "built-ins/Object/seal/seal-weakmap.js": "CRASH", "built-ins/Object/seal/seal-weakref.js": "CRASH", "built-ins/Object/seal/seal-weakset.js": "CRASH", "built-ins/Promise/all/S25.4.4.1_A2.1_T1.js": "CRASH", @@ -9569,6 +9557,7 @@ "built-ins/TypedArray/of/not-a-constructor.js": "FAIL", "built-ins/TypedArray/of/prop-desc.js": "FAIL", "built-ins/TypedArray/of/resized-with-out-of-bounds-and-in-bounds-indices.js": "CRASH", + "built-ins/TypedArray/out-of-bounds-get-and-set.js": "CRASH", "built-ins/TypedArray/prototype/byteLength/resized-out-of-bounds-1.js": "CRASH", "built-ins/TypedArray/prototype/byteLength/resized-out-of-bounds-2.js": "CRASH", "built-ins/TypedArray/prototype/byteOffset/resized-out-of-bounds.js": "CRASH", @@ -10801,23 +10790,10 @@ "built-ins/Uint8Array/prototype/toHex/name.js": "FAIL", "built-ins/Uint8Array/prototype/toHex/nonconstructor.js": "FAIL", "built-ins/Uint8Array/prototype/toHex/results.js": "CRASH", - "built-ins/WeakMap/empty-iterable.js": "CRASH", - "built-ins/WeakMap/get-set-method-failure.js": "CRASH", - "built-ins/WeakMap/is-a-constructor.js": "CRASH", - "built-ins/WeakMap/iterable-failure.js": "CRASH", "built-ins/WeakMap/iterable-with-object-keys.js": "CRASH", "built-ins/WeakMap/iterable-with-symbol-keys.js": "CRASH", - "built-ins/WeakMap/iterator-close-after-set-failure.js": "CRASH", - "built-ins/WeakMap/iterator-item-first-entry-returns-abrupt.js": "CRASH", - "built-ins/WeakMap/iterator-item-second-entry-returns-abrupt.js": "CRASH", - "built-ins/WeakMap/iterator-items-are-not-object-close-iterator.js": "CRASH", "built-ins/WeakMap/iterator-items-keys-cannot-be-held-weakly.js": "CRASH", - "built-ins/WeakMap/iterator-next-failure.js": "CRASH", - "built-ins/WeakMap/iterator-value-failure.js": "CRASH", - "built-ins/WeakMap/no-iterable.js": "CRASH", - "built-ins/WeakMap/properties-of-map-instances.js": "CRASH", "built-ins/WeakMap/proto-from-ctor-realm.js": "FAIL", - "built-ins/WeakMap/prototype/constructor.js": "CRASH", "built-ins/WeakMap/prototype/delete/delete-entry-with-object-key-initial-iterable.js": "CRASH", "built-ins/WeakMap/prototype/delete/delete-entry-with-object-key.js": "CRASH", "built-ins/WeakMap/prototype/delete/delete-entry-with-symbol-key-initial-iterable.js": "CRASH", @@ -10827,7 +10803,6 @@ "built-ins/WeakMap/prototype/delete/does-not-have-weakmapdata-internal-slot-object.js": "CRASH", "built-ins/WeakMap/prototype/delete/does-not-have-weakmapdata-internal-slot-set.js": "CRASH", "built-ins/WeakMap/prototype/delete/does-not-have-weakmapdata-internal-slot-weakmap-prototype.js": "CRASH", - "built-ins/WeakMap/prototype/delete/not-a-constructor.js": "CRASH", "built-ins/WeakMap/prototype/delete/returns-false-if-key-cannot-be-held-weakly.js": "CRASH", "built-ins/WeakMap/prototype/delete/returns-false-when-object-key-not-present.js": "CRASH", "built-ins/WeakMap/prototype/delete/returns-false-when-symbol-key-not-present.js": "CRASH", @@ -10840,7 +10815,6 @@ "built-ins/WeakMap/prototype/get/does-not-have-weakmapdata-internal-slot-map.js": "CRASH", "built-ins/WeakMap/prototype/get/does-not-have-weakmapdata-internal-slot-set.js": "CRASH", "built-ins/WeakMap/prototype/get/does-not-have-weakmapdata-internal-slot.js": "CRASH", - "built-ins/WeakMap/prototype/get/not-a-constructor.js": "CRASH", "built-ins/WeakMap/prototype/get/returns-undefined-if-key-cannot-be-held-weakly.js": "CRASH", "built-ins/WeakMap/prototype/get/returns-undefined-with-object-key.js": "CRASH", "built-ins/WeakMap/prototype/get/returns-undefined-with-symbol-key.js": "CRASH", @@ -10852,7 +10826,6 @@ "built-ins/WeakMap/prototype/has/does-not-have-weakmapdata-internal-slot-object.js": "CRASH", "built-ins/WeakMap/prototype/has/does-not-have-weakmapdata-internal-slot-set.js": "CRASH", "built-ins/WeakMap/prototype/has/does-not-have-weakmapdata-internal-slot-weakmap-prototype.js": "CRASH", - "built-ins/WeakMap/prototype/has/not-a-constructor.js": "CRASH", "built-ins/WeakMap/prototype/has/returns-false-when-key-cannot-be-held-weakly.js": "CRASH", "built-ins/WeakMap/prototype/has/returns-false-when-object-key-not-present.js": "CRASH", "built-ins/WeakMap/prototype/has/returns-false-when-symbol-key-not-present.js": "CRASH", @@ -10871,7 +10844,6 @@ "built-ins/WeakMap/prototype/set/does-not-have-weakmapdata-internal-slot-object.js": "CRASH", "built-ins/WeakMap/prototype/set/does-not-have-weakmapdata-internal-slot-set.js": "CRASH", "built-ins/WeakMap/prototype/set/does-not-have-weakmapdata-internal-slot-weakmap-prototype.js": "CRASH", - "built-ins/WeakMap/prototype/set/not-a-constructor.js": "CRASH", "built-ins/WeakMap/prototype/set/returns-this-when-ignoring-duplicate.js": "CRASH", "built-ins/WeakMap/prototype/set/returns-this.js": "CRASH", "built-ins/WeakMap/prototype/set/this-not-object-throw-boolean.js": "CRASH", @@ -10881,8 +10853,6 @@ "built-ins/WeakMap/prototype/set/this-not-object-throw-symbol.js": "CRASH", "built-ins/WeakMap/prototype/set/this-not-object-throw-undefined.js": "CRASH", "built-ins/WeakMap/prototype/set/throw-if-key-cannot-be-held-weakly.js": "CRASH", - "built-ins/WeakMap/set-not-callable-throws.js": "CRASH", - "built-ins/WeakMap/undefined-newtarget.js": "CRASH", "built-ins/WeakRef/instance-extensible.js": "CRASH", "built-ins/WeakRef/is-a-constructor.js": "CRASH", "built-ins/WeakRef/newtarget-prototype-is-not-object.js": "CRASH", @@ -14795,7 +14765,6 @@ "language/expressions/class/subclass-builtins/subclass-Promise.js": "CRASH", "language/expressions/class/subclass-builtins/subclass-RegExp.js": "CRASH", "language/expressions/class/subclass-builtins/subclass-SharedArrayBuffer.js": "CRASH", - "language/expressions/class/subclass-builtins/subclass-WeakMap.js": "CRASH", "language/expressions/class/subclass-builtins/subclass-WeakRef.js": "CRASH", "language/expressions/class/subclass-builtins/subclass-WeakSet.js": "CRASH", "language/expressions/coalesce/tco-pos-null.js": "CRASH", @@ -19143,7 +19112,6 @@ "language/statements/class/subclass-builtins/subclass-Promise.js": "CRASH", "language/statements/class/subclass-builtins/subclass-RegExp.js": "CRASH", "language/statements/class/subclass-builtins/subclass-SharedArrayBuffer.js": "CRASH", - "language/statements/class/subclass-builtins/subclass-WeakMap.js": "CRASH", "language/statements/class/subclass-builtins/subclass-WeakRef.js": "CRASH", "language/statements/class/subclass-builtins/subclass-WeakSet.js": "CRASH", "language/statements/class/subclass/builtin-objects/ArrayBuffer/regular-subclassing.js": "CRASH", @@ -19167,7 +19135,6 @@ "language/statements/class/subclass/builtin-objects/RegExp/regular-subclassing.js": "CRASH", "language/statements/class/subclass/builtin-objects/RegExp/super-must-be-called.js": "CRASH", "language/statements/class/subclass/builtin-objects/WeakMap/regular-subclassing.js": "CRASH", - "language/statements/class/subclass/builtin-objects/WeakMap/super-must-be-called.js": "CRASH", "language/statements/class/subclass/builtin-objects/WeakSet/regular-subclassing.js": "CRASH", "language/statements/class/subclass/builtin-objects/WeakSet/super-must-be-called.js": "CRASH", "language/statements/class/subclass/derived-class-return-override-catch-finally-arrow.js": "CRASH", From 594bc6d106c2494fcee4a10688bd4dca3ee35165 Mon Sep 17 00:00:00 2001 From: yossydev Date: Thu, 6 Feb 2025 23:16:21 +0900 Subject: [PATCH 04/10] chore(ecmascript): update skip --- tests/metrics.json | 4 ++-- tests/skip.json | 10 +++++++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/metrics.json b/tests/metrics.json index 09f8834fc..e6f4e47d4 100644 --- a/tests/metrics.json +++ b/tests/metrics.json @@ -2,8 +2,8 @@ "results": { "crash": 13142, "fail": 9060, - "pass": 24534, - "skip": 65, + "pass": 24526, + "skip": 73, "timeout": 0, "unresolved": 0 }, diff --git a/tests/skip.json b/tests/skip.json index 835463072..d3c357cb9 100644 --- a/tests/skip.json +++ b/tests/skip.json @@ -18,6 +18,11 @@ "built-ins/Array/prototype/lastIndexOf/15.4.4.15-8-9.js", "built-ins/Array/prototype/push/S15.4.4.7_A3.js", "built-ins/Array/prototype/reduceRight/length-near-integer-limit.js", + "built-ins/Array/prototype/fill/resizable-buffer.js", + "built-ins/Array/prototype/slice/coerced-start-end-grow.js", + "built-ins/Array/prototype/sort/comparefn-grow.js", + "built-ins/Array/prototype/at/typed-array-resizable-buffer.js", + "built-ins/Array/prototype/entries/resizable-buffer-grow-mid-iteration.js", "built-ins/Array/S15.4.5.2_A1_T1.js", "built-ins/Array/S15.4.5.2_A3_T3.js", "built-ins/Array/S15.4_A1.1_T10.js", @@ -28,6 +33,7 @@ "built-ins/Object/defineProperty/15.2.3.6-4-154.js", "built-ins/Object/defineProperty/15.2.3.6-4-155.js", "built-ins/Object/defineProperty/15.2.3.6-4-183.js", + "built-ins/Object/defineProperty/coerced-P-grow.js", "built-ins/parseFloat/S15.1.2.3_A6.js", "built-ins/parseInt/S15.1.2.2_A8.js", "built-ins/RegExp/property-escapes/generated/Alphabetic.js", @@ -42,6 +48,8 @@ "built-ins/TypedArray/prototype/copyWithin/coerced-values-end-detached-prototype.js", "built-ins/TypedArray/prototype/copyWithin/coerced-values-end-detached.js", "built-ins/TypedArray/prototype/copyWithin/coerced-values-start-detached.js", + "built-ins/TypedArray/prototype/entries/resizable-buffer-grow-mid-iteration.js", + "built-ins/TypedArray/prototype/at/resizable-buffer.js", "harness/propertyhelper-verifywritable-array-length.js", "language/comments/S7.4_A5.js", "language/comments/S7.4_A6.js", @@ -68,4 +76,4 @@ "staging/sm/regress/regress-610026.js", "staging/sm/String/normalize-generateddata-input.js" ] -} +} \ No newline at end of file From 2190159874b5824785c05dcfac18dd082e56f50d Mon Sep 17 00:00:00 2001 From: yossydev Date: Fri, 7 Feb 2025 22:36:11 +0900 Subject: [PATCH 05/10] chore(ecmascript): do not copy --- .../weak_map_objects/weak_map_constructor.rs | 2 +- .../weak_map_objects/weak_map_prototype.rs | 24 ------------------- 2 files changed, 1 insertion(+), 25 deletions(-) diff --git a/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_constructor.rs b/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_constructor.rs index f7947c224..b984bc0cc 100644 --- a/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_constructor.rs +++ b/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_constructor.rs @@ -14,7 +14,7 @@ use crate::ecmascript::abstract_operations::operations_on_objects::{ }; use crate::ecmascript::abstract_operations::testing_and_comparison::is_callable; use crate::ecmascript::builtins::array::ArrayHeap; -use crate::ecmascript::builtins::keyed_collections::weak_map_objects::weak_map_prototype::canonicalize_keyed_collection_key; +use crate::ecmascript::builtins::keyed_collections::map_objects::map_prototype::canonicalize_keyed_collection_key; use crate::ecmascript::builtins::ordinary::ordinary_create_from_constructor; use crate::ecmascript::builtins::weak_map::data::WeakMapData; use crate::ecmascript::builtins::weak_map::WeakMap; diff --git a/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_prototype.rs b/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_prototype.rs index 086416bb4..da2ab7fd6 100644 --- a/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_prototype.rs +++ b/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_prototype.rs @@ -2,9 +2,6 @@ // 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/. -use std::ops::Index; - -use crate::ecmascript::types::HeapNumber; use crate::engine::context::GcScope; use crate::{ ecmascript::{ @@ -105,24 +102,3 @@ impl WeakMapPrototype { .build(); } } - -#[inline(always)] -/// ### [24.5.1 CanonicalizeKeyedCollectionKey ( key )](https://tc39.es/ecma262/#sec-canonicalizekeyedcollectionkey) -/// The abstract operation CanonicalizeKeyedCollectionKey takes argument key -/// (an ECMAScript language value) and returns an ECMAScript language value. -pub(crate) fn canonicalize_keyed_collection_key( - agent: &impl Index, Output = f64>, - key: Value, -) -> Value { - // 1. If key is -0𝔽, return +0𝔽. - if let Value::SmallF64(key) = key { - // Note: Only f32 should hold -0. - if key.into_f64() == -0.0 { - return 0.into(); - } - } else if let Value::Number(key) = key { - debug_assert_ne!(agent[key], -0.0, "HeapNumber should never be -0.0"); - } - // 2. Return key. - key -} From 993cba5ed56bee34ef460e0609bb61256e1c6514 Mon Sep 17 00:00:00 2001 From: yossydev Date: Tue, 11 Feb 2025 14:22:43 +0900 Subject: [PATCH 06/10] chore(ecmascript): add_entries_from_iterable params Map to Object --- .../map_objects/map_constructor.rs | 13 ++- .../weak_map_objects/weak_map_constructor.rs | 80 +++---------------- 2 files changed, 20 insertions(+), 73 deletions(-) diff --git a/nova_vm/src/ecmascript/builtins/keyed_collections/map_objects/map_constructor.rs b/nova_vm/src/ecmascript/builtins/keyed_collections/map_objects/map_constructor.rs index c4a828e9f..71e7d5481 100644 --- a/nova_vm/src/ecmascript/builtins/keyed_collections/map_objects/map_constructor.rs +++ b/nova_vm/src/ecmascript/builtins/keyed_collections/map_objects/map_constructor.rs @@ -298,7 +298,14 @@ pub fn add_entries_from_iterable_map_constructor<'a>( } } - add_entries_from_iterable(agent, target.unbind(), iterable, adder.unbind(), gc) + Ok(Map::try_from(add_entries_from_iterable( + agent, + target.into_object().unbind(), + iterable, + adder.unbind(), + gc, + )?) + .unwrap()) } /// ### [24.1.1.2 AddEntriesFromIterable ( target, iterable, adder )](https://tc39.es/ecma262/#sec-add-entries-from-iterable) @@ -316,11 +323,11 @@ pub fn add_entries_from_iterable_map_constructor<'a>( /// > key. pub(crate) fn add_entries_from_iterable<'a>( agent: &mut Agent, - target: Map, + target: Object, iterable: Value, adder: Function, mut gc: GcScope<'a, '_>, -) -> JsResult> { +) -> JsResult> { let target = target.bind(gc.nogc()).scope(agent, gc.nogc()); let adder = adder.bind(gc.nogc()).scope(agent, gc.nogc()); // 1. Let iteratorRecord be ? GetIterator(iterable, SYNC). diff --git a/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_constructor.rs b/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_constructor.rs index b984bc0cc..445d02102 100644 --- a/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_constructor.rs +++ b/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_constructor.rs @@ -6,14 +6,10 @@ use std::hash::Hasher; use ahash::AHasher; -use crate::ecmascript::abstract_operations::operations_on_iterator_objects::{ - get_iterator, if_abrupt_close_iterator, iterator_close, iterator_step_value, -}; -use crate::ecmascript::abstract_operations::operations_on_objects::{ - call_function, get, get_method, try_get, -}; +use crate::ecmascript::abstract_operations::operations_on_objects::{get, get_method, try_get}; use crate::ecmascript::abstract_operations::testing_and_comparison::is_callable; use crate::ecmascript::builtins::array::ArrayHeap; +use crate::ecmascript::builtins::keyed_collections::map_objects::map_constructor::add_entries_from_iterable; use crate::ecmascript::builtins::keyed_collections::map_objects::map_prototype::canonicalize_keyed_collection_key; use crate::ecmascript::builtins::ordinary::ordinary_create_from_constructor; use crate::ecmascript::builtins::weak_map::data::WeakMapData; @@ -263,68 +259,12 @@ pub fn add_entries_from_iterable_weak_map_constructor<'a>( } } - add_entries_from_iterable(agent, target.unbind(), iterable, adder.unbind(), gc) -} - -/// ### [24.1.1.2 AddEntriesFromIterable ( target, iterable, adder )](https://tc39.es/ecma262/#sec-add-entries-from-iterable) -/// -/// The abstract operation AddEntriesFromIterable takes arguments target (an -/// Object), iterable (an ECMAScript language value, but not undefined or -/// null), and adder (a function object) and returns either a normal completion -/// containing an ECMAScript language value or a throw completion. adder will -/// be invoked, with target as the receiver. -/// -/// > NOTE: The parameter iterable is expected to be an object that implements -/// > an @@iterator method that returns an iterator object that produces a two -/// > element array-like object whose first element is a value that will be used -/// > as a WeakMap key and whose second element is the value to associate with that -/// > key. -pub(crate) fn add_entries_from_iterable<'a>( - agent: &mut Agent, - target: WeakMap, - iterable: Value, - adder: Function, - mut gc: GcScope<'a, '_>, -) -> JsResult> { - let target = target.bind(gc.nogc()).scope(agent, gc.nogc()); - let adder = adder.bind(gc.nogc()).scope(agent, gc.nogc()); - // 1. Let iteratorRecord be ? GetIterator(iterable, SYNC). - let mut iterator_record = get_iterator(agent, iterable, false, gc.reborrow())?; - // 2. Repeat, - loop { - // a. Let next be ? IteratorStepValue(iteratorRecord). - let next = iterator_step_value(agent, &mut iterator_record, gc.reborrow())?; - // b. If next is DONE, return target. - let Some(next) = next else { - return Ok(target.get(agent).bind(gc.into_nogc())); - }; - // c. If next is not an Object, then - let Ok(next) = Object::try_from(next) else { - // i. Let error be ThrowCompletion(a newly created TypeError object). - let error = agent.throw_exception_with_static_message( - ExceptionType::TypeError, - "Invalid iterator next return value", - gc.nogc(), - ); - // ii. Return ? IteratorClose(iteratorRecord, error). - return iterator_close(agent, &iterator_record, Err(error), gc.reborrow()); - }; - // d. Let k be Completion(Get(next, "0")). - let k = get(agent, next, 0.into(), gc.reborrow()); - // e. IfAbruptCloseIterator(k, iteratorRecord). - let k = if_abrupt_close_iterator(agent, k, &iterator_record, gc.reborrow())?; - // f. Let v be Completion(Get(next, "1")). - let v = get(agent, next, 1.into(), gc.reborrow()); - // g. IfAbruptCloseIterator(v, iteratorRecord). - let v = if_abrupt_close_iterator(agent, v, &iterator_record, gc.reborrow())?; - // h. Let status be Completion(Call(adder, target, « k, v »)). - let status = call_function( - agent, - adder.get(agent), - target.get(agent).into_value(), - Some(ArgumentsList(&[k, v])), - gc.reborrow(), - ); - let _ = if_abrupt_close_iterator(agent, status, &iterator_record, gc.reborrow())?; - } + Ok(WeakMap::try_from(add_entries_from_iterable( + agent, + target.into_object().unbind(), + iterable, + adder.unbind(), + gc, + )?) + .unwrap()) } From 5d19165a51c782543b134afa871f74a8d8296ce0 Mon Sep 17 00:00:00 2001 From: yossydev Date: Tue, 11 Feb 2025 14:27:13 +0900 Subject: [PATCH 07/10] chore(ecmascript): remove size --- nova_vm/src/ecmascript/builtins/weak_map/data.rs | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/nova_vm/src/ecmascript/builtins/weak_map/data.rs b/nova_vm/src/ecmascript/builtins/weak_map/data.rs index 8c8251cb8..cba47cc3f 100644 --- a/nova_vm/src/ecmascript/builtins/weak_map/data.rs +++ b/nova_vm/src/ecmascript/builtins/weak_map/data.rs @@ -35,19 +35,6 @@ pub(crate) struct WeakMapData { } impl WeakMapHeapData { - /// ### [24.2.1.5 WeakMapDataSize ( setData )](https://tc39.es/ecma262/#sec-setdatasize) - /// - /// The abstract operation MapDataSize takes argument setData (a List of either - /// ECMAScript language values or EMPTY) and returns a non-negative integer. - #[inline(always)] - pub fn size(&self) -> u32 { - // 1. Let count be 0. - // 2. For each element e of setData, do - // a. If e is not EMPTY, set count to count + 1. - // 3. Return count. - self.weak_map_data.weak_map_data.borrow().len() as u32 - } - pub fn keys(&self) -> &[Option] { &self.weak_map_data.keys } From 26d58923fc265925733fe443c9e042265e097db0 Mon Sep 17 00:00:00 2001 From: yossydev Date: Tue, 11 Feb 2025 14:46:20 +0900 Subject: [PATCH 08/10] chore(ecmascript): WeakMap key is object or symbol --- .../weak_map_objects/weak_map_constructor.rs | 54 ++++++++++++------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_constructor.rs b/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_constructor.rs index 445d02102..d0115eb5b 100644 --- a/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_constructor.rs +++ b/nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_constructor.rs @@ -228,26 +228,40 @@ pub fn add_entries_from_iterable_weak_map_constructor<'a>( unreachable!() }; let slice = entry.as_slice(&array_heap); - let key = canonicalize_keyed_collection_key(numbers, slice[0].unwrap()); - let key_hash = hasher(key); - let value = slice[1].unwrap(); - let next_index = keys.len() as u32; - let entry = map_data.entry( - key_hash, - |hash_equal_index| keys[*hash_equal_index as usize].unwrap() == key, - |index_to_hash| hasher(keys[*index_to_hash as usize].unwrap()), - ); - match entry { - hashbrown::hash_table::Entry::Occupied(occupied) => { - // We have duplicates in the array. Latter - // ones overwrite earlier ones. - let index = *occupied.get(); - values[index as usize] = Some(value); - } - hashbrown::hash_table::Entry::Vacant(vacant) => { - vacant.insert(next_index); - keys.push(Some(key)); - values.push(Some(value)); + if let Some(value) = slice[0] { + if value.is_object() || value.is_symbol() { + let key = canonicalize_keyed_collection_key(numbers, value); + let key_hash = hasher(key); + let value = slice[1].unwrap(); + let next_index = keys.len() as u32; + let entry = map_data.entry( + key_hash, + |hash_equal_index| { + keys[*hash_equal_index as usize].unwrap() == key + }, + |index_to_hash| { + hasher(keys[*index_to_hash as usize].unwrap()) + }, + ); + match entry { + hashbrown::hash_table::Entry::Occupied(occupied) => { + // We have duplicates in the array. Latter + // ones overwrite earlier ones. + let index = *occupied.get(); + values[index as usize] = Some(value); + } + hashbrown::hash_table::Entry::Vacant(vacant) => { + vacant.insert(next_index); + keys.push(Some(key)); + values.push(Some(value)); + } + } + } else { + return Err(agent.throw_exception_with_static_message( + ExceptionType::TypeError, + "WeakMap key must be an Object or Symbol", + gc.nogc(), + )); } } } From 52b67a0cc070b071d1bd43c5014a52c38f2a3cfe Mon Sep 17 00:00:00 2001 From: yossydev Date: Tue, 11 Feb 2025 16:41:11 +0900 Subject: [PATCH 09/10] chore(ecmascritp): update --- tests/expectations.json | 1 - tests/metrics.json | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/expectations.json b/tests/expectations.json index edf1f30a4..6972a72cf 100644 --- a/tests/expectations.json +++ b/tests/expectations.json @@ -9557,7 +9557,6 @@ "built-ins/TypedArray/of/not-a-constructor.js": "FAIL", "built-ins/TypedArray/of/prop-desc.js": "FAIL", "built-ins/TypedArray/of/resized-with-out-of-bounds-and-in-bounds-indices.js": "CRASH", - "built-ins/TypedArray/out-of-bounds-get-and-set.js": "CRASH", "built-ins/TypedArray/prototype/byteLength/resized-out-of-bounds-1.js": "CRASH", "built-ins/TypedArray/prototype/byteLength/resized-out-of-bounds-2.js": "CRASH", "built-ins/TypedArray/prototype/byteOffset/resized-out-of-bounds.js": "CRASH", diff --git a/tests/metrics.json b/tests/metrics.json index e6f4e47d4..cf977f2c1 100644 --- a/tests/metrics.json +++ b/tests/metrics.json @@ -1,8 +1,8 @@ { "results": { - "crash": 13142, - "fail": 9060, - "pass": 24526, + "crash": 13104, + "fail": 9059, + "pass": 24565, "skip": 73, "timeout": 0, "unresolved": 0 From 5508ceea7c9836cc7285a75490e0c329fde177c5 Mon Sep 17 00:00:00 2001 From: yossydev Date: Wed, 5 Mar 2025 23:09:42 +0900 Subject: [PATCH 10/10] chore(ecmascriprt): refacrot weak_map data --- .../src/ecmascript/builtins/weak_map/data.rs | 243 +++--------------- 1 file changed, 34 insertions(+), 209 deletions(-) diff --git a/nova_vm/src/ecmascript/builtins/weak_map/data.rs b/nova_vm/src/ecmascript/builtins/weak_map/data.rs index 533092285..11f35265c 100644 --- a/nova_vm/src/ecmascript/builtins/weak_map/data.rs +++ b/nova_vm/src/ecmascript/builtins/weak_map/data.rs @@ -1,59 +1,36 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// 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/. - use crate::{ - ecmascript::types::{ - BIGINT_DISCRIMINANT, HeapNumber, HeapPrimitive, HeapString, NUMBER_DISCRIMINANT, - OrdinaryObject, STRING_DISCRIMINANT, Value, bigint::HeapBigInt, - }, - engine::context::Bindable, + ecmascript::types::{Object, Symbol, Value}, heap::{CompactionLists, HeapMarkAndSweep, PrimitiveHeapIndexable, WorkQueues}, }; -use ahash::AHasher; +use ahash::AHashMap; use core::{ - cell::RefCell, - hash::{Hash, Hasher}, + hash::Hash, sync::atomic::{AtomicBool, Ordering}, }; -use hashbrown::{HashTable, hash_table::Entry}; -#[derive(Debug, Default)] -pub struct WeakMapHeapData { - pub(crate) object_index: Option>, - weak_map_data: WeakMapData, +#[derive(Debug, Hash, Eq, PartialEq)] +pub(crate) enum SymbolOrObject<'a> { + Symbol(Symbol<'a>), + Object(Object<'a>), } #[derive(Debug, Default)] pub(crate) struct WeakMapData { - // TODO: Use a ParallelVec to remove one unnecessary allocation. - // pub(crate) key_values: ParallelVec, Option> - pub(crate) keys: Vec>>, - pub(crate) values: Vec>>, - /// Low-level hash table pointing to keys-values indexes. - pub(crate) weak_map_data: RefCell>, + pub(crate) weak_map_data: AHashMap, Value<'static>>, pub(crate) needs_primitive_rehashing: AtomicBool, } -impl WeakMapHeapData { - pub fn keys(&self) -> &[Option] { - &self.weak_map_data.keys - } - - pub fn values(&self) -> &[Option] { - &self.weak_map_data.values - } +#[derive(Debug, Default)] +pub struct WeakMapHeapData { + pub(crate) weak_map_data: WeakMapData, +} +impl WeakMapHeapData { pub fn clear(&mut self) { - // 3. For each Record { [[Key]], [[Value]] } p of M.[[MapData]], do - // a. Set p.[[Key]] to EMPTY. - // b. Set p.[[Value]] to EMPTY. - self.weak_map_data.weak_map_data.get_mut().clear(); - self.weak_map_data.values.fill(None); - self.weak_map_data.keys.fill(None); + self.weak_map_data.weak_map_data.clear(); } - pub(crate) fn borrow(&self, arena: &impl PrimitiveHeapIndexable) -> &WeakMapData { + pub(crate) fn borrow(&mut self, arena: &impl PrimitiveHeapIndexable) -> &WeakMapData { self.weak_map_data.rehash_if_needed(arena); &self.weak_map_data } @@ -66,200 +43,48 @@ impl WeakMapHeapData { impl WeakMapData { fn rehash_if_needed_mut(&mut self, arena: &impl PrimitiveHeapIndexable) { - if !*self.needs_primitive_rehashing.get_mut() { + if !self.needs_primitive_rehashing.load(Ordering::Relaxed) { return; } - - rehash_map_data(&self.keys, self.weak_map_data.get_mut(), arena); + self.rehash_map_data(); self.needs_primitive_rehashing .store(false, Ordering::Relaxed); } - fn rehash_if_needed(&self, arena: &impl PrimitiveHeapIndexable) { + fn rehash_if_needed(&mut self, arena: &impl PrimitiveHeapIndexable) { if !self.needs_primitive_rehashing.load(Ordering::Relaxed) { return; } - - rehash_map_data(&self.keys, &mut self.weak_map_data.borrow_mut(), arena); + self.rehash_map_data(); self.needs_primitive_rehashing .store(false, Ordering::Relaxed); } -} -fn rehash_map_data( - keys: &[Option], - map_data: &mut HashTable, - arena: &impl PrimitiveHeapIndexable, -) { - let hasher = |value: Value| { - let mut hasher = AHasher::default(); - value.unbind().hash(arena, &mut hasher); - hasher.finish() - }; - let hashes = { - let hasher = |discriminant: u8| { - let mut hasher = AHasher::default(); - discriminant.hash(&mut hasher); - hasher.finish() - }; - [ - (0u8, hasher(STRING_DISCRIMINANT)), - (1u8, hasher(NUMBER_DISCRIMINANT)), - (2u8, hasher(BIGINT_DISCRIMINANT)), - ] - }; - for (id, hash) in hashes { - let eq = |equal_hash_index: &u32| { - let value = keys[*equal_hash_index as usize].unwrap(); - match id { - 0 => HeapString::try_from(value).is_ok(), - 1 => HeapNumber::try_from(value).is_ok(), - 2 => HeapBigInt::try_from(value).is_ok(), - _ => unreachable!(), - } - }; - let mut entries = Vec::new(); - while let Ok(entry) = map_data.find_entry(hash, eq) { - entries.push(*entry.get()); - entry.remove(); + fn rehash_map_data(&mut self) { + let mut new_map = AHashMap::new(); + for (key, value) in self.weak_map_data.drain() { + new_map.insert(key, value); } - entries.iter().for_each(|entry| { - let key = keys[*entry as usize].unwrap(); - let key_hash = hasher(key); - let result = map_data.entry( - key_hash, - |equal_hash_index| { - // It should not be possible for there to be an equal item - // in the WeakMap already. - debug_assert_ne!(keys[*equal_hash_index as usize].unwrap(), key); - false - }, - |index_to_hash| hasher(keys[*index_to_hash as usize].unwrap()), - ); - - let Entry::Vacant(result) = result else { - unreachable!(); - }; - result.insert(*entry); - }); + self.weak_map_data = new_map; } } impl HeapMarkAndSweep for WeakMapHeapData { fn mark_values(&self, queues: &mut WorkQueues) { - let Self { - object_index, - weak_map_data, - } = self; - object_index.mark_values(queues); - weak_map_data - .keys + self.weak_map_data + .weak_map_data .iter() - .for_each(|value| value.mark_values(queues)); - weak_map_data - .values - .iter() - .for_each(|value| value.mark_values(queues)); + .for_each(|(_, value)| { + value.mark_values(queues); + }); } fn sweep_values(&mut self, compactions: &CompactionLists) { - let Self { - object_index, - weak_map_data, - } = self; - let WeakMapData { - keys, - values, - needs_primitive_rehashing, - weak_map_data, - } = weak_map_data; - let weak_map_data = weak_map_data.get_mut(); - object_index.sweep_values(compactions); - - let hasher = |value: Value| { - let mut hasher = AHasher::default(); - if value.try_hash(&mut hasher).is_err() { - // A heap String, Number, or BigInt required rehashing as part - // of moving an Object key inside the HashTable. The heap - // vectors for those data points are currently being sweeped on - // another thread, so we cannot access them right now even if - // we wanted to. This situation should be fairly improbable as - // it requires mixing eg. long string and object values in the - // same WeakMap, so it's not pressing right now. Still, it must be - // handled eventually. We essentially mark the heap hash data - // as "dirty". Any lookup shall then later check this boolean - // and rehash primitive keys if necessary. - needs_primitive_rehashing.store(true, Ordering::Relaxed); - let value = HeapPrimitive::try_from(value).unwrap(); - // Return a hash based on the discriminant. This we are likely - // to cause hash collisions but we avoid having to rehash all - // keys; we can just rehash the primitive keys that match the - // discriminant hash. - core::mem::discriminant(&value).hash(&mut hasher); - } - hasher.finish() - }; - assert_eq!(keys.len(), values.len()); - for index in 0..keys.len() as u32 { - let key = &mut keys[index as usize]; - let Some(key) = key else { - // Skip empty slots. - continue; - }; - // Sweep value without any concerns. - values[index as usize].sweep_values(compactions); - - let old_key = *key; - key.sweep_values(compactions); - let new_key = *key; - - if old_key == new_key { - // No identity change, no hash change. - continue; - } - - if !new_key.is_object() { - // Non-objects do not change their hash even if their identity - // changes. - continue; - } - // Object changed identity; it must be moved in the map_data. - let old_hash = hasher(old_key); - let new_hash = hasher(new_key); - if let Ok(old_entry) = - weak_map_data.find_entry(old_hash, |equal_hash_index| *equal_hash_index == index) - { - // We do not always find an entry if a previous item has - // shifted ontop of our old hash. - old_entry.remove(); - } - let new_entry = weak_map_data.entry( - new_hash, - |equal_hash_index| { - // It's not possible for there to be another hash index that - // holds our item. But! It's possible that we're eg. shifting - // 32 to 30, and then 30 to 29. Thus 32 will happily override - // 30. - values[*equal_hash_index as usize].unwrap() == new_key - }, - |index_to_hash| { - let value = values[*index_to_hash as usize].unwrap(); - hasher(value) - }, - ); - match new_entry { - hashbrown::hash_table::Entry::Occupied(mut occupied) => { - // We found an existing entry that points to a - // (necesssarily) different slot that contains the same - // value. This value will necessarily be removed later; we - // can just reuse this slot. - *occupied.get_mut() = index; - } - hashbrown::hash_table::Entry::Vacant(vacant) => { - // This is the expected case: We're not overwriting a slot. - vacant.insert(index); - } - } + let mut new_map = AHashMap::new(); + for (key, mut value) in self.weak_map_data.weak_map_data.drain() { + value.sweep_values(compactions); + new_map.insert(key, value); } + self.weak_map_data.weak_map_data = new_map; } }