Issue 29743730: Issue 6559 - Use maps and sets where appropriate (Closed)
|
Created:
April 5, 2018, 5:38 p.m. by Manish Jethani Modified:
April 6, 2018, 2:37 p.m. Reviewers:
kzar CC:
sergei Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
Patch Set 1 #
Total comments: 14
Patch Set 2 : Update comments #
Total comments: 2
Patch Set 3 : Revert logic in getUnconditionalFilterKeys #
MessagesTotal messages: 8
Patch Set 1 https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js#new... lib/elemHide.js:94: for (let collection of [keyByFilter, filtersByDomain, filterKeyBySelector, Note that there's no need to create new objects for each of these, we can just call the clear method to start over. https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js#new... lib/elemHide.js:231: let list = exceptions.get(filter.selector); If list is falsy we know it's undefined, since we only ever set it to an array. https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js#new... lib/elemHide.js:298: if (!unconditionalFilterKeys) Essentially we're getting the values of filterKeyBySelector, we can just use the values method here.
https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js#new... lib/elemHide.js:73: * Lookup table, keys are known element hiding exceptions Nit: This comment reads a bit weirdly now that it's a Set IMO https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js#new... lib/elemHide.js:231: let list = exceptions.get(filter.selector); On 2018/04/06 05:36:16, Manish Jethani wrote: > If list is falsy we know it's undefined, since we only ever set it to an array. Acknowledged. https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js#new... lib/elemHide.js:298: if (!unconditionalFilterKeys) On 2018/04/06 05:36:16, Manish Jethani wrote: > Essentially we're getting the values of filterKeyBySelector, we can just use the > values method here. Previously we were caching though. But come to think of it I don't think the provideFilterKeys parameter is used any more since we switched to Firefox WebExt. I just had a look through the codebases to confirm as well. If so we can remove this anyway. Could you double check too? https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js#new... lib/elemHide.js:364: if (seenFilters.has(filterKey)) This is a hotspot, we spent quite some time getting it to run faster. So please be careful to test your changes didn't slow things down. Looking through my notes one (simplistic) test we used was this: (function() { var start = performance.now(); for (var i = 0; i < 1000; i += 1) ElemHide.getSelectorsForDomain("www.extremetech.com"); return performance.now() - start; }()); (I'm not against the change, I think it might even speed things up. At the time IIRC we wanted to use a Map / Set but coudn't due to the old Safari versions we supported.)
Patch Set 2: Update comments https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js#new... lib/elemHide.js:73: * Lookup table, keys are known element hiding exceptions On 2018/04/06 11:42:20, kzar wrote: > Nit: This comment reads a bit weirdly now that it's a Set IMO Done. https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js#new... lib/elemHide.js:298: if (!unconditionalFilterKeys) On 2018/04/06 11:42:20, kzar wrote: > On 2018/04/06 05:36:16, Manish Jethani wrote: > > Essentially we're getting the values of filterKeyBySelector, we can just use > the > > values method here. > > Previously we were caching though. > > But come to think of it I don't think the provideFilterKeys parameter is used > any more since we switched to Firefox WebExt. I just had a look through the > codebases to confirm as well. If so we can remove this anyway. Could you double > check too? So it looks like we could do away with the concept of filter keys altogether? Now since we have maps/sets we don't need to generate keys, we can directly look up by the filter in a map/set. Anyway, all of this should be a separate patch. Yes I checked, at least provideFilterKeys isn't used anywhere. https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js#new... lib/elemHide.js:364: if (seenFilters.has(filterKey)) On 2018/04/06 11:42:20, kzar wrote: > This is a hotspot, we spent quite some time getting it to run faster. So please > be careful to test your changes didn't slow things down. Looking through my > notes one (simplistic) test we used was this: > > (function() { > var start = performance.now(); > for (var i = 0; i < 1000; i += 1) > ElemHide.getSelectorsForDomain("www.extremetech.com"); > return performance.now() - start; > }()); > > > (I'm not against the change, I think it might even speed things up. At the time > IIRC we wanted to use a Map / Set but coudn't due to the old Safari versions we > supported.) I ran this particular test and it's 2s vs 1.6s on Chrome and 6s vs. 3.8s on Firefox. On both platforms the new code performs faster. https://codereview.adblockplus.org/29743730/diff/29744598/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29743730/diff/29744598/lib/elemHide.js#new... lib/elemHide.js:29: * @type {Filter[]} I've updated some of the types here. https://codereview.adblockplus.org/29743730/diff/29744598/lib/elemHide.js#new... lib/elemHide.js:35: * @type {Map.<Filter,number>} The key is in fact a number.
https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js#new... lib/elemHide.js:298: if (!unconditionalFilterKeys) On 2018/04/06 12:44:09, Manish Jethani wrote: > Yes I checked, at least provideFilterKeys isn't used anywhere. Cool, so looks like we could remove that parameter and associated stuff like getUncoditionalFilterKeys. > So it looks like we could do away with the concept of filter keys > altogether? Now since we have maps/sets we don't need to generate keys, > we can directly look up by the filter in a map/set. Good point, I think you might be right. I'll open an issue for that and get a review together, unless you mind? > Anyway, all of this should be a separate patch. Yea, agreed. But in that case please could you put the logic of getUncoditionalFilterKeys back to how it was (except where it needed to be different since Objects were replaced with Maps/Sets). https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js#new... lib/elemHide.js:364: if (seenFilters.has(filterKey)) On 2018/04/06 12:44:09, Manish Jethani wrote: > On 2018/04/06 11:42:20, kzar wrote: > > This is a hotspot, we spent quite some time getting it to run faster. So > please > > be careful to test your changes didn't slow things down. Looking through my > > notes one (simplistic) test we used was this: > > > > (function() { > > var start = performance.now(); > > for (var i = 0; i < 1000; i += 1) > > ElemHide.getSelectorsForDomain("www.extremetech.com"); > > return performance.now() - start; > > }()); > > > > > > (I'm not against the change, I think it might even speed things up. At the > time > > IIRC we wanted to use a Map / Set but coudn't due to the old Safari versions > we > > supported.) > > I ran this particular test and it's 2s vs 1.6s on Chrome and 6s vs. 3.8s on > Firefox. On both platforms the new code performs faster. Acknowledged.
Patch Set 3: Revert logic in getUnconditionalFilterKeys https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js#new... lib/elemHide.js:298: if (!unconditionalFilterKeys) On 2018/04/06 13:07:54, kzar wrote: > On 2018/04/06 12:44:09, Manish Jethani wrote: > > Yes I checked, at least provideFilterKeys isn't used anywhere. > > Cool, so looks like we could remove that parameter and associated stuff like > getUncoditionalFilterKeys. > > > So it looks like we could do away with the concept of filter keys > > altogether? Now since we have maps/sets we don't need to generate keys, > > we can directly look up by the filter in a map/set. > > Good point, I think you might be right. I'll open an issue for that and get a > review together, unless you mind? Sure, go ahead. > > Anyway, all of this should be a separate patch. > > Yea, agreed. But in that case please could you put the logic of > getUncoditionalFilterKeys back to how it was (except where it needed to be > different since Objects were replaced with Maps/Sets). Done.
https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29743730/diff/29743731/lib/elemHide.js#new... lib/elemHide.js:364: if (seenFilters.has(filterKey)) On 2018/04/06 12:44:09, Manish Jethani wrote: > On 2018/04/06 11:42:20, kzar wrote: > > This is a hotspot, we spent quite some time getting it to run faster. So > please > > be careful to test your changes didn't slow things down. Looking through my > > notes one (simplistic) test we used was this: > > > > (function() { > > var start = performance.now(); > > for (var i = 0; i < 1000; i += 1) > > ElemHide.getSelectorsForDomain("www.extremetech.com"); > > return performance.now() - start; > > }()); > > > > > > (I'm not against the change, I think it might even speed things up. At the > time > > IIRC we wanted to use a Map / Set but coudn't due to the old Safari versions > we > > supported.) > > I ran this particular test and it's 2s vs 1.6s on Chrome and 6s vs. 3.8s on > Firefox. On both platforms the new code performs faster. BTW I tried replacing `filterKey in seenFilters` with `[].hasOwnProperty.call(seenFilters, filterKey)` and then just `seenFilters[filterKey]`, in the latter case the performance comes close to Set, but Set still performs the best.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
