Opened 8 months ago

Last modified 4 months ago

#6559 reviewing change

Use maps and sets where appropriate

Reported by: mjethani Assignee: mjethani
Priority: P2 Milestone:
Module: Core Keywords:
Cc: kzar, sergz, jsonesen, greiner Blocked By:
Blocking: #6425, #6753 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.

In lib/filterStorage.js, convert FilterStorage.knownSubscriptions to a Map.

In lib/subscriptionClasses.js, convert Subscription.knownSubscriptions to a Map.

In lib/subscriptionClasses.js, convert SpecialSubscription.defaultsMap to a Map.

In lib/filterClasses.js, convert Filter.prototype.subscriptions 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.

With changeset 03c07a4ba673, the type of the subscriptions property of the Filter class is Set rather than Array. This would require a change in UI in messageResponder.js (and possibly elsewhere) (#6876) and in the extension in lib/whitelisting.js.

Hints for testers

Test that the downloading and loading of subscriptions works correctly.

Test that element hiding and element hiding emulation filters, with and without exceptions, work correctly.

Test that filters from all types of subscriptions are loaded and saved correctly. For example, add only one filter ##.foo to the "My filters" box; then restart the browser; the filter should still be there. Try with multiple filters. Likewise, pick random filters from EasyList+AA to try out between browser restarts and see that the filters always work.

Change History (33)

comment:1 Changed 8 months 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 8 months ago by abpbot

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

comment:3 Changed 8 months ago by kzar

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

comment:4 Changed 8 months ago by abpbot

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

comment:5 Changed 8 months ago by mjethani

  • Review URL(s) modified (diff)

comment:6 Changed 6 months ago by jsonesen

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

comment:7 follow-up: Changed 6 months 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 6 months 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 6 months 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 6 months ago by mjethani

  • Review URL(s) modified (diff)

comment:11 Changed 6 months ago by abpbot

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

comment:12 Changed 6 months ago by mjethani

Reported #6739 in UI for the last change.

comment:13 Changed 6 months ago by abpbot

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

comment:14 Changed 6 months ago by mjethani

  • Description modified (diff)

comment:15 Changed 6 months ago by mjethani

  • Description modified (diff)
  • Review URL(s) modified (diff)

comment:16 Changed 6 months ago by mjethani

  • Priority changed from P3 to P2
  • Ready set

comment:17 Changed 6 months ago by mjethani

  • Status changed from reopened to reviewing

comment:18 Changed 6 months ago by mjethani

  • Description modified (diff)

comment:19 Changed 6 months ago by greiner

  • Blocking 6739 added

comment:20 Changed 6 months ago by wspee

  • Blocking 6753 added

comment:21 Changed 5 months ago by greiner

  • Blocking 6739 removed

comment:22 Changed 5 months ago by mjethani

  • Description modified (diff)

comment:23 Changed 4 months ago by mjethani

  • Description modified (diff)

comment:24 Changed 4 months ago by mjethani

  • Review URL(s) modified (diff)

comment:25 Changed 4 months ago by abpbot

comment:26 Changed 4 months ago by mjethani

  • Description modified (diff)

comment:27 follow-ups: Changed 4 months ago by mjethani

  • Cc greiner added

@greiner Filter.prototype.subscriptions is a Set object now. You would have to make a change in UI in messageResponder.js and possibly elsewhere.

Last edited 4 months ago by mjethani (previous) (diff)

comment:28 in reply to: ↑ 27 Changed 4 months ago by mjethani

Replying to mjethani:

@greiner Filter.prototype.subscriptions is a Set object now.

This would also require a change in the web extension in lib/whitelisting.js.

comment:29 Changed 4 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Subscription downloading, element hiding and emulation filters (with exceptions), and storing of filters seem to be working as expected.

ABP 3.2.0.2103
Chrome 68 / 55 / 49 / Windows 10
Firefox 61 / 55 / 51 / Windows 10

comment:30 in reply to: ↑ 27 ; follow-up: Changed 4 months ago by greiner

Replying to mjethani:

@greiner Filter.prototype.subscriptions is a Set object now. You would have to make a change in UI in messageResponder.js and possibly elsewhere.

I've created #6876 to apply that change to adblockplusui. Is this change going to be included in the very next extension release already?

comment:31 in reply to: ↑ 30 Changed 4 months ago by mjethani

Replying to greiner:

Replying to mjethani:

@greiner Filter.prototype.subscriptions is a Set object now. You would have to make a change in UI in messageResponder.js and possibly elsewhere.

I've created #6876 to apply that change to adblockplusui. Is this change going to be included in the very next extension release already?

It should be in the next proper release (3.4), but not in the next "snippets-only" or hotfix release (3.3.x). If by "very next" you meant 3.3, then no.

comment:32 Changed 4 months ago by mjethani

  • Description modified (diff)

comment:33 Changed 4 months ago by mjethani

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