Issue 5735 - Use JS Map instead of Object for property domains of Filter objects

# based on https://codereview.adblockplus.org/29550598/

[kzar, 2017-09-20 15:02:41]

When I was working on getSelectorsForDomain previously I found using Maps worked
better for Chrome but was either slower or unsupported on Safari. I guess since
we no longer support Safari on the main branch we're OK, and this change will
likely speed things up, but it would be nice if you could confirm this change
doesn't slow things down.

IIRC this is how I was testing its performance:

(function() {
  let start = performance.now();
  for (let i = 0; i < 1000; i += 1)
    ElemHide.getSelectorsForDomain("www.extremetech.com");
  return performance.now() - start;
}());

[Wladimir Palant, 2017-09-21 08:11:43]

From all I know, Safari is the only browser where we have to worry about Map
performance - and we no longer support it. So this is the right time to start
using Map.

https://codereview.adblockplus.org/29550662/diff/29550663/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29550662/diff/29550663/lib/elemHide.js#new...
lib/elemHide.js:70: defaultDomains.set("", true);
You can initialize the Map object immediately: new Map([["", true]])

https://codereview.adblockplus.org/29550662/diff/29550663/lib/elemHide.js#new...
lib/elemHide.js:111: filters = filtersByDomain[domain] = Object.create(null);
Shouldn't this be turned in to a Map object as well while we are at it?

https://codereview.adblockplus.org/29550662/diff/29550663/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29550662/diff/29550663/lib/filterClasses.j...
lib/filterClasses.js:390: domains.set(list[0], true);
You can initialize the Map object immediately: new Map([["", false], [list[0],
true]])

https://codereview.adblockplus.org/29550662/diff/29550663/lib/filterClasses.j...
lib/filterClasses.js:470: if (isDomainIncluded != undefined)
We usually use the typeof operator: `typeof isDomainIncluded != "undefined"`

[sergei, 2017-09-21 10:50:34]

https://codereview.adblockplus.org/29550662/diff/29550663/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29550662/diff/29550663/lib/elemHide.js#new...
lib/elemHide.js:70: defaultDomains.set("", true);
On 2017/09/21 08:11:44, Wladimir Palant wrote:
> You can initialize the Map object immediately: new Map([["", true]])

Done.

https://codereview.adblockplus.org/29550662/diff/29550663/lib/elemHide.js#new...
lib/elemHide.js:111: filters = filtersByDomain[domain] = Object.create(null);
On 2017/09/21 08:11:44, Wladimir Palant wrote:
> Shouldn't this be turned in to a Map object as well while we are at it?

No, I find the change of filtersByDomain self-contained enough because the
current change is rather aimed at filterClasses module and the change in
elemHide.js is a consequence. In addition smaller commits are always better, for
instance we can easier compare the contribution in the performance and memory
usage from a change of type of one object with many entries with the change of
types of many objects with not so many entries.

https://codereview.adblockplus.org/29550662/diff/29550663/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29550662/diff/29550663/lib/filterClasses.j...
lib/filterClasses.js:390: domains.set(list[0], true);
On 2017/09/21 08:11:44, Wladimir Palant wrote:
> You can initialize the Map object immediately: new Map([["", false], [list[0],
> true]])

Done. BTW, I would like to pay attention to the impact of these tree additional
arrays on the performance and memory usage.

https://codereview.adblockplus.org/29550662/diff/29550663/lib/filterClasses.j...
lib/filterClasses.js:470: if (isDomainIncluded != undefined)
On 2017/09/21 08:11:44, Wladimir Palant wrote:
> We usually use the typeof operator: `typeof isDomainIncluded != "undefined"`

Acknowledged.

[sergei, 2017-09-21 10:52:45]

On 2017/09/20 15:02:41, kzar wrote:
> it would be nice if you could confirm this change
> doesn't slow things down.
> 
> IIRC this is how I was testing its performance:
> 
> (function() {
>   let start = performance.now();
>   for (let i = 0; i < 1000; i += 1)
>     ElemHide.getSelectorsForDomain("www.extremetech.com");
>   return performance.now() - start;
> }());

Thanks for pointing on it, I will definitely try to estimate how it affects the
performance but if there are no objections a bit later.

[Wladimir Palant, 2017-09-21 10:53:01]

LGTM

https://codereview.adblockplus.org/29550662/diff/29550663/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29550662/diff/29550663/lib/filterClasses.j...
lib/filterClasses.js:390: domains.set(list[0], true);
On 2017/09/21 10:50:34, sergei wrote:
> BTW, I would like to pay attention to the impact of these tree additional
> arrays on the performance and memory usage.

Feel free to measure that. I assume that the JS engine uses shortcuts here so
that the arrays aren't actually created.

[kzar, 2017-09-22 10:42:57]

On 2017/09/21 10:52:45, sergei wrote:
> Thanks for pointing on it, I will definitely try to estimate how it affects
the
> performance but if there are no objections a bit later.

Well IMO if we're going to test the performance of the change, we might as well
do so before pushing it.