Issue 7046 - Defer caching of domain maps

For element hiding filters, the domains property is accessed only a couple of times (and only once after this patch). It doesn't make sense to cache the generated map since it is never going to be used again.

We generalize this by keeping track of whether the domains getter has been accessed at least once before and cache the value only if this is true.

This patch reduces the initial JS used heap by ~2.5 MB.

[Manish Jethani, 2018-10-15 15:37:00]

Patch Set 1-6

As the description says, this reduces the initial memory footprint by another
~2.5 MB.

I'll file an issue.

https://codereview.adblockplus.org/29909555/diff/29909568/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29909555/diff/29909568/lib/elemHide.js#new...
lib/elemHide.js:288: let {domains, selector} = filter;
Here we are just minimizing access to the domains getter.

https://codereview.adblockplus.org/29909555/diff/29909568/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29909555/diff/29909568/lib/filterClasses.j...
lib/filterClasses.js:527: if (!domains || cacheValue)
There is a change in the semantics here in that we now always cache the value if
it is null. This makes sense; it doesn't make sense to keep regenerating the
value if it amounts to null anyway.

https://codereview.adblockplus.org/29909555/diff/29909568/test/filterClasses.js
File test/filterClasses.js (right):

https://codereview.adblockplus.org/29909555/diff/29909568/test/filterClasses....
test/filterClasses.js:608: // Make sure the generated domain map is cached on
first access.
We need to set this in order to test our deduplication logic.

https://codereview.adblockplus.org/29909555/diff/29909572/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29909555/diff/29909572/lib/filterClasses.j...
lib/filterClasses.js:534: Object.defineProperty(this, "domains", {value:
domains});
It just occurred to me that we don't have to set this property when
`domainSource` is null.

[Manish Jethani, 2018-10-15 20:39:03]

On 2018/10/15 15:37:00, Manish Jethani wrote:
> Patch Set 1-6
> 
> As the description says, this reduces the initial memory footprint by another
> ~2.5 MB.
> 
> I'll file an issue.

Filed:

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

[hub, 2018-10-16 16:51:50]

LGTM

[Manish Jethani, 2018-10-16 18:03:25]

Patch Set 7: Change shouldCacheDomain function to cacheDomain property

Sorry, I reworked this a little bit. I don't think there's any need to have a
`shouldCacheDomains` function, it can just be a property instead.

Also updated the JSDoc comments (`@inheitdoc` doesn't actually work because
these are not proper classes, instead it's better to just use `@see` and
specific the type of the property again in the subclass).

[Manish Jethani, 2018-10-16 18:25:42]

On 2018/10/16 18:03:25, Manish Jethani wrote:
> Patch Set 7: Change shouldCacheDomain function to cacheDomain property
> 
> Sorry, I reworked this a little bit. I don't think there's any need to have a
> `shouldCacheDomains` function, it can just be a property instead.
> 
> Also updated the JSDoc comments (`@inheitdoc` doesn't actually work because
> these are not proper classes, instead it's better to just use `@see` and
> specific the type of the property again in the subclass).

Unfortunately this update also includes a rebase with
https://hg.adblockplus.org/adblockpluscore/rev/cd837fd7d60e sorry about that.

[hub, 2018-10-16 21:15:55]

LGTM