[js] use ES6 Map for haxe.ds.*Map when -D js-es >= 6

[nadako]

Some notes:

• I tried to use a common base class for them, but @:coreApi checks don't seem to look for parent-class methods, so I ended up copy-pasting the implementation, moving larger methods into js.Boot.

• Ideally we would inherit from native Map instead of wrapping it, but it's not really possible until we implement Add ES6 class support #6546.

• I added JsIteratorAdapter that adaps ES6 iteration protocol to haxe, but it only works if you call hasNext before next, which is required by the spec according to @ncannasse. However we have some unit tests that tell otherwise (added in 789c022 and related to the discussion in JS generator handles obj[""] badly #3908). So I added another guard to the #if for now. I guess we should remove the test and update the documentation for Iterator<T> to make things clear.

• I didn't use new js.Map(this.m) for the copy method as it's not supported by IE.

• I looked into also implementing haxe.ds.WeakMap using the native ES6 WeakMap, but unfortunately its API is insufficient:

  Because of references being weak, WeakMap keys are not enumerable (i.e. there is no method giving you a list of the keys). If they were, the list would depend on the state of garbage collection, introducing non-determinism. If you want to have a list of keys, you should use a Map.

  This raises questions if haxe.ds.WeakMap should have iterator and copy methods.

[RealyUniqueName]

I didn't use new js.Map(this.m) for the copy method as it's not supported by IE

According to that table keys(), values(), and entries() are not supported also, but you are using them.
I guess one should not enable js_es=6 if one wants to maintain IE compatibility.

[nadako]

I didn't use new js.Map(this.m) for the copy method as it's not supported by IE

According to that table keys(), values(), and entries() are not supported also, but you are using them.
I guess one should not enable js_es=6 if one wants to maintain IE compatibility.

You're right, damn...

[ncannasse]

According to the benchmarks, we don't want this by default, right?

[nadako]

Yeah, @back2dos did some benchmarking and even ObjectMap and StringMap are faster than native js maps, might be because of the specified iteration order or something. I'm thinking about not merging this at all and only adding JsIteratorAdapter which is unrelated and useful.

[Simn]

It might still be worthwhile to merge this in case somebody wants to really target ES6 and doesn't want the Haxe maps. But it shouldn't be the default even with the es6 flag or whatever set...

[kevinresol]

Yeah, @back2dos did some benchmarking and even ObjectMap and StringMap are faster than native js maps

I just wrote a test on iterating map values and seems like I got some different results:
https://try.haxe.org/#A5981

Using Haxe ObjectMap:
0.7220001220703125s

Using es6 Map and native for-of loop:
0.021999835968017578s

Using es6 Map and JsIteratorAdapter:
0.09300017356872559s

(macOS 10.13.6 + Chrome 69.0.3497.100)

Note: somehow I couldn't use the JsIteratorAdapter inline, i.e. for(v in new JsIteratorAdapter<Int>(map.values())) will loop nothing

(and don't forget to link to #7349)

[back2dos]

Ok. Well that complicates things. My benchmark didn't measure the speed of iterators, but that of get (most relevant for coconut ^^): https://try.haxe.org/#9e661

[nadako]

Iteration might be slow because of the fact it loops twice and allocates a lot (it creates an array of keys and then returns an iterator over that). (Un)surprisinigly, ES6 doesn't support for of and iterator protocol for normal objects (which haxe maps use under the hood), but I wonder if some generator trick can be used to create a "proper" iterator for an object.

[back2dos]

Yeah, it's true that the implementation is not optimal. It first produces the keys and then looks them up through the whole getId dance.

I've tried using an ES6 iterator and did get a speedup, but it's still far from native Map performance: https://try.haxe.org/#7E262

I would guess that there's perhaps a special optimization for Map iterators in the runtime that we'll have a hard time to beat?

In any case, I think we could store the keys as array, instead of an object. I'll try what that yields later today.

[Simn]

@nadako Do you think this is worth the effort? If not, feel free to close. Otherwise, please update!

[nadako]

It probably is, will update once I get a minute, I'll have to look into this again to see if we can use/subclass js Map directly instead of wrapping it into another object.

[saem]

Just read this, the benchmark comparison is concerning. The current haxe Map is mutating the object, that might seem OK for this micro-benchmark but I believe it causes potentially permanent deoptimizations for VMs maintaining any sort of caches for object definitions. Also, collision issues?

Looking at V8 where the internals are well discussed this can easily lead to permanent deoptimization see: https://v8.dev/blog/fast-properties.

Additionally, if said object is in more than one data structure, say simultaneously an array of otherwise the same type, I believe this will deoptimize those structures permanently as well.

I've done a quick read of the internals of Map in V8, which is based heavily off Firefox so the story is likely similar, and the structures are far more optimal. Providing tightly packed fixed arrays as pages in terms of internal layout and specialized types for maps with less than 256 items.

Outside of edge cases, I think it'd be very dangerous to draw any conclusions that'd suggest the current haxe code is faster. In fact, if go as far as to say it's always slower and without a comprehensive set of benchmarks otherwise I'd encourage people to be very unconvinced.

One can edit the existing benchmark, simply strip away all usage of haxe.ds.Map and all of a sudden the ES6 Map code runs faster over there same data set.

In my case it went from 0.1 to 0.07 and was stable over repeated runs.

[saem]

I should add that V8 specializes for not only size of map but also the key type. There might be more but I'm working off memory of research from a few days ago.

Specifically, I'm using Heaps targeting modern browsers, electron, and hashlink. Map implementations over linear storage, like the implementations I've seen on the browser side, are rather optimal for approaches such as ECS for data storage (entity component system) and that's what I'm attempting to do.

An up-to-date haxe.ds.Map would be very helpful, heck even simple modifications to js.lib.Map such that it was compatible with the interface IMap I believe would solve many issues.

[nadako]

Ok, since this issue is now hanging in my new github notification inbox marked as undone, I'll look into it soon.

[saem]

tl;dr Sorry, I couldn't help write an updated fix.

Tried implementing this locally in my project to see I could help, focusing solely on getting Map as an extern. I mostly made the complier happy...

The blocker is making the native Map's entries compatible with KevValueIterator. As that uses an anonymous structure with key & value properties, while JavaScript returns a pair (array) and I'm not seeing a reasonable way to unify things presently without excessive allocations. Abstracts don't unify, so that's out and a class with an inline constructor (hoping it wouldn't cost anything) also didn't work.

Presently all I'm coming up with are for potentially radical changes to KeyValueIterator and the anonymous definition, but I don't know the complier and specifically the limitations well here so I'm not sure how to proceed.

Sorry I couldn't help provide an updated patch though I did learn a few things along the way. 😅

[nadako]

I also started working on a new attempt to implement this in #9303 I'll close this PR as it's seriously outdated now.