Issue 7096 - Make it possible to lazy initialize a filter's subscriptions

Based on https://codereview.adblockplus.org/29934588/

[Manish Jethani, 2018-11-03 21:31:25]

Patch Set 1

The idea is to have filter objects that are stateless (no instance-specific
state) and ephemeral (not saved but rather discarded after use). In order to do
this, we need the ability to add the subscriptions for a filter to the filter
object at any time in the object's lifecycle.

Also see inline comments.

(This patch doesn't change anything in practice, it's a setup for what is to
follow in the next patches.)

https://codereview.adblockplus.org/29935568/diff/29935569/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/29935568/diff/29935569/lib/filterListener....
lib/filterListener.js:143: if (!filter.subscriptionsAdded)
The subscriptions will already be added when the subscriptions are initially
loaded, but when we move to ephemeral filter objects, newly created filter
objects won't have their subscriptions added by default.

This code runs when the subscriptions are initially loaded and then again when a
new version of a subscription is downloaded. For the latter case, the
performance will be worse than it currently is, but this happens once a day per
subscription on average, it should not be a problem. In comparison, having
ephemeral filter objects that don't have their subscriptions already added would
allow us to save several megabytes of memory (the savings come from the objects
not being added to the `Filter.knownFilters` map as well as not being held on to
in other places). It's a good tradeoff.

https://codereview.adblockplus.org/29935568/diff/29935569/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29935568/diff/29935569/lib/filterStorage.j...
lib/filterStorage.js:138: if (subscription.filters.indexOf(filter) != -1)
This is super inefficient, but currently it is not needed in practice, and in
the future (a couple of patches later) it's going to be a set lookup.

See the big idea here: https://codereview.adblockplus.org/29934588/

[Manish Jethani, 2018-11-03 21:35:09]

Patch Set 2

[Manish Jethani, 2018-11-03 21:40:34]

To test this, comment out the following line in `lib/iniParser.js` and run the
tests:

  filter.addSubscription(currentSubscription);

[hub, 2018-11-10 04:33:21]

https://codereview.adblockplus.org/29935568/diff/29935573/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29935568/diff/29935573/lib/filterClasses.j...
lib/filterClasses.js:101: // Once set to true, the cannot be set back to false
even if all
typo.

https://codereview.adblockplus.org/29935568/diff/29935573/lib/filterClasses.j...
lib/filterClasses.js:104: this._subscriptions = this._subscriptions;
I don't understand what's happening here. This assignment is a noop.

https://codereview.adblockplus.org/29935568/diff/29935573/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29935568/diff/29935573/lib/filterStorage.j...
lib/filterStorage.js:138: if (subscription.filters.indexOf(filter) != -1)
this is quite an expensive search, isn't it?

[Manish Jethani, 2018-11-15 22:58:53]

Patch Set 3: Fix typo

https://codereview.adblockplus.org/29935568/diff/29935573/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29935568/diff/29935573/lib/filterClasses.j...
lib/filterClasses.js:104: this._subscriptions = this._subscriptions;
On 2018/11/10 04:33:21, hub wrote:
> I don't understand what's happening here. This assignment is a noop.

Not really, it sets the property on the this object, originally the property is
on the object's prototype.

https://codereview.adblockplus.org/29935568/diff/29935573/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29935568/diff/29935573/lib/filterStorage.j...
lib/filterStorage.js:138: if (subscription.filters.indexOf(filter) != -1)
On 2018/11/10 04:33:21, hub wrote:
> this is quite an expensive search, isn't it?

Yes, so here's the thing:

 1.  This function will not be executed on startup, because the initial filter
objects will already have their subscriptions assigned in INIParser; once those
objects are discarded and then when a subscription is updated, it will be called
for any newly created filter objects

 2.  In
https://codereview.adblockplus.org/29934588/diff/29934599/lib/subscriptionCla...
I intend to add a Set object containing the filter text, so this will become a
set lookup and it will be faster (see searchFilter method)

[hub, 2018-11-16 17:04:39]

https://codereview.adblockplus.org/29935568/diff/29935573/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29935568/diff/29935573/lib/filterClasses.j...
lib/filterClasses.js:104: this._subscriptions = this._subscriptions;
On 2018/11/15 22:58:52, Manish Jethani wrote:
> On 2018/11/10 04:33:21, hub wrote:
> > I don't understand what's happening here. This assignment is a noop.
> 
> Not really, it sets the property on the this object, originally the property
is
> on the object's prototype.

Acknowledged.

https://codereview.adblockplus.org/29935568/diff/29935573/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29935568/diff/29935573/lib/filterStorage.j...
lib/filterStorage.js:138: if (subscription.filters.indexOf(filter) != -1)
On 2018/11/15 22:58:52, Manish Jethani wrote:
> On 2018/11/10 04:33:21, hub wrote:
> > this is quite an expensive search, isn't it?
> 
> Yes, so here's the thing:
> 
>  1.  This function will not be executed on startup, because the initial filter
> objects will already have their subscriptions assigned in INIParser; once
those
> objects are discarded and then when a subscription is updated, it will be
called
> for any newly created filter objects
> 
>  2.  In
>
https://codereview.adblockplus.org/29934588/diff/29934599/lib/subscriptionCla...
> I intend to add a Set object containing the filter text, so this will become a
> set lookup and it will be faster (see searchFilter method)

So shall we make sure the review 29934588 lands first?

[Manish Jethani, 2018-11-17 19:49:17]

https://codereview.adblockplus.org/29935568/diff/29935573/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29935568/diff/29935573/lib/filterStorage.j...
lib/filterStorage.js:138: if (subscription.filters.indexOf(filter) != -1)
On 2018/11/16 17:04:39, hub wrote:
> On 2018/11/15 22:58:52, Manish Jethani wrote:
> > On 2018/11/10 04:33:21, hub wrote:
> > > this is quite an expensive search, isn't it?
> > 
> > Yes, so here's the thing:
> > 
> >  1.  This function will not be executed on startup, because the initial
filter
> > objects will already have their subscriptions assigned in INIParser; once
> those
> > objects are discarded and then when a subscription is updated, it will be
> called
> > for any newly created filter objects
> > 
> >  2.  In
> >
>
https://codereview.adblockplus.org/29934588/diff/29934599/lib/subscriptionCla...
> > I intend to add a Set object containing the filter text, so this will become
a
> > set lookup and it will be faster (see searchFilter method)
> 
> So shall we make sure the review 29934588 lands first?

OK, let's do this one first instead, it's easier:

https://codereview.adblockplus.org/29935564/

I'll prepare the other one.

[Manish Jethani, 2018-11-18 00:50:58]

Patch Set 4: Rebase and refactor
Patch Set 5: Fix typo

Now based on https://codereview.adblockplus.org/29934588/

https://codereview.adblockplus.org/29935568/diff/29945597/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29935568/diff/29945597/lib/filterClasses.j...
lib/filterClasses.js:126: * @package
All subscription-related methods on the filter object are now package-level
access. Outside code should use filterStorage.subscriptionsForFilter() and
filterStorage.getSubscriptionCountForFilter()

[Manish Jethani, 2018-11-18 02:32:16]

Patch Set 6: Rebase

[Manish Jethani, 2018-11-18 04:57:38]

Patch Set 7: Use new methods in two more places

[hub, 2018-11-19 18:43:39]

LGTM

[Manish Jethani, 2018-11-20 02:53:32]

On 2018/11/19 18:43:39, hub wrote:
> LGTM

Sorry, I have removed hasFilter() in the base patch, this no longer works as it
is. We might not even need to do it this way. Let's focus on the base patch
first.