Opened on 10/09/2018 at 04:42:23 PM

Closed on 04/27/2019 at 09:48:23 AM

Last modified on 07/25/2019 at 01:58:00 PM

#7029 closed change (fixed)

Remove subscriptions property of Filter object

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

Description (last modified by mjethani)


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.

Attachments (0)

Change History (21)

comment:1 Changed on 10/09/2018 at 04:42:48 PM by mjethani

  • Component changed from Unknown to Core

comment:2 Changed on 10/09/2018 at 04:49:50 PM by greiner

  • Cc greiner added

comment:3 Changed on 10/09/2018 at 04:57:23 PM by mjethani

  • Cc jsonesen added

comment:4 Changed on 10/09/2018 at 05:00:12 PM by sergz

  • Cc sergz added

comment:5 Changed on 11/19/2018 at 07:23:49 PM by 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.

comment:6 Changed on 11/20/2018 at 02:21:55 AM by 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?

comment:7 Changed on 02/26/2019 at 12:35:12 PM by 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).

comment:8 Changed on 02/26/2019 at 12:36:03 PM by mjethani

  • Review URL(s) modified (diff)

comment:9 Changed on 02/26/2019 at 12:36:13 PM by mjethani

  • Owner set to mjethani

comment:10 Changed on 04/06/2019 at 08:47:07 PM by mjethani

  • Cc sebastian kzar added

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

comment:11 Changed on 04/07/2019 at 12:37:53 AM by 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.

Last edited on 04/07/2019 at 12:38:10 AM by sebastian

comment:12 Changed on 04/07/2019 at 11:49:16 AM by mjethani

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

comment:13 Changed on 04/07/2019 at 11:57:58 AM by abpbot

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

comment:14 Changed on 04/07/2019 at 12:07:31 PM by mjethani

  • Description modified (diff)

comment:15 Changed on 04/07/2019 at 12:09:05 PM by mjethani

  • Summary changed from Remove Filter.prototype.subscriptions if possible to Remove subscriptions property of Filter object

comment:16 Changed on 04/07/2019 at 12:09:23 PM by mjethani

  • Ready set
  • Status changed from new to reviewing

comment:17 Changed on 04/07/2019 at 03:23:35 PM by mjethani

  • Review URL(s) modified (diff)

comment:18 Changed on 04/08/2019 at 11:20:20 AM by greiner

  • Blocking 7448 added

comment:19 Changed on 04/09/2019 at 02:47:59 AM by mjethani

  • Blocking 7097 added

comment:20 Changed on 04/27/2019 at 09:48:23 AM by mjethani

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:21 Changed on 07/25/2019 at 01:58:00 PM by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Working as described in #7094.

Microsoft Edge 44.17763.1.0 / Windows 10 1809

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

Add Comment

Modify Ticket

Change Properties
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from mjethani.
Note: See TracTickets for help on using tickets.