feat(ecmascript): WeakMap Constructor #561

yossydev · 2025-02-06T13:59:15Z

The WeakMap constructor itself is almost the same as the Map constructor, so most of the implementation is unchanged from Map.
Among them, add_entries_from_iterable_map_constructor is designed to accept a Map type, so I have modified it to accept a WeakMap instead.
I’m wondering whether this is a good approach implementation-wise or if it should be further abstracted.

ref: #148
doc: https://tc39.es/ecma262/multipage/keyed-collections.html#sec-weakmap-iterable

aapoalas

Thank you for this! You've picked a prickly customer again; Weak references AKA ephemerons are something that I've not yet implemented in the GC, and I somewhat dread implementing them due to the horrible things they require :D

Implementing the constructor and the prototype methods is definitely possible even without the GC support, though, so no worries there! We'll just need to file off a few rough edges.

aapoalas · 2025-02-06T16:25:47Z

nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_prototype.rs

+/// ### [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(


issue: Don't copy this code, refer to it.

thank you! I fixed! 2190159

aapoalas · 2025-02-06T16:26:48Z

nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_constructor.rs

+                                unreachable!()
+                            };
+                            let slice = entry.as_slice(&array_heap);
+                            let key = canonicalize_keyed_collection_key(numbers, slice[0].unwrap());


issue: The key in a WeakMap cannot be any Value but needs to be an object or a symbol.

Thank you! I’ve fixed this too! 26d5892
However, I think the code will probably change again once we create the enum 👀 ref: #561 (comment)

aapoalas · 2025-02-06T16:27:43Z

nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_constructor.rs

+/// > 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>(


issue: We should reuse the add_entries_from_iterable over in map_constructor.rs here and just change its target type to be Object.

(There is also a possibility of making it take an adder: impl FnMut but that's kind of a side point.)

thank you! I fixed this too! 993cba5

aapoalas · 2025-02-06T16:33:53Z

nova_vm/src/ecmascript/builtins/weak_map.rs

@@ -89,6 +89,17 @@ impl<'a> From<WeakMap<'a>> for Object<'a> {
    }
 }

+impl<'a> TryFrom<Object<'a>> for WeakMap<'a> {


praise: Nice; it's a bit awkward to be missing these all over the place :D

aapoalas · 2025-02-06T16:35:11Z

nova_vm/src/ecmascript/builtins/weak_map/data.rs

-    // 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<Option<Value>>,


issue: The keys are always Object or Symbol. That should probably be a new enum equivalent to Object but with the Symbol added in.

Naming is always the hardest part... SymbolOrObject sounds dumb so probably not that. Identifiable? That's very vague... I don't know of a good name for these :/ "Referenceable" is somewhat correct. Spec calls these "value with identity", and tests for them via "CanBeHeldWeakly". "WeaklyReferenceableValue", "ValueWithIdentity", "IdentityValue", ... All of these sound a bit weak :) (pun very much intended!)

Good ideas are very much welcome on the naming front :)

Would something like WeakMapKey or WeakKey be too vague and lacking information? 👀

The expected enum to be created looks something like this, right? 👀

#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum WeakMapKey<'a> { Object(Option<Object<'a>>), Symbol(Option<Symbol<'a>>), } #[derive(Debug, Default)] pub(crate) struct WeakMapData<'a> { // TODO: Use a ParallelVec to remove one unnecessary allocation. // pub(crate) key_values: ParallelVec<Option<Value>, Option<Value>> pub(crate) keys: Vec<Option<WeakMapKey<'a>>>, pub(crate) values: Vec<Option<Value>>, /// Low-level hash table pointing to keys-values indexes. pub(crate) weak_map_data: RefCell<HashTable<u32>>, pub(crate) needs_primitive_rehashing: AtomicBool, }

Hmmm.... WeakKey sounds pretty good indeed. It might even suggest a unification of all objects that hold weak references (WeakMap, WeakSet, WeakRef, FinalizationRegistry) to not hold a reference to the key object/symbol directly but instead point to a 'weak key" vector which would then hold the object/symbol reference. This would make weak objects more expensive to access but would make GC'ing them maybe more straightforward. Choices, choices...

But that's a decision for later.

issue; The WeakMapKey enum should not be the kind of "recursive enum" that you've suggested there, but should instead be basically a copy-paste of the Object enum with just one extra variant, Symbol added in. The Symbol variant's value should also be SYMBOL_DISCRIMINANT.

aapoalas · 2025-02-06T16:36:53Z

nova_vm/src/ecmascript/builtins/weak_map/data.rs

+    pub(crate) keys: Vec<Option<Value>>,
+    pub(crate) values: Vec<Option<Value>>,
+    /// Low-level hash table pointing to keys-values indexes.
+    pub(crate) weak_map_data: RefCell<HashTable<u32>>,


issue: We actually do not need the keys and values here at all: A WeakMap cannot be iterated in its insertion order, so we do not need to keep keys and values separately from the hashmap.

I believe we should use a plain AHashMap<SymbolOrObject, Value> here.

Thank you!

With key and value removed, the WeakMapData struct would look like this, right? 👀

pub(crate) struct WeakMapData { /// Low-level hash table pointing to keys-values indexes. pub(crate) weak_map_data: AHashMap<SymbolOrObject, Value>, pub(crate) needs_primitive_rehashing: AtomicBool, }

Yeah, that looks perfect.

aapoalas · 2025-02-06T16:50:50Z

nova_vm/src/ecmascript/builtins/weak_map/data.rs

+    /// 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 {


issue: The 24.2.1.5 abstract operation isn't about getting the data size, and we're mildly unlikely to need one as it's not possible to read the size of a WeakMap from JS.

The keys and values are likewise probably not going to be needed.

thank you! I removed this! 5d19165

yossydev · 2025-02-07T03:26:06Z

Thank you for this! You've picked a prickly customer again; Weak references AKA ephemerons are something that I've not yet implemented in the GC, and I somewhat dread implementing them due to the horrible things they require :D

Implementing the constructor and the prototype methods is definitely possible even without the GC support, though, so no worries there! We'll just need to file off a few rough edges.

I see, so that’s how it is.
Looking at just the constructor, I thought it was almost the same as Map and wouldn’t be that difficult…

I’ll fix the parts you pointed out in the review later! Thank you!!

aapoalas · 2025-03-02T17:34:20Z

nova_vm/src/ecmascript/builtins/keyed_collections/weak_map_objects/weak_map_constructor.rs

+        .unbind()
+        .bind(gc.nogc());
+        // 3. Set map.[[WeakMapData]] to a new empty List.
+        let iterable = arguments.get(0);


issue: Take the iterable parameter out from arguments at the very beginning of the function and bind it to gc.nogc(). Also do the same for new_target, although in this case (and in most cases) it is already correct as-is.

The iterable parameter usage, however, currently holds a possibility for use-after-free bugs when interleaved GC is turned on.

Thank you! I probably managed to fix it while resolving the conflicts!

aapoalas · 2025-03-02T17:37:43Z

nova_vm/src/ecmascript/builtins/weak_map/data.rs

-    // 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<Option<Value>>,


issue; The WeakMapKey enum should not be the kind of "recursive enum" that you've suggested there, but should instead be basically a copy-paste of the Object enum with just one extra variant, Symbol added in. The Symbol variant's value should also be SYMBOL_DISCRIMINANT.

aapoalas · 2025-03-02T17:38:23Z

nova_vm/src/ecmascript/builtins/weak_map/data.rs

+    pub(crate) keys: Vec<Option<Value>>,
+    pub(crate) values: Vec<Option<Value>>,
+    /// Low-level hash table pointing to keys-values indexes.
+    pub(crate) weak_map_data: RefCell<HashTable<u32>>,


Yeah, that looks perfect.

aapoalas requested changes Feb 6, 2025

View reviewed changes

yossydev added 9 commits February 11, 2025 16:37

feat(ecmascript): WeakMap Constructor

4e8a614

chore(ecmascript): update metrics

04b7f6b

chore(ecmascript): update expectations

0d611a9

chore(ecmascript): update skip

594bc6d

chore(ecmascript): do not copy

2190159

chore(ecmascript): add_entries_from_iterable params Map to Object

993cba5

chore(ecmascript): remove size

5d19165

chore(ecmascript): WeakMap key is object or symbol

26d5892

chore(ecmascritp): update

52b67a0

yossydev force-pushed the feat/weakmap-constructor branch from 5b49404 to 52b67a0 Compare February 11, 2025 07:41

yossydev requested a review from aapoalas February 11, 2025 07:46

aapoalas reviewed Mar 2, 2025

View reviewed changes

yossydev added 2 commits March 5, 2025 21:25

Merge branch 'main' into feat/weakmap-constructor

5e00dde

chore(ecmascriprt): refacrot weak_map data

5508cee

aapoalas mentioned this pull request Apr 7, 2025

WeakMap & FinalizationRegistroy #623

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ecmascript): WeakMap Constructor #561

feat(ecmascript): WeakMap Constructor #561

yossydev commented Feb 6, 2025

aapoalas left a comment

aapoalas Feb 6, 2025

yossydev Feb 11, 2025

aapoalas Feb 6, 2025

yossydev Feb 11, 2025

aapoalas Feb 6, 2025

yossydev Feb 11, 2025

aapoalas Feb 6, 2025

aapoalas Feb 6, 2025

yossydev Feb 11, 2025

yossydev Feb 11, 2025 •

edited

Loading

aapoalas Feb 11, 2025

aapoalas Mar 2, 2025

aapoalas Feb 6, 2025

yossydev Feb 11, 2025

aapoalas Mar 2, 2025

aapoalas Feb 6, 2025

yossydev Feb 11, 2025

yossydev commented Feb 7, 2025

aapoalas Mar 2, 2025

yossydev Mar 5, 2025

aapoalas Mar 2, 2025

aapoalas Mar 2, 2025

feat(ecmascript): WeakMap Constructor #561

Are you sure you want to change the base?

feat(ecmascript): WeakMap Constructor #561

Conversation

yossydev commented Feb 6, 2025

aapoalas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yossydev Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yossydev commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yossydev Feb 11, 2025 •

edited

Loading