Issue 7094 - Keep subscription filters by text only

[Manish Jethani, 2018-11-19 02:12:20]

Patch Set 1

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

This patch inexplicably slashes the memory footprint by ~8 MB. I don't now what
happened, I was only intending this as an intermediate step. Maybe there's a bug
somewhere.

Can you take a look, please? If this reduces the footprint by so much then it's
fantastic.

[Manish Jethani, 2018-11-19 02:15:30]

For some background, the basic idea here is that we keep only the text in the
subscription object. Most of the time only the text is needed, except when a
subscription is updated, disabled, or removed (all rare). At load time, we
substitute the call to Filter.fromText() in INIParser with one in
filterListener.

https://codereview.adblockplus.org/29946572/diff/29946573/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29946572/diff/29946573/lib/filterStorage.j...
lib/filterStorage.js:204: for (let filter of filterText)
We should reduce the old filter text here to only include the filters that have
really been removed. This will make it faster.

[Manish Jethani, 2018-11-19 02:29:44]

Patch Set 2: Fix bug

No longer making any difference to memory footprint.

https://codereview.adblockplus.org/29946572/diff/29946582/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/29946572/diff/29946582/lib/filterListener....
lib/filterListener.js:244: filter.addSubscription(subscription);
This was the bug. The filters did not belong to any subscription.

[Manish Jethani, 2018-11-21 05:11:53]

The base patch has landed, this is the next step.

[Manish Jethani, 2018-11-21 05:27:53]

Patch Set 3: Minor changes

https://codereview.adblockplus.org/29946572/diff/29949555/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29946572/diff/29949555/lib/filterStorage.j...
lib/filterStorage.js:512: yield* Filter.fromText(text).serialize();
This means that serialization would become slower, but it doesn't happen often
enough and it's a generator function (async) anyway. In any case, I'll measure
this.

[hub, 2018-11-30 01:18:07]

https://codereview.adblockplus.org/29946572/diff/29949555/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/29946572/diff/29949555/lib/filterListener....
lib/filterListener.js:264: removeFilter(Filter.fromText(text));
here remove filter takes longer since we have to go with `Filter.fromText()`
first...

https://codereview.adblockplus.org/29946572/diff/29949555/lib/filterListener....
lib/filterListener.js:281: removeFilter(Filter.fromText(text));
...so does disable subscription.

[Manish Jethani, 2018-11-30 07:28:06]

https://codereview.adblockplus.org/29946572/diff/29949555/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/29946572/diff/29949555/lib/filterListener....
lib/filterListener.js:264: removeFilter(Filter.fromText(text));
On 2018/11/30 01:18:07, hub wrote:
> here remove filter takes longer since we have to go with `Filter.fromText()`
> first...

Yes, this is a cost of this change, and by itself there is no benefit in fact.
But if we take the changes in aggregate, then this is the tradeoff:

 * significantly fewer filter objects in memory (~10 MB)
 * the filter objects that don't exist (i.e. "ephemeral") need to be recreated,
or at least the filter text needs to be parsed again
 * the subscriptions to which a filter belongs need to be found again by looking
up the filter text in each known subscription

In the current patch it's just a lookup in `Filter.knownFilters`, which is not
too significant by itself (given that subscriptions are removed or disabled only
rarely).

In the final version, I know that some operations like removing/disabling
subscriptions and filters will become more expensive, though not necessarily
adding/reenabling subscriptions and filters (and certainly not on initial load).

The program spends most of its time doing filter matching, which is unaffected,
as is the initial loading of the subscriptions. For the rest of the operations,
which occur only in response to user action or occasional (once per hour at
most) processing in the background, I believe this is a good tradeoff of memory
vs. performance.

https://codereview.adblockplus.org/29946572/diff/29949555/lib/filterListener....
lib/filterListener.js:281: removeFilter(Filter.fromText(text));
On 2018/11/30 01:18:07, hub wrote:
> ...so does disable subscription.

See above.

[hub, 2019-01-03 20:22:49]

LGTM

https://codereview.adblockplus.org/29946572/diff/29949555/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/29946572/diff/29949555/lib/filterListener....
lib/filterListener.js:264: removeFilter(Filter.fromText(text));
On 2018/11/30 07:28:05, Manish Jethani wrote:
> On 2018/11/30 01:18:07, hub wrote:
> > here remove filter takes longer since we have to go with `Filter.fromText()`
> > first...
> 
> Yes, this is a cost of this change, and by itself there is no benefit in fact.
> But if we take the changes in aggregate, then this is the tradeoff:
> 
>  * significantly fewer filter objects in memory (~10 MB)
>  * the filter objects that don't exist (i.e. "ephemeral") need to be
recreated,
> or at least the filter text needs to be parsed again
>  * the subscriptions to which a filter belongs need to be found again by
looking
> up the filter text in each known subscription
> 
> In the current patch it's just a lookup in `Filter.knownFilters`, which is not
> too significant by itself (given that subscriptions are removed or disabled
only
> rarely).
> 
> In the final version, I know that some operations like removing/disabling
> subscriptions and filters will become more expensive, though not necessarily
> adding/reenabling subscriptions and filters (and certainly not on initial
load).
> 
> The program spends most of its time doing filter matching, which is
unaffected,
> as is the initial loading of the subscriptions. For the rest of the
operations,
> which occur only in response to user action or occasional (once per hour at
> most) processing in the background, I believe this is a good tradeoff of
memory
> vs. performance.

Acknowledged.

[Manish Jethani, 2019-02-16 15:12:03]

Patch Set 4: Rebase

I am yet to assess the performance impact but overall I am fairly sure that we
need to proceed in this direction.

[hub, 2019-02-19 18:01:08]

LGTM