Use a JavaScript Map in ElemHide

[tschuster]

Background

Maps are a new standard feature for doing key-value maps. Right now we use simple objects, which is not necessary and might use a bit more memory as well.

What to change

In elemHide.js replace filterByKey and keyByFilter with Maps. This seems to be a net memory win of a few percent, sadly not much. The biggest win comes from having smaller "dictionaries" that were used by the JS engine internally to optimize the two objects.

before:

│   │  ├──13.32 MB (06.35%) -- class(Object)
│   │  │  ├───7.48 MB (03.57%) -- shapes
│   │  │  │   ├──5.29 MB (02.52%) -- gc-heap
│   │  │  │   │  ├──5.21 MB (02.48%) ── dict
│   │  │  │   │  └──0.09 MB (00.04%) -- (2 tiny)

after:

│   │  │  │  └───6.16 MB (02.94%) -- shapes
│   │  │  │      ├──4.06 MB (01.94%) -- gc-heap
│   │  │  │      │  ├──3.90 MB (01.87%) ── dict
│   │  │  │      │  └──0.16 MB (00.08%) -- (2 tiny)
│   │  │  │      │     ├──0.14 MB (00.07%) ── tree

Furthermore it would be nice if we could change the keyByFilter map to have filter objects as keys instead of the filter.text. However right now this might actually functions as a way to avoid duplicate filters (i.e. different filter objects, but the same text)

[greiner]

Note that this change will also affect non-Gecko versions. Unfortunately, ​according to MDN Map has only been available in Chrome since version 38 and in Safari since version 7.1 but we're still supporting older versions of those browsers.

Polyfilling Map for browsers that don't support it yet could be an acceptable solution since JSHydra currently doesn't convert Map and even if, would potentially have a negative impact on the already subpar performance of ElemHide.getSelectorsForDomain().

[trev]

I don't see a big issue with requiring Chrome 38 given that the current release is Chrome 45. Safari is a problem however, the upgrade behavior there is outright horrible. So this change is indeed non-trivial. We would also have to verify that libadblockplus supports Map (I think it does). We could make JSHydra convert maps into plain objects but replacing foo.get(key) by foo[key] blindly sounds like a footgun - how can JSHydra know that foo is really a map?

[tschuster]

Can't we just use normal web practices and polyfill a Map implementation when it doesn't exist? Detecting this just with JSHydra seems indeed quite error prone.

[greiner]

Replying to tschuster:

Can't we just use normal web practices and polyfill a Map implementation when it doesn't exist? Detecting this just with JSHydra seems indeed quite error prone.

I agree (see my comment above). Without sacrificing compabitility it would have a negative impact only on older browser versions but would give us the performance benefit in versions that support it.

[sergz]

It seems it's already implemented even at bigger scale and BTW the type of filterByKey is array because the type of the key is number.

What do you think about either closing of this issue or precising? There still can be places where switching to Map could be beneficial, however according to my experience a careful profiling is strictly required before landing such changes.

[kzar]

Yes, this is a duplicate of #5735.

FWIW we originally we didn't use Maps for these lookups due to Safari, but IIRC their performance was otherwise better and so I'm glad we're using Maps for them now.