Issue 7250 - Optimize third-party request check

About the public suffix map:

 -  Memory footprint: 224 KB
 -  Time to build: 0.2% of extension startup time

[Manish Jethani, 2019-01-31 10:14:15]

Patch Set 1

I'd like to get this into ABP 3.5. We going into code freeze soon.

https://codereview.adblockplus.org/29994555/diff/29994558/lib/domain.js
File lib/domain.js (right):

https://codereview.adblockplus.org/29994555/diff/29994558/lib/domain.js#newco...
lib/domain.js:22: let publicSuffixMap = buildPublicSuffixMap();
Maps are faster for dynamic lookups (where the property name is not known) and
also safer in such cases.

https://codereview.adblockplus.org/29994555/diff/29994558/lib/domain.js#newco...
lib/domain.js:34: function* suffixes(domain)
We will reuse this function in other places (e.g. isActiveOnDomain).

https://codereview.adblockplus.org/29994555/diff/29994558/lib/domain.js#newco...
lib/domain.js:68: function getDomain(hostname)
At least twice as fast as the previous implementation.

[Manish Jethani, 2019-01-31 12:23:59]

Patch Set 2: Add JSDoc
Patch Set 3: Use NaN instead of null

Also filed an issue:

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

[Manish Jethani, 2019-01-31 12:32:54]

The tests are here:

https://codereview.adblockplus.org/29993591/

I can include them in this patch if you like.

[hub, 2019-02-01 19:57:57]

On 2019/01/31 12:32:54, Manish Jethani wrote:
> The tests are here:
> 
> https://codereview.adblockplus.org/29993591/
> 
> I can include them in this patch if you like.

I think we can fold the test for this into the same review.

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

https://codereview.adblockplus.org/29994555/diff/29994562/lib/domain.js
File lib/domain.js (right):

https://codereview.adblockplus.org/29994555/diff/29994562/lib/domain.js#newco...
lib/domain.js:37: map.set(key, publicSuffixes[key]);
Wouldn't `new Map(publicSuffixes);` work?

[Manish Jethani, 2019-02-02 05:04:48]

Patch Set 4: Add tests

https://codereview.adblockplus.org/29994555/diff/29994562/lib/domain.js
File lib/domain.js (right):

https://codereview.adblockplus.org/29994555/diff/29994562/lib/domain.js#newco...
lib/domain.js:37: map.set(key, publicSuffixes[key]);
On 2019/02/01 19:58:08, hub wrote:
> Wouldn't `new Map(publicSuffixes);` work?

No, the `Map` constructor expects a two-dimensional array (actually any iterable
that yields key-value pairs).

We could do this:

  return new Map(Object.keys(publicSuffixes)
                 .map(key => [key, publicSuffixes[key]]));

But this is thrice as slow.

We could also do this:

  return new Map(Object.entries(publicSuffixes));

But this is only supported on Chrome 54+ and is actually 6-7 times as slow.

The way the function does it now is the fastest way to do it.

[hub, 2019-02-04 21:56:25]

LGTM

https://codereview.adblockplus.org/29994555/diff/29994562/lib/domain.js
File lib/domain.js (right):

https://codereview.adblockplus.org/29994555/diff/29994562/lib/domain.js#newco...
lib/domain.js:37: map.set(key, publicSuffixes[key]);
On 2019/02/02 05:04:47, Manish Jethani wrote:
> On 2019/02/01 19:58:08, hub wrote:
> > Wouldn't `new Map(publicSuffixes);` work?
> 
> No, the `Map` constructor expects a two-dimensional array (actually any
iterable
> that yields key-value pairs).
> 
> We could do this:
> 
>   return new Map(Object.keys(publicSuffixes)
>                  .map(key => [key, publicSuffixes[key]]));
> 
> But this is thrice as slow.
> 
> We could also do this:
> 
>   return new Map(Object.entries(publicSuffixes));
> 
> But this is only supported on Chrome 54+ and is actually 6-7 times as slow.
> 
> The way the function does it now is the fastest way to do it.

Acknowledged.