Remove subscriptions property of Filter object

[mjethani]

Background

This is another idea based on Sebastian's comment on #6916.

When a new filter is added in lib/filterListener.js, we check if the filter is part of any enabled subscriptions before adding the filter to one of the "containers" (lib/matcher.js, lib/elemHide.js, etc.). For each filter, right now this is just iteration over a Set object, and after #6916 it is just directly a single Subscription object for the default case. An alternative way to do this would be to look up the Filter object in each enabled Subscription object. This would almost certainly be slower; but if this makes the subscriptions property of the Filter class redundant, it could speed up the parsing of the patterns.ini (skipping the call to the addSubscription method).

Additionally, while looking up the filter in a subscription would be infeasible if it is a list (array), it may be feasible if it is a set. Making it a set would change the current semantics, in that a filter would be able to appear only once in a subscription. Since (1) the action of a filter (blocking requests or hiding elements) is ​idempotent and (2) the ordering of filters in a subscription is of no significance, the list of filters could be a set instead. The only caveat here is snippet filters (#6538), which may have "side" effects in the document and the ordering of which may be of some significance.

This idea is worth exploring.

What to change

See ​patch.

Integration notes

The Filter class no longer has a subscriptions property. In order to get the subscriptions for a filter, use filterStorage.subscriptions(filter.text) (also a generator) instead of filter.subscriptions().

Note that filterStorage.subscriptions(filter.text) will look for the filter in each known subscription, which is slower. If this is required to be done frequently in any part of your code, it may be worth maintaining your own cache of filters to their subscriptions, but you would have to invalidate the cache any time the relationship between filters and subscriptions changes (e.g. a new subscription is added). Use one of the filter events for this. If it becomes cumbersome, please ask us to implement it in adblockpluscore using a different API.

In adblockpluschrome, lib/firefoxDataCleanup.js accesses Filter.knownFilters, but this object will no longer contain filters from disabled subscriptions on startup. Accordingly, either update the code or do something else (see comment:11).

Hints for testers

Use the hints in #7094 and ​core#10.

[BrentM]

The AdBlock filter list data collection processing does reference the subscription information when a Filter gets a hit (specifically the URL of the filter list). The subscription information was requested by Stephan Porz.

[mjethani]

Thanks, Brent.

I think we'll always have a way to access a filter's subscriptions (see also the more pertinent #7096), the only question is whether we can get rid of this property, which maintains a permanent list, and replace it with something that is dynamically generated when needed.

Is the list of subscriptions for a filter always needed or is it only needed when the DevTools panel is open, as in the case of ABP?

[mjethani]

After playing around with this idea a lot, I have concluded that it's OK to get rid of the subscriptions method of the Filter class. The replacement for filter.subscriptions() would be filterStorage.subscriptions(filter.text). In my tests the performance was generally OK or even better with this change.

I have uploaded a ​patch.

Note: This will be another step towards making filter objects lightweight/stateless/ephemeral (#7097).

[mjethani]

My only concern with the current patch, which is already LGTM'd but I am not landing it, is that Filter.knownFilters will no longer include filters from disabled subscriptions. I did not think that Filter.knownFilters was a public API so I figured it did not matter. But lib/firefoxDataCleanup.js in adblockpluschrome does indeed access it.

@sebastian does Filter.knownFilters need to contain filters from disabled subscriptions for that use case? If yes, we will have to think of a workaround. In the worst case, for this change only, we can go ahead and create the object anyway even though the subscription is disabled (see ​https://codereview.adblockplus.org/30013628/diff/30016567/lib/iniParser.js).

[sebastian]

Let's just remove lib/firefoxDataCleanup.js. That code is only used when updating from Adblock Plus 2.9.x for Firefox. I don't think that we need to support this scenario anymore. We have removed other migration code, after a much shorter transition period in the past.

[mjethani]

Alright, in that case I'll go ahead and land this change. Thanks!

[abpbot]

A commit referencing this issue has landed:
​Issue 7029 - Remove subscriptions property of Filter object

[Ross]

Done. Working as described in #7094.

ABP 0.9.15.2340
Microsoft Edge 44.17763.1.0 / Windows 10 1809

ABP 3.5.2.2340
Chrome 49.0.2623.75 / Windows 10 1809
Chrome 75.0.3770.142 / Windows 10 1809
Opera 36.0.2130.65 / Windows 10 1809
Opera 62.0.3331.72 / Windows 10 1809
Firefox 51.0 / Windows 10 1809
Firefox 68.0 / Windows 10 1809
Firefox Mobile 68.0 / Android 7.2.2