Issue 7029 - Remove subscriptions property of Filter object

[Manish Jethani, 2019-02-26 12:29:57]

Patch Set 1

https://codereview.adblockplus.org/30013628/diff/30016567/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/30013628/diff/30016567/lib/filterListener....
lib/filterListener.js:137: * @param {?Array.<Subscription>} [subscriptions]
subscriptions to which the
A filter needs only _one_ enabled subscription for it to be added. When we
already have such a subscription for the filter, we just pass `[subscription]`
to this function (see elsewhere in this file).

https://codereview.adblockplus.org/30013628/diff/30016567/lib/filterStorage.js
File lib/filterStorage.js (left):

https://codereview.adblockplus.org/30013628/diff/30016567/lib/filterStorage.j...
lib/filterStorage.js:201: disconnectSubscriptionFilters(subscription,
oldFilterText);
You might think that looking up the subscriptions for a filter makes this more
expensive, but actually since we no longer have to disconnect/connect filters
from/to their subscriptions, this is actually faster in practice as long as you
have a reasonable number of subscriptions (I tried with 5).

https://codereview.adblockplus.org/30013628/diff/30016567/lib/subscriptionCla...
File lib/subscriptionClasses.js (right):

https://codereview.adblockplus.org/30013628/diff/30016567/lib/subscriptionCla...
lib/subscriptionClasses.js:71: * A searchable index of filter text in the filter
subscription.
We might be able to find a better way to do this than to use a Set object, but
this is not too expensive either.

https://codereview.adblockplus.org/30013628/diff/30016567/lib/subscriptionCla...
lib/subscriptionClasses.js:164: if (!this._filterTextIndex)
We build the index on first call. Unfortunately we never clear it until the list
of filters changes again. Maybe there's a better way, but again this is not too
expensive in terms of memory usage.

https://codereview.adblockplus.org/30013628/diff/30016567/lib/subscriptionCla...
lib/subscriptionClasses.js:186: findFilterIndex(filter, fromIndex = 0)
While we're at it, let's rename this function to something that makes more
sense. `searchFilter` implies that we are searching a filter; actually we're
searching a subscription for the index of a filter. `findFilterIndex` seems more
appropriate.

https://codereview.adblockplus.org/30013628/diff/30016567/test/filterStorage_...
File test/filterStorage_readwrite.js (left):

https://codereview.adblockplus.org/30013628/diff/30016567/test/filterStorage_...
test/filterStorage_readwrite.js:187: // Make sure the relationships between
filters and subscriptions are set
This doesn't make sense anymore.

[hub, 2019-03-08 21:36:20]

https://codereview.adblockplus.org/30013628/diff/30016567/lib/filterStorage.js
File lib/filterStorage.js (left):

https://codereview.adblockplus.org/30013628/diff/30016567/lib/filterStorage.j...
lib/filterStorage.js:201: disconnectSubscriptionFilters(subscription,
oldFilterText);
On 2019/02/26 12:29:57, Manish Jethani wrote:
> You might think that looking up the subscriptions for a filter makes this more
> expensive, but actually since we no longer have to disconnect/connect filters
> from/to their subscriptions, this is actually faster in practice as long as
you
> have a reasonable number of subscriptions (I tried with 5).

my default config has 6. Add one language, that make 7. And then if I remember,
each user filter is a special subscription.

https://codereview.adblockplus.org/30013628/diff/30016567/lib/subscriptionCla...
File lib/subscriptionClasses.js (right):

https://codereview.adblockplus.org/30013628/diff/30016567/lib/subscriptionCla...
lib/subscriptionClasses.js:207: this._filterTextIndex = null;
Shouldn't it be more efficiant that we add the `filter.text` to
`this._filterTextIndex` (and remove in deleteFilterAt) instead of deleting it?
This would mean hasFilterText wouldn't need to create it.

[Manish Jethani, 2019-03-30 20:50:35]

https://codereview.adblockplus.org/30013628/diff/30016567/lib/filterStorage.js
File lib/filterStorage.js (left):

https://codereview.adblockplus.org/30013628/diff/30016567/lib/filterStorage.j...
lib/filterStorage.js:201: disconnectSubscriptionFilters(subscription,
oldFilterText);
On 2019/03/08 21:36:19, hub wrote:
> On 2019/02/26 12:29:57, Manish Jethani wrote:
> > You might think that looking up the subscriptions for a filter makes this
more
> > expensive, but actually since we no longer have to disconnect/connect
filters
> > from/to their subscriptions, this is actually faster in practice as long as
> you
> > have a reasonable number of subscriptions (I tried with 5).
> 
> my default config has 6. Add one language, that make 7. And then if I
remember,
> each user filter is a special subscription.

Each user-defined filter is not a special subscription, only each type
(blocking, hiding, etc.) is a special subscription. Actually there are only two
types: blocking and hiding.

I have the following lists:

 1.  Adblock Warning Removal List
 2.  ABP filters
 3.  EasyPrivacy
 4.  Fanboy's Social Blocking List
 5.  EasyList Germany+EasyList
 6.  EasyList China+EasyList
 7.  Allow nonintrusive advertising
 8.  EasyList

I added a `console.time(subscription.title)` and
`console.timeEnd(subscription.title)` to the start and end of this function
respectively. And then I manually clicked on the "Update all filter lists"
button on the options page.

Here's what I got:

Before (3 runs):

  Adblock Warning Removal List: 2.574951171875ms
  ABP filters: 2.102294921875ms
  EasyPrivacy: 121.102294921875ms
  Fanboy's Social Blocking List: 85.465087890625ms
  EasyList Germany+EasyList: 343.808837890625ms
  EasyList China+EasyList: 382.23095703125ms
  EasyList: 231.58203125ms
  Allow nonintrusive advertising: 170.242919921875ms

  ABP filters: 2.664794921875ms
  Adblock Warning Removal List: 2.403076171875ms
  Fanboy's Social Blocking List: 39.18603515625ms
  EasyPrivacy: 47.486083984375ms
  EasyList: 378.468994140625ms
  EasyList Germany+EasyList: 389.359130859375ms
  EasyList China+EasyList: 364.27099609375ms
  Allow nonintrusive advertising: 62.0830078125ms

  Adblock Warning Removal List: 1.76513671875ms 
  ABP filters: 2.052978515625ms
  EasyPrivacy: 45.468994140625ms
  Fanboy's Social Blocking List: 47.925048828125ms
  EasyList Germany+EasyList: 331.406982421875ms
  EasyList China+EasyList: 462.910888671875ms
  EasyList: 313.959228515625ms
  Allow nonintrusive advertising: 164.7099609375ms

After (3 runs):

  Adblock Warning Removal List: 3.661865234375ms
  ABP filters: 26.75830078125ms
  EasyPrivacy: 153.299072265625ms
  Fanboy's Social Blocking List: 101.579833984375ms
  EasyList Germany+EasyList: 298.937744140625ms
  EasyList China+EasyList: 368.741943359375ms
  Allow nonintrusive advertising: 65.531005859375ms
  EasyList: 289.38916015625ms

  Adblock Warning Removal List: 2.134765625ms
  ABP filters: 2.312255859375ms
  EasyPrivacy: 90.31005859375ms
  Fanboy's Social Blocking List: 76.914794921875ms
  EasyList Germany+EasyList: 327.611083984375ms
  EasyList: 305.14697265625ms
  EasyList China+EasyList: 358.02294921875ms
  Allow nonintrusive advertising: 66.77587890625ms

  Adblock Warning Removal List: 1.616943359375ms
  ABP filters: 2.01611328125ms
  EasyPrivacy: 60.262939453125ms
  Fanboy's Social Blocking List: 64.0009765625ms
  EasyList: 292.301025390625ms
  EasyList China+EasyList: 342.841064453125ms
  EasyList Germany+EasyList: 315.51220703125ms
  Allow nonintrusive advertising: 137.76318359375ms

It is comparable or better.

Let's remember a few things:

 1.  This happens in the background, at most once per hour
 2.  We can optimize this by looking only at the delta between old and new
filters (though this optimization is not necessary for this patch)
 3.  We are going to switch to incremental filter list updates, which will give
us a delta anyway

I also checked the load time by adding `console.time("Load")` and
`console.timeEnd("Load")` in the `importData()` function in
`lib/filterStorage.js` and reloading the extension multiple times.

Before (3 runs):

  Load: 1321.534912109375ms
  Load: 1121.9091796875ms
  Load: 1387.422607421875ms

After (3 runs):

  Load: 774.85986328125ms
  Load: 620.704833984375ms
  Load: 648.51416015625ms

This patch significantly speeds up loading.

https://codereview.adblockplus.org/30013628/diff/30016567/lib/subscriptionCla...
File lib/subscriptionClasses.js (right):

https://codereview.adblockplus.org/30013628/diff/30016567/lib/subscriptionCla...
lib/subscriptionClasses.js:207: this._filterTextIndex = null;
On 2019/03/08 21:36:19, hub wrote:
> Shouldn't it be more efficiant that we add the `filter.text` to
> `this._filterTextIndex` (and remove in deleteFilterAt) instead of deleting it?
> This would mean hasFilterText wouldn't need to create it.

Adding it right away would mean more memory usage and worse performance when the
extension is loaded (slower loading). Instead, we create it when it's needed
(i.e. when the subscriptions are updated).

Even when a subscription is updated, when the filters are being added/removed it
should just set it to `null` which will be faster. Later when we need to find
out in which subscriptions each filter exists, then for each subscription the
index is created only once.

[Manish Jethani, 2019-03-30 21:01:45]

https://codereview.adblockplus.org/30013628/diff/30016567/lib/iniParser.js
File lib/iniParser.js (left):

https://codereview.adblockplus.org/30013628/diff/30016567/lib/iniParser.js#ol...
lib/iniParser.js:112:
Filter.fromText(text).addSubscription(currentSubscription);
A side effect of this change is that `Filter` objects are not created for
disabled subscriptions. They get created for enabled subscriptions for the first
time in `lib/filterListener.js` instead.

Unfortunately this does not last for too long because within a minute of the
extension being loaded the lists are synced and in `onSubscriptionUpdated()` in
`lib/filterListener.js` we end up creating objects (even for filters we are
about to remove!). I intend to address all of this, step by step.

[hub, 2019-04-04 03:48:15]

https://codereview.adblockplus.org/30013628/diff/30016567/lib/iniParser.js
File lib/iniParser.js (left):

https://codereview.adblockplus.org/30013628/diff/30016567/lib/iniParser.js#ol...
lib/iniParser.js:112:
Filter.fromText(text).addSubscription(currentSubscription);
On 2019/03/30 21:01:44, Manish Jethani wrote:
> A side effect of this change is that `Filter` objects are not created for
> disabled subscriptions. They get created for enabled subscriptions for the
first
> time in `lib/filterListener.js` instead.
> 
> Unfortunately this does not last for too long because within a minute of the
> extension being loaded the lists are synced and in `onSubscriptionUpdated()`
in
> `lib/filterListener.js` we end up creating objects (even for filters we are
> about to remove!). I intend to address all of this, step by step.

In this patch or in a future one?

[Manish Jethani, 2019-04-04 05:44:20]

https://codereview.adblockplus.org/30013628/diff/30016567/lib/iniParser.js
File lib/iniParser.js (left):

https://codereview.adblockplus.org/30013628/diff/30016567/lib/iniParser.js#ol...
lib/iniParser.js:112:
Filter.fromText(text).addSubscription(currentSubscription);
On 2019/04/04 03:48:15, hub wrote:
> On 2019/03/30 21:01:44, Manish Jethani wrote:
> > A side effect of this change is that `Filter` objects are not created for
> > disabled subscriptions. They get created for enabled subscriptions for the
> first
> > time in `lib/filterListener.js` instead.
> > 
> > Unfortunately this does not last for too long because within a minute of the
> > extension being loaded the lists are synced and in `onSubscriptionUpdated()`
> in
> > `lib/filterListener.js` we end up creating objects (even for filters we are
> > about to remove!). I intend to address all of this, step by step.
> 
> In this patch or in a future one?

In a future one, because it is an enhancement on top of this and not necessary
to do for what we're trying to achieve here. The aim of this patch is to make
all filter objects lose any state so they can be discarded. After this, we can
do <https://codereview.adblockplus.org/29935564/> and then we should be able to
lose the `ElemHide` objects at least.

[hub, 2019-04-04 20:54:45]

LGTM

https://codereview.adblockplus.org/30013628/diff/30016567/lib/iniParser.js
File lib/iniParser.js (left):

https://codereview.adblockplus.org/30013628/diff/30016567/lib/iniParser.js#ol...
lib/iniParser.js:112:
Filter.fromText(text).addSubscription(currentSubscription);
On 2019/04/04 05:44:20, Manish Jethani wrote:
> On 2019/04/04 03:48:15, hub wrote:
> > On 2019/03/30 21:01:44, Manish Jethani wrote:
> > > A side effect of this change is that `Filter` objects are not created for
> > > disabled subscriptions. They get created for enabled subscriptions for the
> > first
> > > time in `lib/filterListener.js` instead.
> > > 
> > > Unfortunately this does not last for too long because within a minute of
the
> > > extension being loaded the lists are synced and in
`onSubscriptionUpdated()`
> > in
> > > `lib/filterListener.js` we end up creating objects (even for filters we
are
> > > about to remove!). I intend to address all of this, step by step.
> > 
> > In this patch or in a future one?
> 
> In a future one, because it is an enhancement on top of this and not necessary
> to do for what we're trying to achieve here. The aim of this patch is to make
> all filter objects lose any state so they can be discarded. After this, we can
> do <https://codereview.adblockplus.org/29935564/> and then we should be able
to
> lose the `ElemHide` objects at least.

Acknowledged.