Opened 4 years ago

Closed 2 years ago

#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)

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)

Change History (8)

comment:1 Changed 4 years ago by tschuster

  • Description modified (diff)

comment:2 Changed 4 years ago 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 4 years ago 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 4 years ago 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 4 years ago 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 2 years ago by fhd

  • Cc trev removed

comment:7 Changed 2 years ago 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 2 years ago 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.

Note: See TracTickets for help on using tickets.