Opened on 09/17/2015 at 06:20:29 PM

Closed on 01/05/2018 at 04:02:30 PM

#3095 closed change (duplicate)

Use a JavaScript Map in ElemHide

Reported by: tschuster Assignee:
Priority: Unknown Milestone:
Module: Core Keywords:
Cc: greiner, sergz, kzar Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by tschuster)


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.


│   │  ├──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)


│   │  │  │  └───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)

Attachments (0)

Change History (8)

comment:1 Changed on 09/17/2015 at 06:38:39 PM by tschuster

  • Description modified (diff)

comment:2 Changed on 09/24/2015 at 10:58:20 AM by greiner

  • Cc greiner trev added
  • Component changed from Unknown to Core

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().

comment:3 Changed on 09/24/2015 at 11:16:31 AM by 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?

comment:4 follow-up: Changed on 10/01/2015 at 11:50:59 AM by 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.

comment:5 in reply to: ↑ 4 Changed on 10/01/2015 at 11:55:33 AM by 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.

comment:6 Changed on 12/21/2017 at 11:28:41 AM by fhd

  • Cc trev removed

comment:7 Changed on 01/04/2018 at 11:33:51 AM by sergz

  • Cc sergz kzar added
  • Owner tschuster deleted
  • Platform changed from Firefox to Unknown / Cross platform

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.

comment:8 Changed on 01/05/2018 at 04:02:30 PM by kzar

  • Resolution set to duplicate
  • Status changed from new to closed

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.

Add Comment

Modify Ticket

Change Properties
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from (none).
Note: See TracTickets for help on using tickets.