Use maps and sets where appropriate

[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.

[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.

[abpbot]

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

[kzar]

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

[abpbot]

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

[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?

[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.

[kzar]

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

[abpbot]

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

[mjethani]

Reported #6739 in UI for the last change.

[abpbot]

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

[abpbot]

A commit referencing this issue has landed:
​Issue 6559 - Convert subscriptions property of Filter class to a set

[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.

[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.

[Ross]

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

[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?

[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.

[mjethani]

The work here is done. I am closing this ticket.