RR:JS collections

[Emily Crutcher]

John,  master of JRE collections, Could you review this?

For big applications, map's performance can end up being a bottleneck.  This code review introduces the AbstractJsMap, which is a slightly modified API so  that we can create faster map implementations.


The code contains new directories, so here is a read-only branch with the code in it:http://code.google.com/p/google-web-toolkit-incubator/source/browse/#svn/branches/ecc/collectionsReview

Attached is the benchmark for putting then getting 1000 through 10,000 strings, where  HashMap is compared to the new JsStringMap. For the compiled put/get benchmark, it is between 300%-500% faster.

  Thanks,

           Emily



--
"There are only 10 types of people in the world: Those who understand binary, and those who don't"

[Folke]

Hi Emily

Did you commit the files? I'm really interested in your
implementation. This directory is empty:
http://code.google.com/p/google-web-toolkit-incubator/source/browse/branches/ecc/collectionsReview/src/com/google/gwt/gen2/collection

Folke

On Sep 5, 12:28 am, "Emily Crutcher" <e...@google.com> wrote:
> John,  master of JRE collections, Could you review this?
>
> For big applications, map's performance can end up being a bottleneck.  This
> code review introduces the AbstractJsMap, which is a slightly modified API
> so  that we can create faster map implementations.
>
> The code contains new directories, so here is a read-only branch with the

> code in it:http://code.google.com/p/google-web-toolkit-incubator/source/browse/#...

>
> Attached is the benchmark for putting then getting 1000 through 10,000
> strings, where  HashMap is compared to the new JsStringMap. For the compiled
> put/get benchmark, it is between 300%-500% faster.
>
>   Thanks,
>
>            Emily
>
> --
> "There are only 10 types of people in the world: Those who understand
> binary, and those who don't"
>

>  Mozilla4_003.png
> 22KViewDownload
>
>  report-JsStringMap.xml
> 11KViewDownload

[Emily Crutcher]

Ah, I see what happened, when tortoise makes a branch of the working directory it, logically enough, does not include files which have not been officially added to trunk. The files should be there now.

[Bruce Johnson]

I'm in favor of something much less designed. The extra object (i.e. the internal JSO) seems unnecessarily costly, considering that we're not trying to comply with an existing API such as the JRE Map interface.

Why not avoid an abstract base class that necessitates polymorphism, adapter objects, etc., and instead use a JSO directly?

final class JsMapStringObject<V> extends JavaScriptObject {

  public native V get(String k) /*-{ return this[k]; }-*/; 

  public native void put(String k, V v) /*-{ this[k] = v; }-*/; 

}

This is admittedly less than full-featured and might need a little more (of course), but if the point is to do something special-purpose that's fast, then shouldn't we take it all the way?

In the same vein, we could also create:

JsMapStringInt

JsMapIntObject

JsMapIntInt

...and maybe others...

[Folke]

What happens when you use "prototype" or "length" or "name" as keys? I
remember a FastStringMap somewhere in GWT that prepends each key with
a colon to avoid collision.

On Sep 5, 5:45 am, "Emily Crutcher" <e...@google.com> wrote:
> Ah, I see what happened, when tortoise makes a branch of the working
> directory it, logically enough, does not include files which have not been
> officially added to trunk. The files should be there now.
>
>
>
> On Thu, Sep 4, 2008 at 7:36 PM, Folke <mess...@gmail.com> wrote:
>
> > Hi Emily
>
> > Did you commit the files? I'm really interested in your
> > implementation. This directory is empty:
>

> >http://code.google.com/p/google-web-toolkit-incubator/source/browse/b...

[Thomas Broyer]

On 5 sep, 06:09, "Bruce Johnson" <br...@google.com> wrote:
> I'm in favor of something much less designed. The extra object (i.e. the
> internal JSO) seems unnecessarily costly, considering that we're not trying
> to comply with an existing API such as the JRE Map interface.
> Why not avoid an abstract base class that necessitates polymorphism, adapter
> objects, etc., and instead use a JSO directly?

I suppose because you could cast any JSO to such a "map" and thus
break some assumptions?

@Emily: why not use hasOwnProperty for contains (rather than the "in"
operator)? (and an hasOwnProperty check within "for (k in map)" loops)
Of course this would mean that get() could return a non-null value
while contains() for the same key returns "false"...

> final class JsMapStringObject<V> extends JavaScriptObject {
>   public native V get(String k) /*-{ return this[k]; }-*/;
>   public native void put(String k, V v) /*-{ this[k] = v; }-*/;
>
> }
>
> This is admittedly less than full-featured and might need a little more (of
> course),

such as:
public native boolean has(String k) /*-{ return
this.hasOwnProperty(k); }-*/
(could be "return (k in this)", or there might be both methods: has
and hasOwn)
and:
public native void delete(String k) /*-{ delete this[k]; }-*/;

> but if the point is to do something special-purpose that's fast,
> then shouldn't we take it all the way?

+1 to include such a thing (btw, JsArray restricts values to JSOs, so
either JsMap needs the same restriction, or JsArray has to accept any
Object)

> In the same vein, we could also create:
>
> JsMapStringInt

I would call it JsMapInt, as "keys" in JavaScript are property names,
hence strings only.

> JsMapIntObject
> JsMapIntInt

Er, surely you mean JsArray and JsArrayInteger here? ;-)

(note that jso.cast<JsMap<V>>().get("1") would be strictly equivalent
to jso.cast<JsArray<V>>().get(1), as in JavaScript, array indices are
actually properties that can be evaluated as integers)

[Emily Crutcher]

That's what the weird test in "put" is for, as the current working hypothesis is that removing all keys that appear in the prototype object will prevent this problem.  I went through and tested a bunch of them, so it seems solid, but only time will tell if all the browsers/random word combinations are licked.  This is one of the reasons these classes are going into incubator rather then gwt to begin with.

[Emily Crutcher]

@Emily: why not use hasOwnProperty for contains (rather than the "in"
operator)? (and an hasOwnProperty check within "for (k in map)" loops)
Of course this would mean that get() could return a non-null value
while contains() for the same key returns "false"...

hasOwnProperty is not supported  in Safari 2, so I'm trying to avoid it, though using it in containsKey may be required, in which case it just won't work correctly on the Safari 2 browser.
 

[John Tamplin]

On Fri, Sep 5, 2008 at 9:57 AM, Emily Crutcher <e...@google.com> wrote:

That's what the weird test in "put" is for, as the current working hypothesis is that removing all keys that appear in the prototype object will prevent this problem.  I went through and tested a bunch of them, so it seems solid, but only time will tell if all the browsers/random word combinations are licked.  This is one of the reasons these classes are going into incubator rather then gwt to begin with.

When HashMap was rewritten to use a colon prefix, there was a long discussion about trying to get "clean" objects and the conclusion was that you could not get reliable behavior on all browsers.  If I remember correctly, adding the string "watch" on Firefox was one that couldn't be addressed without it, and there were other problems as well.

--
John A. Tamplin
Software Engineer (GWT), Google

[Joel Webber]

A strong +1 (+2?) to the idea of having a super-fast JsStringMap.

Assuming that John's concern about special cases like 'watch' can be dealt with, I think this would be a valuable addition.

There are a few things that aren't really clear to me though:

1. What's the point of having a FastMap<K,V> that selects its implementation based on web/hosted mode? I can see how the JS implementation might be a little slower in hosted mode, but I'm not sure that this warrants adding that much complexity.

2. Can you actually use an arbitrary object as the key type? Based on the native put() implementation (map[key] = value), I'd guess not. I may be missing something, but I don't see any way to limit the key type as it's currently specified.

3. The whole JSMapAdapter thing seems like it defeats the purpose of having a fast map implementation. If I were ever going to use this, it seems like it would make more sense just to use the regular HashMap, since it's going to be sub-optimal either way.

One other question: Does the implementation of AbstractJsMap.size() really have to be O(N)? If that's the case, we might actually want to avoid having it (or maintain the information some other way), since the whole point is to provide exactly the map interface that can be made fast, whatever those restrictions may be.

[BobV]

On Fri, Sep 5, 2008 at 7:45 AM, Joel Webber <j...@google.com> wrote:
> 2. Can you actually use an arbitrary object as the key type? Based on the
> native put() implementation (map[key] = value), I'd guess not. I may be
> missing something, but I don't see any way to limit the key type as it's
> currently specified.

If you're willing to rely on an implementation detail like this, the
identity hashCode value of an object is based on a per-module counter.
You could use this in web mode, and in hosted mode, just delegate to
an IdentityHashMap.

--
Bob Vawter
Google Web Toolkit Team

[Emily Crutcher]

My experiments seem to say that is no longer true, but I'll double check Firefox 1.5.

[Emily Crutcher]

For those of you who are interested, I've started a draft collection of design principles for the js map collection.  However, don't feel required to read through them, as I'm going to throw together another straw man using them and send it out to the list.

[Bruce Johnson]

Design principles look sensible, although I'm not sure about prioritizing "put" over "get", since "get" happens *so* much more frequently in real apps. If you really want to specify something related to this, maybe you could refine it to, "Multiple contiguous calls to put() must be fast" which could imply (in theory) some sort of lazy accumulation scheme that defers build a true map until the first get() is called. Just a thought.

[Ray Cromwell]

Quick nits: Should probably say something about iteration capability
and what performance considerations apply there (direct methods on
JsMap for iteration, or Iterator-style interface?). Also, is this
going to be isomorphic to identity hash maps in the sense that the
hashCode/equals() of a key won't matter?

-Ray

[Emily Crutcher]

Iteration is an interesting question I was planning to punt on until the design was more stable, as I don't believe we can truly answer that question without having the basic implementation nailed down.

I don't think it's one-to-one comparable with either identity or normal equality maps, as the definition of when two keys are equal will solely rely on the underlying JavaScript interpretation of the keys, and that will be done using the fastest semantics we can get. So for strings it is equality based, as that is  how the underlying JavaScript API treats their strings, but for user class objects it will be identity based, as the fastest way for us to index them is via a extended property added to the object.

[Emily Crutcher]

I wonder if it would be worth creating a Java map with a data collection mode, as how/when the maps are used might make a huge difference to the optimal performance characteristics of the native js map.

[John Tamplin]

On Fri, Sep 5, 2008 at 1:14 PM, Emily Crutcher <e...@google.com> wrote:

I don't think it's one-to-one comparable with either identity or normal equality maps, as the definition of when two keys are equal will solely rely on the underlying JavaScript interpretation of the keys, and that will be done using the fastest semantics we can get. So for strings it is equality based, as that is  how the underlying JavaScript API treats their strings, but for user class objects it will be identity based, as the fastest way for us to index them is via a extended property added to the object.

Note that you can't add properties to some JSOs, so if you rely on that some objects can't be stored in these maps.

[Emily Crutcher]

Good point, I should make it clear that  "user class object" does not include objects that extend JavaScriptObject.

[John Tamplin]

On Fri, Sep 5, 2008 at 1:14 PM, Emily Crutcher <e...@google.com> wrote:

Iteration is an interesting question I was planning to punt on until the design was more stable, as I don't believe we can truly answer that question without having the basic implementation nailed down.

I don't think it's one-to-one comparable with either identity or normal equality maps, as the definition of when two keys are equal will solely rely on the underlying JavaScript interpretation of the keys, and that will be done using the fastest semantics we can get. So for strings it is equality based, as that is  how the underlying JavaScript API treats their strings, but for user class objects it will be identity based, as the fastest way for us to index them is via a extended property added to the object.

I guess for me the big question is what is trying to be accomplished here.  With overlay types, it is pretty trivial to get "bare metal" JS maps, so it seems that this should be something more, with abstractions to make it more "Java-like" while not adding things which make it inefficient.  I think it might be worthwhile to preserve Java equality semantics here by using the hash code as the key into the JS map, and managing collisions there.  Something like:

map = {hash1: [obj1, obj2, obj3], hash2: [obj4], hash3: [obj5, obj6]}

That also gets you away from worring about keys colliding on different (or future) browsers, and you preserve Java equality semantics.  Lookup should still be O(1) since hopefully collisions are rare, iteration is quick, etc.  If a hash function is slow, it would be up to the hash function writer to cache the result in the object as is already best practice, rather than the Map implementation doing that automatically for them.

One other suggestion -- I think rather than optimizing lots of puts in a row you want to allow a JSO to be passed to initialize the map.  From my experience, simply having all the put calls is slow and takes up more space in the generated JS -- the primary way I improved NumberFormat's startup performance was by simply building a JSO directly with the data I needed rather than doing a bunch of puts into a map.  So, for the users that will care abouot this, I think passing in a JSO to initialize the map will be a better choice.

[Emily Crutcher]

I guess for me the big question is what is trying to be accomplished here.  With overlay types, it is pretty trivial to get "bare metal" JS maps, so it seems that this should be something more, with abstractions to make it more "Java-like" while not adding things which make it inefficient.  I think it might be worthwhile to preserve Java equality semantics here by using the hash code as the key into the JS map, and managing collisions there.  Something like:

map = {hash1: [obj1, obj2, obj3], hash2: [obj4], hash3: [obj5, obj6]}

That also gets you away from worring about keys colliding on different (or future) browsers, and you preserve Java equality semantics.  Lookup should still be O(1) since hopefully collisions are rare, iteration is quick, etc.  If a hash function is slow, it would be up to the hash function writer to cache the result in the object as is already best practice, rather than the Map implementation doing that automatically for them.

We can probably prototype different varieties. As it might be interesting to implement one version using the algorithm above and see what the performance characteristics are verses the expando-solution: as creating these classes are dirt simple, what makes creating this API worthwhile is finding the right dirt-simple implementation to use.

One other suggestion -- I think rather than optimizing lots of puts in a row you want to allow a JSO to be passed to initialize the map.  From my experience, simply having all the put calls is slow and takes up more space in the generated JS -- the primary way I improved NumberFormat's startup performance was by simply building a JSO directly with the data I needed rather than doing a bunch of puts into a map.  So, for the users that will care abouot this, I think passing in a JSO to initialize the map will be a better choice.

That is a very interesting idea, as it seems to jive with the goal of optimizing speed at the expense of API time.  

--
John A. Tamplin
Software Engineer (GWT), Google

[Folke]

What about "constructor"? Isn't that one of those properties you
cannot remove completely?

assertNull(newMap.get("constructor")); // "function()"
assertFalse(newMap.containsKey("constructor"));
newMap.put("constructor", "ctor");
assertEquals(1, newMap.size());
assertEquals("ctor", newMap.get("constructor"));
assertTrue(newMap.containsKey("constructor"));
newMap.remove("constructor");
assertNull(newMap.get("constructor")); // "Object()"
assertFalse(newMap.containsKey("constructor"));

With FF3 I can't even get past the first assertNull().

[Emily Crutcher]

Cool! Thanks I will use that in my unit test for the next straw man.

[Emily Crutcher]

John, et, al.

 Here is a revised version of the last JS collections class for review.  Here are the major differences:

• Allows JSStringMap to be initialized with native JavaScriptObjects.
• Introduced Collection_production module that replaces JsStringMap with a native JS object class when the user wants to be in production mode.  This required the following changes
• Hide the JsStringMap constructor, instead you use JsStringMap.create() to create JsStringMap instance
• Introduce super package to incubator
• Hide the AbstractJsMap class.
• Removed FastMap wrapper, replaced it with WrappedJsStringMap

The benchmarks are somewhat interesting, as it turns out that memory allocation from one can serious screw up the others. Therefore here I've included each variant as separate benchmarks posted on the JsMap wiki page.  

[John Tamplin]

On Wed, Sep 10, 2008 at 6:29 PM, Emily Crutcher <e...@google.com> wrote:

The benchmarks are somewhat interesting, as it turns out that memory allocation from one can serious screw up the others. Therefore here I've included each variant as separate benchmarks posted on the JsMap wiki page.  

I haven't looked at the code yet, but what about non-IE benchmarks?  Also, it looks like these results show linear time, and don't specify exactly what is being benchmarked -- unless you are including iteration in there, I would expect any hashmap benchmarks to be sub-linear.

[Emily Crutcher]

Are you having trouble with the patch? The benchmark used is included in it.   The benchmark varies by the number of puts and gets, so 500 = 500 puts + 500 gets, so would have been suprised to see  sub-linear time.

 

 

IE is the primary motivator for this set of changes, as it has the slowest JS system and is most sensitive to memory constraints, so the IE benchmarks are the ones I am focusing on. 

[John Tamplin]

On Thu, Sep 11, 2008 at 9:18 AM, Emily Crutcher <e...@google.com> wrote:

Are you having trouble with the patch? The benchmark used is included in it.   The benchmark varies by the number of puts and gets, so 500 = 500 puts + 500 gets, so would have been suprised to see  sub-linear time.

As I said, I hadn't looked at the code yet, just the benchmark page.  I assumed the number was the size of the hash map, as that is what most of our data structure benchmarks use.

Anyway, my main point is that someone just looking at the Wiki isn't going to know what it means so there should be additional text describing what those charts are -- sorry I wasn't more clear.
 

IE is the primary motivator for this set of changes, as it has the slowest JS system and is most sensitive to memory constraints, so the IE benchmarks are the ones I am focusing on. 

That's fine, but it would be helpful to know that there isn't some issue on the other browsers.  As an example, remember the pathological case of arrays on Safari when the index exceeds a certain threshold.

[Emily Crutcher]

 
Anyway, my main point is that someone just looking at the Wiki isn't going to know what it means so there should be additional text describing what those charts are -- sorry I wasn't more clear.

Ah! Yes, that makes perfect sense, wiki has been updated. 

 

IE is the primary motivator for this set of changes, as it has the slowest JS system and is most sensitive to memory constraints, so the IE benchmarks are the ones I am focusing on. 

That's fine, but it would be helpful to know that there isn't some issue on the other browsers.  As an example, remember the pathological case of arrays on Safari when the index exceeds a certain threshold.

Yep, I figured that would be the next phase, i.e. if this design was accepted I would polish up the code (more javadocs etc, some extra tests), do the full suite of benchmarks to check for weird stand-out cases then submit for final review. As that will all take a bit of time, I don't want to do it until I know the design has been stabilized though.

 

--
John A. Tamplin
Software Engineer (GWT), Google

[Emily Crutcher]

Just a ping for you to look at the code/design...

[John Tamplin]

This is not a complete review, but some initial feedback:

• Shouldn't the selenium changes be part of a separate patch?  Regardless, Freeland or Eric would be better to review that portion.
  
• Did you read the GWTC threads about trying to do this in GWT 1.4?  The consensus was that it was not safe to rely on being able to remove all properties, and that even if it were possible it would be fragile to future browser changes (the second thread has a test HTML file, though it is missing __proto__).  I think you will need to rely on a prefix, just as we do in FastStringMap.  At there very least, you should include "__proto__", "prototype", "constructor", "toString", "watch", "unwatch", and "valueOf" in your test.  An alternative would be to not attempt to clean up things like that and simply document that no JS-defined property can be used in the string, though that seems problematic.
  
• Are you sure the complexity regarding a separate hosted-mode implementation is justified?  Aside from more opportunities for errors, it means two separate implementations are not tested to be exactly the same, so any holes in the tests are an opportunity for divergent behavior in hosted mode.
• Missing newlines at the end of all the .gwt.xml files.
  

[Emily Crutcher]

On Thu, Sep 18, 2008 at 3:33 PM, John Tamplin <j...@google.com> wrote:

This is not a complete review, but some initial feedback:

• Shouldn't the selenium changes be part of a separate patch?  Regardless, Freeland or Eric would be better to review that portion.

Yep, that was already committed to gwt-incubator trunk, the patch is older then the commit.
 

• 
  
• Did you read the GWTC threads about trying to do this in GWT 1.4?  The consensus was that it was not safe to rely on being able to remove all properties, and that even if it were possible it would be fragile to future browser changes (the second thread has a test HTML file, though it is missing __proto__).  I think you will need to rely on a prefix, just as we do in FastStringMap.  At there very least, you should include "__proto__", "prototype", "constructor", "toString", "watch", "unwatch", and "valueOf" in your test.  An alternative would be to not attempt to clean up things like that and simply document that no JS-defined property can be used in the string, though that seems problematic. 

What code are you looking at? I'm not removing any properties and already testing watch.
 

• Are you sure the complexity regarding a separate hosted-mode implementation is justified?  Aside from more opportunities for errors, it means two separate implementations are not tested to be exactly the same, so any holes in the tests are an opportunity for divergent behavior in hosted mode.

Did you have a chance to run the benchmark I sent you?
 

• Missing newlines at the end of all the .gwt.xml files.

Our gwt.xml are supposed to have an extra new line at the end of them?

 

• 
  

--
John A. Tamplin
Software Engineer (GWT), Google

[John Tamplin]

On Thu, Sep 18, 2008 at 3:44 PM, Emily Crutcher <e...@google.com> wrote:

Yep, that was already committed to gwt-incubator trunk, the patch is older then the commit.

Ok, so I should ignore that, right?

• Did you read the GWTC threads about trying to do this in GWT 1.4?  The consensus was that it was not safe to rely on being able to remove all properties, and that even if it were possible it would be fragile to future browser changes (the second thread has a test HTML file, though it is missing __proto__).  I think you will need to rely on a prefix, just as we do in FastStringMap.  At there very least, you should include "__proto__", "prototype", "constructor", "toString", "watch", "unwatch", and "valueOf" in your test.  An alternative would be to not attempt to clean up things like that and simply document that no JS-defined property can be used in the string, though that seems problematic. 

What code are you looking at? I'm not removing any properties and already testing watch.

 The JsMap.patch you sent (I duplicated watch, but the others don't appear to be tested).  In particular, __proto__ was determined to be the showstopper before.

• Are you sure the complexity regarding a separate hosted-mode implementation is justified?  Aside from more opportunities for errors, it means two separate implementations are not tested to be exactly the same, so any holes in the tests are an opportunity for divergent behavior in hosted mode.

Did you have a chance to run the benchmark I sent you? 

Not yet, but the performance in OOPHM is not terribly relevant since this will be in long before OOPHM is.

• Missing newlines at the end of all the .gwt.xml files.

Our gwt.xml are supposed to have an extra new line at the end of them?

Every line should be terminated, but here the last one isn't.

[Emily Crutcher]

Ok, so I should ignore that, right?

Yep.

• Did you read the GWTC threads about trying to do this in GWT 1.4?  The consensus was that it was not safe to rely on being able to remove all properties, and that even if it were possible it would be fragile to future browser changes (the second thread has a test HTML file, though it is missing __proto__).  I think you will need to rely on a prefix, just as we do in FastStringMap.  At there very least, you should include "__proto__", "prototype", "constructor", "toString", "watch", "unwatch", and "valueOf" in your test.  An alternative would be to not attempt to clean up things like that and simply document that no JS-defined property can be used in the string, though that seems problematic. 

What code are you looking at? I'm not removing any properties and already testing watch.

 The JsMap.patch you sent (I duplicated watch, but the others don't appear to be tested).  In particular, __proto__ was determined to be the showstopper before.

good, so we're looking at the same code :-).    I'll double check the others.  As far as I know, the previous conversations on GWT-contrib had focused around removing a specific list of  keywords, the technique used here is to test to make sure the key is defined in the current object, not its prototype. 

• Are you sure the complexity regarding a separate hosted-mode implementation is justified?  Aside from more opportunities for errors, it means two separate implementations are not tested to be exactly the same, so any holes in the tests are an opportunity for divergent behavior in hosted mode.

Did you have a chance to run the benchmark I sent you? 

Not yet, but the performance in OOPHM is not terribly relevant since this will be in long before OOPHM is.

This collection set is targeted for GWT 2.0, not GWT 1.6, so I do think it is relevent.  If you could run it, I'd find it very useful in determining if we need a hosted mode implementation.



 

• Missing newlines at the end of all the .gwt.xml files.

Our gwt.xml are supposed to have an extra new line at the end of them?

Every line should be terminated, but here the last one isn't.

Makes sense, thanks.

 

--
John A. Tamplin
Software Engineer (GWT), Google

[Damon Lundin]

I realize I may be jumping in late to this discussion, but Bruce just
introduced me to this fine contributors group. At a talk during the
Google I/O conference, I mentioned the benefit of using the GWT
FastStringMap implementation over the HashMap implementation and I
decided to re-run my analysis for 1.5 to see if the 1.5 compiler was
smart enough to pick a faster implementation and unfortunately that
does not appear to be the case. I hope I'm not hijacking Emily's
thread, but I just thought some of you would find my (basic) data
interesting for HashMap vs. FastStringMap.

I posted my results up on my company development blog if you don't
mind me not copy-and-pasting it all here:

http://development.lombardi.com/?p=95

[Emily Crutcher]

The place we can run into trouble using the FastStringMap techniques is with IE6 when there are a lot of other JS objects, because of its less-then-optimal garbage collection algorithms. However, IE has the nice property it doesn't lie to you about the value of isOwnProperty(), so we can use an alternative js string map implementation for IE. 

I have also found that using a JsUniqueIdMap seems significantly faster  then hash map, though I still need to prove that with more benchmarks, and is certainly smaller.