Issue 29774573: Issue 6652 - Do not push to unconditional selectors array (Closed)
|
Created:
May 8, 2018, 4:53 p.m. by Manish Jethani Modified:
May 11, 2018, 1:08 p.m. Reviewers:
kzar CC:
sergei Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
Patch Set 1 #Patch Set 2 : Make getUnconditionalSelectors private #
Total comments: 1
Patch Set 3 : Clean up code #
Total comments: 1
Patch Set 4 : Remove unrelated changes #Patch Set 5 : Remove blank line #
Total comments: 3
Patch Set 6 : Rebase #Patch Set 7 : Rebase on patch #29778572 #MessagesTotal messages: 13
Patch Set 1 I found out the real reason why separating out the while loop here speeds up the function 2x. It's not because it's a separate function, it's because it has its own selectors array which it populates instead of touching the unconditional selectors array. My hypothesis is that this doubles the speed because initially when we call Array.slice on the unconditionalSelectors array, it just creates a new object but doesn't make a copy. The copy is made only when the array is actually modified - which it is in the while loop. If we populate a separate array and then concatenate the two, it turns out to be more efficient. The exact reason is still not entirely clear to me. Also I have only tested this on Chrome, let me test on Firefox and get back.
On 2018/05/08 16:57:30, Manish Jethani wrote: > Also I have only tested this on Chrome, let me test on Firefox and get back. Done. I was right, it's the same on Firefox.
Patch Set 2: Make getUnconditionalSelectors private I'm not sure why this function was exposed on the module, it wasn't being called from anywhere and there were no tests for it specifically. We should not expose the internal logic of the module unless it's intentional. https://codereview.adblockplus.org/29774573/diff/29774576/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29774573/diff/29774576/lib/elemHide.js#new... lib/elemHide.js:73: return unconditionalSelectors; Note that now that's this function is private to the module there's no need to create a copy. The caller can decide what's best. For example in the current patch the caller is using Array.concat, which creates a copy anyway.
Patch Set 3: Clean up code https://codereview.adblockplus.org/29774573/diff/29774578/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29774573/diff/29774578/lib/elemHide.js#new... lib/elemHide.js:245: getSelectorsForDomain(domain, criteria = ElemHide.ALL_MATCHING) This is the modern way of specifying default values for parameters.
Patch Set 4: Remove unrelated changes Also I've updated the Trac issue to #6652.
Patch Set 5: Remove blank line Cleaner diff.
https://codereview.adblockplus.org/29774573/diff/29775595/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29774573/diff/29775595/lib/elemHide.js#new... lib/elemHide.js:66: * Returns a list of selectors that apply on each website unconditionally. Please could you also remove this unrelated change? I agree that we might as well put functions that don't need to be used elsewhere outside of the ElemHide Object, but that also applies to _addFiltersByDomain for example and can be done in a separate change.
https://codereview.adblockplus.org/29774573/diff/29775595/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29774573/diff/29775595/lib/elemHide.js#new... lib/elemHide.js:66: * Returns a list of selectors that apply on each website unconditionally. On 2018/05/10 10:59:49, kzar wrote: > Please could you also remove this unrelated change? I agree that we might as > well put functions that don't need to be used elsewhere outside of the ElemHide > Object, but that also applies to _addFiltersByDomain for example and can be done > in a separate change. Well if we're going to keep the function available on the ElemHide object then we'll have to call Array.slice on the result. Removing that call is part of the optimization, but we can only remove it if it's made private to the module (otherwise anyone can mess with the internal state of the module). Or I could leave the function on the ElemHide object but still remove the call to Array.slice. What do you think? Making the function private here was not just about cleaning up code, there was a more specific reason for it that is related to the optimizations we're trying to do.
Patch Set 6: Rebase
https://codereview.adblockplus.org/29774573/diff/29775595/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29774573/diff/29775595/lib/elemHide.js#new... lib/elemHide.js:66: * Returns a list of selectors that apply on each website unconditionally. On 2018/05/11 04:07:10, Manish Jethani wrote: > Well if we're going to keep the function available on the ElemHide object then > we'll have to call Array.slice on the result. Removing that call is part of the > optimization, but we can only remove it if it's made private to the module > (otherwise anyone can mess with the internal state of the module). Gotya, OK that makes sense. This LGTM then, I'd prefer you moved all the private stuff out of the ElemHide Object first as a separate Noissue change, then made this change. But I'll leave it up to you.
Patch Set 7: Rebase on patch #29778572
Nice, LGTM 
|
