Issue 6955 - Avoid making copies of common selector list

[Manish Jethani, 2018-09-16 16:07:47]

Patch Set 1

[Manish Jethani, 2018-09-16 18:54:13]

Scratch this, first we need to do Trac #6652, otherwise this is not meaningful.

[Manish Jethani, 2018-09-17 09:28:43]

Patch Set 2: Cache common selector list

OK, I'm back with an update.

This patch introduces a new concept of "common" selectors, the precise
definition of which is in the JSDoc comment for the `commonSelectors` variable.
This is the list of selectors that applies to all random domains like
example.com, manishjethani.com, and so on (domains that don't appear in any
filter list). The idea is to stop making a copy of this list, which is the same
each time, and return a cached copy instead. This would also allow us to
deduplicate the style sheet, saving even more memory in the extension (#6957).

Note that there is a better way of doing this, which is in patch #29773570 and
has stayed in review forever because of its complexity. I would rather do the
simple version first.

https://codereview.adblockplus.org/29882558/diff/29883559/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29882558/diff/29883559/lib/elemHide.js#new...
lib/elemHide.js:50: * Cached list of selectors that apply on any domain that
neither has any
Here we are introducing the idea of "common" selectors. The precise definition
is in the JSDoc here.

https://codereview.adblockplus.org/29882558/diff/29883559/test/elemHide.js
File test/elemHide.js (right):

https://codereview.adblockplus.org/29882558/diff/29883559/test/elemHide.js#ne...
test/elemHide.js:47: return selectors.slice().sort().filter((selector, index,
sortedSelectors) =>
Need to make a copy now because the return value of getSelectorsForDomain is a
read-only array.

https://codereview.adblockplus.org/29882558/diff/29883559/test/elemHide.js#ne...
test/elemHide.js:277: exports.testCommonSelectorsDedup = function(test)
Read this first.

[Manish Jethani, 2018-09-17 09:30:33]

On 2018/09/17 09:28:43, Manish Jethani wrote:
> Patch Set 2: Cache common selector list
> 
> OK, I'm back with an update.
>
> [...]

Forgot to add, perhaps the best way to start with this is to go through the unit
tests, where I am testing exactly for the desired behavior.

[Manish Jethani, 2018-09-17 12:21:15]

Patch Set 3: Keep track of exceptions per domain

See comment inline.

The performance of getSelectorsForDomain in this patch is comparable for both
known (aol.com) and unknown (x4aabd7c0ddcd.com) domains.

The size of the exceptionCountByDomain map is ~4.5 MB. We'll have to measure
whether the net gain in memory savings is greater than this for a typical
browsing session.

https://codereview.adblockplus.org/29882558/diff/29883564/test/elemHide.js
File test/elemHide.js (right):

https://codereview.adblockplus.org/29882558/diff/29883564/test/elemHide.js#ne...
test/elemHide.js:367: // Generic exception with unrelated domain excluded.
This particular test would not pass with Patch Set 2, and this reflects the
situation in EasyList+AA (unrelated domains are excluded from generic filters).

To fix this Patch Set 3 keeps count of the number of exceptions related to a
domain and uses that to decide whether a domain falls in the common case or not.

[Jon Sonesen, 2018-09-17 19:00:35]

Hey Manish,

Seems like this is a good first step for #6652, but I guess my question is
whether this change is still blocked by that issue? Or rather, is the addition
of the new 'common selectors' providing the work around so #6652 is not needed?
If so is this solution something that can replace the 'blocking' aspects of
6652?

[Manish Jethani, 2018-09-19 13:15:24]

On 2018/09/17 19:00:35, Jon Sonesen wrote:
> Hey Manish,
> 
> Seems like this is a good first step for #6652, but I guess my question is
> whether this change is still blocked by that issue? Or rather, is the addition
> of the new 'common selectors' providing the work around so #6652 is not
needed?
> If so is this solution something that can replace the 'blocking' aspects of
> 6652?

Never mind, I have a better idea now. I'll update the issue and upload a new
patch. Let me try it out first locally.

[Jon Sonesen, 2018-09-23 17:42:01]

On 2018/09/19 13:15:24, Manish Jethani wrote:
> On 2018/09/17 19:00:35, Jon Sonesen wrote:
> > Hey Manish,
> > 
> > Seems like this is a good first step for #6652, but I guess my question is
> > whether this change is still blocked by that issue? Or rather, is the
addition
> > of the new 'common selectors' providing the work around so #6652 is not
> needed?
> > If so is this solution something that can replace the 'blocking' aspects of
> > 6652?
> 
> Never mind, I have a better idea now. I'll update the issue and upload a new
> patch. Let me try it out first locally.

Ack

[Jon Sonesen, 2018-10-02 17:28:26]

Looking at the review, and it seems mostly good but I am taking my time since
there are quite a lot of changes. One thing I wonder about is this change's
relation to #6428[0] should this ticket be referenced or perhaps even have the
exception tracking under #6428 since it seems a bit out of scope to changes to
exception data structure in a commi tmeant to just avoid a list copy..

[0]: https://issues.adblockplus.org/ticket/6428

[Manish Jethani, 2018-10-02 23:07:07]

On 2018/10/02 17:28:26, Jon Sonesen wrote:
> Looking at the review, and it seems mostly good but I am taking my time since
> there are quite a lot of changes. One thing I wonder about is this change's
> relation to #6428[0] should this ticket be referenced or perhaps even have the
> exception tracking under #6428 since it seems a bit out of scope to changes to
> exception data structure in a commi tmeant to just avoid a list copy..

We'll have to rethink #6428 now, let's talk about that on the Trac ticket.

This patch is no longer relevant after the changes for #6957 [1]

[1]: https://issues.adblockplus.org/ticket/6955#Resolution