Opened 7 months ago

Last modified 2 weeks ago

#7029 reviewing change

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: Unknown Verified working: no
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

[TBD]

Change History (19)

comment:1 Changed 7 months ago by mjethani

  • Component changed from Unknown to Core

comment:2 Changed 7 months ago by greiner

  • Cc greiner added

comment:3 Changed 7 months ago by mjethani

  • Cc jsonesen added

comment:4 Changed 7 months ago by sergz

  • Cc sergz added

comment:5 Changed 5 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 5 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 8 weeks 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 8 weeks ago by mjethani

  • Review URL(s) modified (diff)

comment:9 Changed 8 weeks ago by mjethani

  • Owner set to mjethani

comment:10 Changed 2 weeks 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 2 weeks 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 2 weeks ago by sebastian (previous) (diff)

comment:12 Changed 2 weeks ago by mjethani

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

comment:13 Changed 2 weeks ago by abpbot

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

comment:14 Changed 2 weeks ago by mjethani

  • Description modified (diff)

comment:15 Changed 2 weeks ago by mjethani

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

comment:16 Changed 2 weeks ago by mjethani

  • Ready set
  • Status changed from new to reviewing

comment:17 Changed 2 weeks ago by mjethani

  • Review URL(s) modified (diff)

comment:18 Changed 2 weeks ago by greiner

  • Blocking 7448 added

comment:19 Changed 2 weeks ago by mjethani

  • Blocking 7097 added
Note: See TracTickets for help on using tickets.