Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 4529242486341632: Issue 235 - Improved performance of ElemHide.getSelectorsForDomain() (Closed)

Created:
Jan. 22, 2015, 1 p.m. by Thomas Greiner
Modified:
June 16, 2016, 11:58 a.m.
Reviewers:
Wladimir Palant
Visibility:
Public.

Description

According to my preliminary tests this change increases the performance of the `getSelectorsForDomain()` function by 30-60% (depending on the installed filters and the domains they are applied to).

Patch Set 1 #

Patch Set 2 : Optimized performance of data structure removal #

Total comments: 24

Patch Set 3 : Fixed domain logic and addressed comments #

Total comments: 10

Patch Set 4 : Rebased to 5434ef6e1545 #

Patch Set 5 : Couple of overall improvements #

Total comments: 10

Patch Set 6 : Always consider generic filters #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+102 lines, -16 lines) Patch
M lib/contentPolicy.js View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M lib/elemHide.js View 1 2 3 4 5 6 chunks +101 lines, -15 lines 1 comment Download

Messages

Total messages: 11
Thomas Greiner
Jan. 22, 2015, 1:02 p.m. (2015-01-22 13:02:22 UTC) #1
Wladimir Palant
http://codereview.adblockplus.org/4529242486341632/diff/5741031244955648/lib/elemHide.js File lib/elemHide.js (right): http://codereview.adblockplus.org/4529242486341632/diff/5741031244955648/lib/elemHide.js#newcode141 lib/elemHide.js:141: if (!("nsIStyleSheetService" in Ci)) Looking up things in Components.interfaces ...
Feb. 23, 2015, 7:20 p.m. (2015-02-23 19:20:16 UTC) #2
Thomas Greiner
http://codereview.adblockplus.org/4529242486341632/diff/5741031244955648/lib/elemHide.js File lib/elemHide.js (right): http://codereview.adblockplus.org/4529242486341632/diff/5741031244955648/lib/elemHide.js#newcode141 lib/elemHide.js:141: if (!("nsIStyleSheetService" in Ci)) On 2015/02/23 19:20:16, Wladimir Palant ...
March 12, 2015, 4 p.m. (2015-03-12 16:00:06 UTC) #3
Wladimir Palant
http://codereview.adblockplus.org/4529242486341632/diff/5676830073815040/lib/elemHide.js File lib/elemHide.js (right): http://codereview.adblockplus.org/4529242486341632/diff/5676830073815040/lib/elemHide.js#newcode456 lib/elemHide.js:456: } This block duplicates the logic below unnecessarily. It ...
June 5, 2015, 9:58 p.m. (2015-06-05 21:58:54 UTC) #4
Thomas Greiner
https://codereview.adblockplus.org/4529242486341632/diff/5676830073815040/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/4529242486341632/diff/5676830073815040/lib/elemHide.js#newcode456 lib/elemHide.js:456: } On 2015/06/05 21:58:54, Wladimir Palant wrote: > This ...
June 23, 2015, 1:28 p.m. (2015-06-23 13:28:41 UTC) #5
Wladimir Palant
https://codereview.adblockplus.org/4529242486341632/diff/29321033/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/4529242486341632/diff/29321033/lib/elemHide.js#newcode62 lib/elemHide.js:62: * Lookup table, blocking filter selectors by domain which ...
June 23, 2015, 1:57 p.m. (2015-06-23 13:57:10 UTC) #6
Thomas Greiner
https://codereview.adblockplus.org/4529242486341632/diff/29321033/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/4529242486341632/diff/29321033/lib/elemHide.js#newcode172 lib/elemHide.js:172: selectors[filter.selector].isActive |= domains[domain]; On 2015/06/23 13:57:09, Wladimir Palant wrote: ...
June 23, 2015, 3:11 p.m. (2015-06-23 15:11:41 UTC) #7
Wladimir Palant
https://codereview.adblockplus.org/4529242486341632/diff/29321033/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/4529242486341632/diff/29321033/lib/elemHide.js#newcode172 lib/elemHide.js:172: selectors[filter.selector].isActive |= domains[domain]; The problem is: you would create ...
June 29, 2015, 9:09 a.m. (2015-06-29 09:09:09 UTC) #8
Thomas Greiner
https://codereview.adblockplus.org/4529242486341632/diff/29321033/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/4529242486341632/diff/29321033/lib/elemHide.js#newcode172 lib/elemHide.js:172: selectors[filter.selector].isActive |= domains[domain]; Generally, I might be able to ...
June 29, 2015, 1:03 p.m. (2015-06-29 13:03:45 UTC) #9
Thomas Greiner
https://codereview.adblockplus.org/4529242486341632/diff/29321033/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/4529242486341632/diff/29321033/lib/elemHide.js#newcode62 lib/elemHide.js:62: * Lookup table, blocking filter selectors by domain which ...
July 2, 2015, 9:32 a.m. (2015-07-02 09:32:50 UTC) #10
Wladimir Palant
July 30, 2015, 11:26 a.m. (2015-07-30 11:26:53 UTC) #11
https://codereview.adblockplus.org/4529242486341632/diff/29321269/lib/elemHid...
File lib/elemHide.js (right):

https://codereview.adblockplus.org/4529242486341632/diff/29321269/lib/elemHid...
lib/elemHide.js:464: continue;
Even with the inconsistent logic changes you are implementing, this is still
wrong. !domainSelectors[filterSelector].isActive shouldn't mean "oh well, maybe
some other domain is allowed" - it's really "oops, should not apply, stop now."
Meaning that ~foo.example.com,example.com##foo really shouldn't apply on
foo.example.com even if that filter is allowed on example.com. Same goes for
exceptions, if we have foo.example.com#@#foo then example.com##foo should *not*
apply on foo.example.com.

I recommend adding unit tests for all the scenarios I mention in the review
comments. I feel like you are consistently forgetting to test the edge cases.

Powered by Google App Engine
This is Rietveld