Opened 11 months ago

Closed 4 months ago

Last modified 4 weeks ago

#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):

https://codereview.adblockplus.org/30013628/
https://gitlab.com/eyeo/adblockplus/adblockpluscore/merge_requests/50

Description (last modified by 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.

Change History (21)

comment:1 Changed 11 months ago by mjethani

  • Component changed from Unknown to Core

comment:2 Changed 11 months ago by greiner

  • Cc greiner added

comment:3 Changed 11 months ago by mjethani

  • Cc jsonesen added

comment:4 Changed 11 months ago by sergz

  • Cc sergz added

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

  • Review URL(s) modified (diff)

comment:9 Changed 6 months ago by mjethani

  • Owner set to mjethani

comment:10 Changed 5 months ago 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 https://codereview.adblockplus.org/30013628/diff/30016567/lib/iniParser.js).

comment:11 Changed 5 months ago 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 5 months ago by sebastian (previous) (diff)

comment:12 Changed 5 months ago by mjethani

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

comment:13 Changed 5 months ago by abpbot

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

comment:14 Changed 5 months ago by mjethani

  • Description modified (diff)

comment:15 Changed 5 months ago by mjethani

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

comment:16 Changed 5 months ago by mjethani

  • Ready set
  • Status changed from new to reviewing

comment:17 Changed 5 months ago by mjethani

  • Review URL(s) modified (diff)

comment:18 Changed 5 months ago by greiner

  • Blocking 7448 added

comment:19 Changed 5 months ago by mjethani

  • Blocking 7097 added

comment:20 Changed 4 months ago by mjethani

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

comment:21 Changed 4 weeks ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

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

Note: See TracTickets for help on using tickets.