feat(ecmascript): Map.groupBy #559
Conversation
| // c. Append entry to map.[[MapData]]. | ||
| todo!() |
There was a problem hiding this comment.
“I don’t know how to implement this part, so I left it as a TODO. 😱😭”
There was a problem hiding this comment.
This should be basically equivalent to step 6 of Map.prototype.set, so you can find the roughly correct code over in map_prototype.rs:455.
There was a problem hiding this comment.
Thank you! Honestly, I’m not confident, but I tried making it while using your reference.
Some tests are still failing, so I think something is probably missing. e507955
There was a problem hiding this comment.
Hmm, from a quick read, this all looks good. I guess figuring out why the tests are failing would need proper investigation :(
yossydev
changed the title
| // c. Append entry to map.[[MapData]]. | ||
| todo!() |
There was a problem hiding this comment.
This should be basically equivalent to step 6 of Map.prototype.set, so you can find the roughly correct code over in map_prototype.rs:455.
| let mut groups = group_by_property(agent, items, callback_fn, gc.reborrow())?; | ||
| // 2. Let map be ! Construct(%Map%). | ||
| let map = agent.current_realm().intrinsics().map(); | ||
| let map = construct(agent, map.into_function(), None, None, gc.reborrow())? |
There was a problem hiding this comment.
suggestion: Because of the ! Construct we know that this will never call into user code (because it is synchronous code and has been marked as never throwing); here it might be a good idea to instead use agent.heap.create(MapHeapData { ... }) directly. Another good idea with that would be to implement a reserve method on MapHeapData (if we don't have that already?) that would be called here with groups.len() so as to ensure we have the proper amount of memory reserved with a minimal amount of allocations.
There was a problem hiding this comment.
Thank you! I tried fixing it, but what do you think??
95c8c22
yossydev
force-pushed
the
feat/map-group-by
branch
from
00cd559 to
e507955
Compare
| map_data.reserve(groups.len()); | ||
| let map = agent.heap.create(map_data); | ||
|
|
||
| let gc = gc.nogc(); |
There was a problem hiding this comment.
nitpick: In this sort of case, better to use gc.into_nogc() for lifetime related reasons.
nova_vm/src/ecmascript/builtins/keyed_collections/map_objects/map_constructor.rs
Outdated
Show resolved
Hide resolved
| let mut groups = group_by_property(agent, items, callback_fn, gc.reborrow())?; | ||
| // 2. Let map be ! Construct(%Map%). | ||
| let map = agent.current_realm().intrinsics().map(); | ||
| let map = construct(agent, map.into_function(), None, None, gc.reborrow())? |
| @@ -84,6 +84,19 @@ impl MapHeapData { | |||
| self.map_data.rehash_if_needed_mut(arena); | |||
| &mut self.map_data | |||
| } | |||
|
|
|||
| pub fn reserve(&mut self, new_len: usize) { | |||
There was a problem hiding this comment.
issue: Take arena: &impl PrimitiveHeapIndexable here to use the value.hash() function; read the key value from map_data.keys[key_index as usize] like elsewhere.
There was a problem hiding this comment.
Thank you! I’d like to confirm something regarding this.
If we add arena as an argument, I think we need to define primitive_heap before calling map_data.reserve(), so would the code look like this?
let groups = group_by_property(agent, items, callback_fn, gc.reborrow())?;
// 2. Let map be ! Construct(%Map%).
let Heap {
maps,
bigints,
numbers,
strings,
..
} = &mut agent.heap;
let primitive_heap = PrimitiveHeap::new(bigints, numbers, strings);
let mut map_data = MapHeapData::default();
map_data.reserve(groups.len(), &primitive_heap);
let map = agent.heap.create(map_data);However, this results in the error:
“cannot borrow agent.heap as mutable more than once at a time second mutable borrow occurs here.”
Does this mean my implementation approach is different from what you had in mind?
Could you explain the correct approach? 🙇♂️
| // c. Append entry to map.[[MapData]]. | ||
| todo!() |
There was a problem hiding this comment.
Hmm, from a quick read, this all looks good. I guess figuring out why the tests are failing would need proper investigation :(
|
Oh, I figured out the test failures: I assume your system somehow ensures that uninitialized memory is always zeroed out. We weren't properly doing zeroing out new data in an ArrayBuffer data when resizing. On your system the tests testing for that work because your system does it automatically. On CI and my machine, it does not happen automatically so the tests fail there. #563 fixes the UB there. |
yossydev
force-pushed
the
feat/map-group-by
branch
from
e507955 to
0924398
Compare
|
@aapoalas 0924398 ← Everything has been fixed except for the issue mentioned in this discussion! |
Amazing, you found it so well!! Haha |
There are still a few more parts that need to be fixed.
ref: #146
doc: https://tc39.es/ecma262/multipage/keyed-collections.html#sec-map.groupby