Issue 30022555: Issue 7324 - Simplify logic for domains property (Closed)
|
Created:
March 4, 2019, 9:47 a.m. by Manish Jethani Modified:
March 6, 2019, 6:28 p.m. Reviewers:
hub Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
Patch Set 1 : #
Total comments: 7
MessagesTotal messages: 6
Patch Set 1 https://codereview.adblockplus.org/30022555/diff/30022562/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/30022555/diff/30022562/lib/filterClasses.j... lib/filterClasses.js:415: this.domainSource = domains.toLowerCase(); Lower-case in advance during initialization. https://codereview.adblockplus.org/30022555/diff/30022562/lib/filterClasses.j... lib/filterClasses.js:502: let {domainSource} = this; Tip: The diff of this function is smaller with the `-b` flag. I flattened it (less nesting, multiple return statements). https://codereview.adblockplus.org/30022555/diff/30022562/test/filterClasses.js File test/filterClasses.js (left): https://codereview.adblockplus.org/30022555/diff/30022562/test/filterClasses.... test/filterClasses.js:634: // For both request blocking and element hiding filters, the value of the The tests are also simplified now.
LGTM https://codereview.adblockplus.org/30022555/diff/30022562/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/30022555/diff/30022562/lib/filterClasses.j... lib/filterClasses.js:415: this.domainSource = domains.toLowerCase(); On 2019/03/04 12:02:54, Manish Jethani wrote: > Lower-case in advance during initialization. Do we know if there is a performance difference between domains already being all lowercase and not? Whether it is worth, in addition to this, to ensure the filters (lists) are normalized.
https://codereview.adblockplus.org/30022555/diff/30022562/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/30022555/diff/30022562/lib/filterClasses.j... lib/filterClasses.js:415: this.domainSource = domains.toLowerCase(); On 2019/03/06 13:10:40, hub wrote: > On 2019/03/04 12:02:54, Manish Jethani wrote: > > Lower-case in advance during initialization. > > Do we know if there is a performance difference between domains already being > all lowercase and not? Whether it is worth, in addition to this, to ensure the > filters (lists) are normalized. You mean that we could require that the domains are already lower-cased in the filter list so we don't have to lower-case them? I don't know if it would make much of a difference to the loading of the filters. I also do expect that we could make lots of other improvements that would give us a much bigger speed boost for the initial loading of the filters.
Message was sent while issue was closed.
https://codereview.adblockplus.org/30022555/diff/30022562/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/30022555/diff/30022562/lib/filterClasses.j... lib/filterClasses.js:415: this.domainSource = domains.toLowerCase(); On 2019/03/06 18:11:10, Manish Jethani wrote: > On 2019/03/06 13:10:40, hub wrote: > > On 2019/03/04 12:02:54, Manish Jethani wrote: > > > Lower-case in advance during initialization. > > > > Do we know if there is a performance difference between domains already being > > all lowercase and not? Whether it is worth, in addition to this, to ensure the > > filters (lists) are normalized. > > You mean that we could require that the domains are already lower-cased in the > filter list so we don't have to lower-case them? I don't know if it would make > much of a difference to the loading of the filters. I also do expect that we > could make lots of other improvements that would give us a much bigger speed > boost for the initial loading of the filters. I was just wondering if toLowerCase() is faster when fed already lowercase strings, and in that case if that would be worth it for the filter lists to be normalized. Just an idea here.
Message was sent while issue was closed.
https://codereview.adblockplus.org/30022555/diff/30022562/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/30022555/diff/30022562/lib/filterClasses.j... lib/filterClasses.js:415: this.domainSource = domains.toLowerCase(); On 2019/03/06 18:14:25, hub wrote: > On 2019/03/06 18:11:10, Manish Jethani wrote: > > On 2019/03/06 13:10:40, hub wrote: > > > On 2019/03/04 12:02:54, Manish Jethani wrote: > > > > Lower-case in advance during initialization. > > > > > > Do we know if there is a performance difference between domains already > being > > > all lowercase and not? Whether it is worth, in addition to this, to ensure > the > > > filters (lists) are normalized. > > > > You mean that we could require that the domains are already lower-cased in the > > filter list so we don't have to lower-case them? I don't know if it would make > > much of a difference to the loading of the filters. I also do expect that we > > could make lots of other improvements that would give us a much bigger speed > > boost for the initial loading of the filters. > > I was just wondering if toLowerCase() is faster when fed already lowercase > strings, and in that case if that would be worth it for the filter lists to be > normalized. Just an idea here. Ah, I see what you mean. I don't expect it to be faster for strings that are already lower-cased. If that were the case, we wouldn't benefit from the change in changeset 0c5fe8fe506f. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
