Issue 7452 - Do not cache element hiding filter objects by default

[Manish Jethani, 2018-11-03 20:01:25]

Patch Set 1

The plan is to make Filter objects (1) stateless and (2) ephemeral (i.e. they
only exist until they are needed). In order to do this successfully, we need to
lose all unnecessary references to Filter objects. This is one place where we
could do this.

This patch patch by itself makes no difference to memory usage or performance,
but it is part of the work. I have a few patches that will ultimately make a
huge difference.

https://codereview.adblockplus.org/29935564/diff/29935565/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29935564/diff/29935565/lib/elemHide.js#new...
lib/elemHide.js:38: * Lookup table, active flag, by filter by domain.
Throughout this file, "filter" now means filter text rather than filter object.

https://codereview.adblockplus.org/29935564/diff/29935565/lib/elemHide.js#new...
lib/elemHide.js:128: filters.set(filter.text, isIncluded ? filter.selector :
null);
Instead of true/false, we now keep the selector or null. This means we never
have to access the filter object again.

https://codereview.adblockplus.org/29935564/diff/29935565/lib/elemHide.js#new...
lib/elemHide.js:243:
addToFiltersByDomain(Filter.fromText(unconditionalFilterForSelector));
This is the one time where we have to get the filter object. This is relatively
rare, there aren't that many exceptions that apply to _generic_ filters.

[hub, 2018-11-22 15:59:34]

https://codereview.adblockplus.org/29935564/diff/29935565/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29935564/diff/29935565/lib/elemHide.js#new...
lib/elemHide.js:38: * Lookup table, active flag, by filter by domain.
On 2018/11/03 20:01:25, Manish Jethani wrote:
> Throughout this file, "filter" now means filter text rather than filter
object.

I'd update the comment to say `by filter text`

[Manish Jethani, 2018-11-23 02:03:34]

Patch Set 2: Rename filter* to filterText*

I've changed the named of the variables as well to reflect that it is the filter
text that is being kept in these.

https://codereview.adblockplus.org/29935564/diff/29935565/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29935564/diff/29935565/lib/elemHide.js#new...
lib/elemHide.js:38: * Lookup table, active flag, by filter by domain.
On 2018/11/22 15:59:33, hub wrote:
> On 2018/11/03 20:01:25, Manish Jethani wrote:
> > Throughout this file, "filter" now means filter text rather than filter
> object.
> 
> I'd update the comment to say `by filter text`

Done.

[Manish Jethani, 2018-11-23 02:04:07]

On 2018/11/23 02:03:34, Manish Jethani wrote:

> I've changed the named of the variables 

*names

[Manish Jethani, 2018-11-23 02:05:43]

By the way, I won't land this until all the other related patches have also been
reviewed, because they only really make sense when taken together. But it's
better to do the reviews separately, it makes it easier.

[hub, 2018-11-28 23:21:31]

https://codereview.adblockplus.org/29935564/diff/29950555/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29935564/diff/29950555/lib/elemHide.js#new...
lib/elemHide.js:244:
addToFiltersByDomain(Filter.fromText(unconditionalFilterTextForSelector));
So here, we call Filter.fromText() while before we didn't.

[Manish Jethani, 2019-04-07 18:05:09]

https://codereview.adblockplus.org/29935564/diff/29950555/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29935564/diff/29950555/lib/elemHide.js#new...
lib/elemHide.js:244:
addToFiltersByDomain(Filter.fromText(unconditionalFilterTextForSelector));
On 2018/11/28 23:21:31, hub wrote:
> So here, we call Filter.fromText() while before we didn't.

Yes, unfortunately this is the price.

I looked at how many times this has to be done: EasyList: 637; EasyList+AA:
1,230. For comparison, there are 32,156 element hiding filters in EasyList+AA. I
think it's worth it. The filter text itself is `##<selector>` so it should be
relatively quick to parse (and if it isn't then we should optimize for that
case).

[Manish Jethani, 2019-04-07 18:14:12]

Patch Set 3: Rebase on GitLab MR #24

I have rebased this on a patch under review because there is overlap and that
one has almost landed.

If you apply this patch, you'll find that it reduces the memory footprint (heap
used) by ~5 MB with EasyList only.

https://codereview.adblockplus.org/29935564/diff/30040555/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29935564/diff/30040555/lib/elemHide.js#new...
lib/elemHide.js:24: const {Filter} = require("./filterClasses");
I have minimized the changes in this file for now so it becomes easier to review
what's actually changed. I'd like to make those changes too (renaming `filter`
to `filterText` in places) but I can do that once the important parts are
reviewed.

In this rebase no logic has changed except for the second argument to
`Filter.fromText()`.

https://codereview.adblockplus.org/29935564/diff/30040555/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29935564/diff/30040555/lib/filterClasses.j...
lib/filterClasses.js:141: return !(filter instanceof ElemHideFilter);
This is a separate function because I think we'll end up adding more conditions
here. e.g. not caching simple request blocking filters like `&act=ads_`.

https://codereview.adblockplus.org/29935564/diff/30040555/lib/filterClasses.j...
lib/filterClasses.js:225: * @param {boolean} [forceCache] Whether to force-cache
the filter object.
The idea is that the second parameter is for internal use only; if
`Filter.fromText()` is called from outside code, it should behave just as
before.

[Manish Jethani, 2019-04-07 18:55:11]

https://codereview.adblockplus.org/29935564/diff/29950555/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29935564/diff/29950555/lib/elemHide.js#new...
lib/elemHide.js:244:
addToFiltersByDomain(Filter.fromText(unconditionalFilterTextForSelector));
On 2019/04/07 18:05:09, Manish Jethani wrote:
> On 2018/11/28 23:21:31, hub wrote:
> > So here, we call Filter.fromText() while before we didn't.
> 
> Yes, unfortunately this is the price.
> 
> I looked at how many times this has to be done: EasyList: 637; EasyList+AA:
> 1,230. For comparison, there are 32,156 element hiding filters in EasyList+AA.
I
> think it's worth it. The filter text itself is `##<selector>` so it should be
> relatively quick to parse (and if it isn't then we should optimize for that
> case).

OK, so I looked into this [1], and parsing of generic element hiding filters is
already as fast as it can be. Maybe you want to take another look sometime.

[1]: https://issues.adblockplus.org/ticket/7445

[hub, 2019-04-08 20:44:54]

LGTM

https://codereview.adblockplus.org/29935564/diff/30040555/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29935564/diff/30040555/lib/filterClasses.j...
lib/filterClasses.js:141: return !(filter instanceof ElemHideFilter);
On 2019/04/07 18:14:11, Manish Jethani wrote:
> This is a separate function because I think we'll end up adding more
conditions
> here. e.g. not caching simple request blocking filters like `&act=ads_`.

Acknowledged.

[Manish Jethani, 2019-04-08 22:01:38]

Patch Set 4: Rebase on next
Patch Set 5: Rename filter* to filterText*

[Manish Jethani, 2019-04-09 02:55:31]

On 2019/04/08 22:01:38, Manish Jethani wrote:
> Patch Set 4: Rebase on next
> Patch Set 5: Rename filter* to filterText*

I have filed a new issue for this since the original ticket is now closed:

https://issues.adblockplus.org/ticket/7452

We still cannot land this because if we do then it would make element hiding
filters unable to retain their state, like whether a filter is disabled, how
many hits it got, and so on. While we do not use these properties in the current
WebExt version, we still support them so they must keep working. I have
described this in the issue.