Issue 5735 - Use JS Map instead of Object for matcher properties filterByKeyword and keywordByFilter

[sergei, 2017-09-21 13:22:21]

https://codereview.adblockplus.org/29551648/diff/29551649/lib/matcher.js
File lib/matcher.js (right):

https://codereview.adblockplus.org/29551648/diff/29551649/lib/matcher.js#newc...
lib/matcher.js:46: * @type {Map.<string,string>}
I will check the difference between Map.<string,string> and Map.<Filter,string>
separately.

https://codereview.adblockplus.org/29551648/diff/29551649/lib/matcher.js#newc...
lib/matcher.js:48: keywordByFilter: null,
strictly speaking the change of type of keywordByFilter could be separated from
the change of type of filterByKeyword but it seems not very interesting from any
performance point of view, because they are very similar and there is no
comparison between "many Object objects" and "many Map objects". However, if
someone wants it then please let me know and I extract it into another
codereview.

https://codereview.adblockplus.org/29551648/diff/29551649/lib/matcher.js#newc...
lib/matcher.js:189: if (typeof list == "undefined")
This change is additional to the change of type. Previously a caller didn't call
_checkEntryMatch with an unknown by filterByKeyword keyword, however it's not
only about performance (no double look up) but also about design because it
reduces the number of places of work with filterByKeyword.

[Wladimir Palant, 2017-09-25 11:07:46]

https://codereview.adblockplus.org/29551648/diff/29551649/lib/matcher.js
File lib/matcher.js (right):

https://codereview.adblockplus.org/29551648/diff/29551649/lib/matcher.js#newc...
lib/matcher.js:46: * @type {Map.<string,string>}
On 2017/09/21 13:22:21, sergei wrote:
> I will check the difference between Map.<string,string> and
Map.<Filter,string>
> separately.

I'd prefer to have this change done in the same go.

https://codereview.adblockplus.org/29551648/diff/29551649/lib/matcher.js#newc...
lib/matcher.js:171: return typeof keyword != "undefined" ? keyword : null;
It seems that this can be simplified:

  return this.keywordByFilter.get(filter.text) || null;

[sergei, 2017-09-25 13:46:34]

https://codereview.adblockplus.org/29551648/diff/29551649/lib/matcher.js
File lib/matcher.js (right):

https://codereview.adblockplus.org/29551648/diff/29551649/lib/matcher.js#newc...
lib/matcher.js:46: * @type {Map.<string,string>}
On 2017/09/25 11:07:46, Wladimir Palant wrote:
> On 2017/09/21 13:22:21, sergei wrote:
> > I will check the difference between Map.<string,string> and
> Map.<Filter,string>
> > separately.
> 
> I'd prefer to have this change done in the same go.

Done.

I have checked it and there is no visible difference in the memory usage.

https://codereview.adblockplus.org/29551648/diff/29551649/lib/matcher.js#newc...
lib/matcher.js:171: return typeof keyword != "undefined" ? keyword : null;
On 2017/09/25 11:07:46, Wladimir Palant wrote:
> It seems that this can be simplified:
> 
>   return this.keywordByFilter.get(filter.text) || null;

It changes the behavior because if the value of `keyword` is the empty string ""
then it returns `null` instead of that empty string. It may be a correct value
(BTW, tests fail but they can be fixed) but I'm not sure about it because it's
not clear what this method is actually for. I would propose to make it in a
separate commit what should simplify the search for a regression if there is
one.

So far, I see that `Matcher.getKeywordForFilter` is used by
`CombinedMatcher.getKeywordForFilter` which seem not used and by
`CombinedMatcher.isSlowFilter` for which it's a correct value. I would propose
to make that change, remove `CombinedMatcher.getKeywordForFilter` and maybe
remove `matcher.hasFilter` in a separate commit.

[Wladimir Palant, 2017-09-26 11:11:20]

LGTM