Opened 2 years ago

Last modified 8 months ago

#6559 closed change

Use maps and sets where appropriate — at Version 15

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

https://codereview.adblockplus.org/29743730/
https://codereview.adblockplus.org/29745555/
https://codereview.adblockplus.org/29800557/
https://codereview.adblockplus.org/29802587/
https://codereview.adblockplus.org/29852555/

Description (last modified by mjethani)

Background

Core uses an object with a null prototype for a map or a set in a few places. For example, in lib/elemHideEmulation.js:

let filters = Object.create(null);

function addFilter(filter)
{
  filters[filter.text] = true;
}

With the upgrade to ES2015 the code could use Map and Set objects instead.

What to change

In lib/downloader.js, convert _downloading to a Set.

In lib/elemHide.js, convert filterKeyBySelector and exceptions each to a Map, convert knownExceptions to a Set.

In lib/elemHideEmulation.js, convert filters to a Set.

Integration notes

With changeset 212fe705ab82, FilterStorage.knownSubscriptions is now a Map object (#6739).

With changeset ae6bf93f1856, the type of SpecialSubscription.defaultsMap is now Map rather than Object.

Change History (15)

comment:1 Changed 2 years ago by sergz

JIC, it's absolutely fine for me and I like to see the work in that direction, however as the next step (it should not be a blocker) it will be very interesting to measure how it affects the memory consumption. In that regard I would like to note that one should rather pay attention to measure not mere JS heap but also RSS, what is essential for us.

comment:2 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 6559 - Use maps and sets where appropriate

comment:3 Changed 2 years ago by kzar

I'm seeing some filterListener.js test failures since that last commit Manish, could you take a look?

comment:4 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 6559 - Fix tests for changeset 662384d3b0ad

comment:5 Changed 2 years ago by mjethani

  • Review URL(s) modified (diff)

comment:6 Changed 2 years ago by jsonesen

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

comment:7 follow-up: Changed 2 years ago by kzar

  • Cc jsonesen added
  • Priority changed from Unknown to P3

I still see more instances of Object.create(null) in the core code, did you check that none of those can be converted to Maps / Sets before closing?

comment:8 in reply to: ↑ 7 Changed 2 years ago by jsonesen

Replying to kzar:

I still see more instances of Object.create(null) in the core code, did you check that none of those can be converted to Maps / Sets before closing?

closed it on accident, I meant to set it as reviewing.

comment:9 Changed 2 years ago by kzar

  • Resolution fixed deleted
  • Status changed from closed to reopened

Ah right, no worries. I'll reopen in that case.

comment:10 Changed 2 years ago by mjethani

  • Review URL(s) modified (diff)

comment:11 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 6559 - Use Map object for known subscriptions

comment:12 Changed 2 years ago by mjethani

Reported #6739 in UI for the last change.

comment:13 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 6559 - Change SpecialSubscription.defaultsMap to Map object

comment:14 Changed 2 years ago by mjethani

  • Description modified (diff)

comment:15 Changed 2 years ago by mjethani

  • Description modified (diff)
  • Review URL(s) modified (diff)
Note: See TracTickets for help on using tickets.