Issue 29886555: Issue 6957 - Add generateStyleSheetForDomain function to ElemHide module (Closed)
|
Created:
Sept. 20, 2018, 12:18 p.m. by Manish Jethani Modified:
Sept. 27, 2018, 5:21 p.m. Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionBased on https://codereview.adblockplus.org/29882562/
Here we add a new generateStyleSheetForDomain function to the ElemHide module. It combines the computation of the selectors that apply to a domain with the generation of the corresponding CSS code, which is made significantly faster now as the default style sheet is cached. It might also help with memory usage, though that is probably no longer an issue with #6967.
Patch Set 1 : #
Total comments: 15
Patch Set 2 : Rebase, remove getSelectorsForDomain #
MessagesTotal messages: 11
Patch Set 1 Please see the description above. I'll update the Trac issue. https://codereview.adblockplus.org/29886555/diff/29886559/lib/elemHide.js File lib/elemHide.js (left): https://codereview.adblockplus.org/29886555/diff/29886559/lib/elemHide.js#old... lib/elemHide.js:207: let selectors = []; This entire block of code is moved into getConditionalSelectorsForDomain https://codereview.adblockplus.org/29886555/diff/29886559/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29886555/diff/29886559/lib/elemHide.js#new... lib/elemHide.js:311: generateStyleSheetForDomain(domain, specificOnly = false) The idea is that calling this function will be faster than calling getSelectorsForDomain followed by createStyleSheet, because this one uses an already cached style sheet. https://codereview.adblockplus.org/29886555/diff/29886559/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29886555/diff/29886559/test/elemHide.js#ne... test/elemHide.js:50: return selectors.slice().sort().filter((selector, index, sortedSelectors) => Somewhat unrelated, but we should make a copy and sort the copy, otherwise it might be confusing if the tests starts to fail because the caller doesn't know that the array is being modified.
https://codereview.adblockplus.org/29886555/diff/29886559/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29886555/diff/29886559/test/elemHide.js#ne... test/elemHide.js:50: return selectors.slice().sort().filter((selector, index, sortedSelectors) => On 2018/09/20 12:27:24, Manish Jethani wrote: > Somewhat unrelated, but we should make a copy and sort the copy, otherwise it > might be confusing if the tests starts to fail because the caller doesn't know > that the array is being modified. Acknowledged. Just curious, since slice returns a shallow copy are there still existing references to the duplicates in the original selectors array and does that impact performance? https://codereview.adblockplus.org/29886555/diff/29886559/test/elemHide.js#ne... test/elemHide.js:75: } I am sort of confused by the set literal `{code, selectors}` Do is hold two Boolean values which are the result of first the deep equal test and then the test.ok? Other than wanting the clarification, can the for loop do without the curly braces? Or is the reasoning behind them that the test line is broken up so the braces help encapsulate it?
https://codereview.adblockplus.org/29886555/diff/29886559/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29886555/diff/29886559/test/elemHide.js#ne... test/elemHide.js:50: return selectors.slice().sort().filter((selector, index, sortedSelectors) => On 2018/09/20 20:10:20, Jon Sonesen wrote: > On 2018/09/20 12:27:24, Manish Jethani wrote: > > Somewhat unrelated, but we should make a copy and sort the copy, otherwise it > > might be confusing if the tests starts to fail because the caller doesn't know > > that the array is being modified. > > Acknowledged. > > Just curious, since slice returns a shallow copy are there still existing > references to the duplicates in the original selectors array and does that > impact performance? The selectors themselves are immutable string objects (strings in JS are immutable), so it should be OK to make a shallow copy. Make a copy itself would affect performance slightly (whatever it costs to make a copy), but over here it doesn't matter since it's a unit test. https://codereview.adblockplus.org/29886555/diff/29886559/test/elemHide.js#ne... test/elemHide.js:75: } On 2018/09/20 20:10:20, Jon Sonesen wrote: > I am sort of confused by the set literal `{code, selectors}` Do is hold two > Boolean values which are the result of first the deep equal test and then the > test.ok? This is called a destructuring assignment [1] [1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/D... Essentially the function returns an object containing `code` and `selectors` properties, and we directly extract those properties into local variables. > Other than wanting the clarification, can the for loop do without the curly > braces? Or is the reasoning behind them that the test line is broken up so the > braces help encapsulate it? There's no technical reason to use the braces, but Dave made a "rule" (more like a convention, it's not an ESLint rule) that if a block spans multiple lines then it should be surrounded by braces. I'm just sticking to this convention. I personally don't care too much about this.
https://codereview.adblockplus.org/29886555/diff/29886559/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29886555/diff/29886559/lib/elemHide.js#new... lib/elemHide.js:311: generateStyleSheetForDomain(domain, specificOnly = false) On 2018/09/20 12:27:24, Manish Jethani wrote: > The idea is that calling this function will be faster than calling > getSelectorsForDomain followed by createStyleSheet, because this one uses an > already cached style sheet. Acknowledged. https://codereview.adblockplus.org/29886555/diff/29886559/lib/elemHide.js#new... lib/elemHide.js:315: createStyleSheet(selectors); Regarding the use of this ternary operation, I wonder how performance critical this particular block is? I ask because I generally prefer strict equals for readability. Notably our code base tends to do a lot of in-lining which in this case ternary fits the visual aspect of this. However, I think using strict if/else is quite a bit more readable I have even heard ternary operations referred to as "ninja code" not that I think it is, but they have a point. Additionally, I got curious about the time differences for processing the two types of evaluations and found an interesting jsperf page[1] Here their results tend to show that if/else with specificOnly and !specificOnly could be up to 77% percent faster (at least based on the truthy/falsy section of the posted link) What do you think? [1]: https://jsperf.com/ternary-vs-simple-if-else https://codereview.adblockplus.org/29886555/diff/29886559/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29886555/diff/29886559/test/elemHide.js#ne... test/elemHide.js:50: return selectors.slice().sort().filter((selector, index, sortedSelectors) => On 2018/09/21 10:53:05, Manish Jethani wrote: > On 2018/09/20 20:10:20, Jon Sonesen wrote: > > On 2018/09/20 12:27:24, Manish Jethani wrote: > > > Somewhat unrelated, but we should make a copy and sort the copy, otherwise > it > > > might be confusing if the tests starts to fail because the caller doesn't > know > > > that the array is being modified. > > > > Acknowledged. > > > > Just curious, since slice returns a shallow copy are there still existing > > references to the duplicates in the original selectors array and does that > > impact performance? > > The selectors themselves are immutable string objects (strings in JS are > immutable), so it should be OK to make a shallow copy. > > Make a copy itself would affect performance slightly (whatever it costs to make > a copy), but over here it doesn't matter since it's a unit test. Yeah I figured since it is test code the importance is minimal also, thanks for explaining :) https://codereview.adblockplus.org/29886555/diff/29886559/test/elemHide.js#ne... test/elemHide.js:75: } On 2018/09/21 10:53:05, Manish Jethani wrote: > On 2018/09/20 20:10:20, Jon Sonesen wrote: > > I am sort of confused by the set literal `{code, selectors}` Do is hold two > > Boolean values which are the result of first the deep equal test and then the > > test.ok? > > This is called a destructuring assignment [1] > > [1]: > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/D... > > Essentially the function returns an object containing `code` and `selectors` > properties, and we directly extract those properties into local variables. > > > Other than wanting the clarification, can the for loop do without the curly > > braces? Or is the reasoning behind them that the test line is broken up so the > > braces help encapsulate it? > > There's no technical reason to use the braces, but Dave made a "rule" (more like > a convention, it's not an ESLint rule) that if a block spans multiple lines then > it should be surrounded by braces. I'm just sticking to this convention. I > personally don't care too much about this. I do think the convention is helpful tbh so I was curious, perhaps it could be added to the eslint-eyeo-config package? I think it's easy enough to remember but would be nice to have my linter plugin tell me hehe.
https://codereview.adblockplus.org/29886555/diff/29886559/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29886555/diff/29886559/lib/elemHide.js#new... lib/elemHide.js:315: createStyleSheet(selectors); On 2018/09/23 18:01:59, Jon Sonesen wrote: > Regarding the use of this ternary operation, I wonder how performance critical > this particular block is? This function is supposed to be very performance critical, in fact it has been optimized and micro-optimized over time. Let me try to run some tests and get back. > I ask because I generally prefer strict equals for readability. Notably our code > base tends to do a lot of in-lining which in this case ternary fits the visual > aspect of this. However, I think using strict if/else is quite a bit more > readable I have even heard ternary operations referred to as "ninja code" not > that I think it is, but they have a point. Fair enough, makes sense. Let me update the patch, along with some other changes. > Additionally, I got curious about the time differences for processing the two > types of evaluations and found an interesting jsperf page[1] Here their results > tend to show that if/else with specificOnly and !specificOnly could be up to 77% > percent faster (at least based on the truthy/falsy section of the posted link) > > What do you think? > > [1]: https://jsperf.com/ternary-vs-simple-if-else The way I'm reading this, it seems we only need to look at the last two here (truthy/falsy vs. ternary truthy/falsy). On my machine here, truthy/falsy is the fastest, while ternary truthy/falsy is 5% slower. Nevertheless, let me actually test this implementation itself.
https://codereview.adblockplus.org/29886555/diff/29886559/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29886555/diff/29886559/lib/elemHide.js#new... lib/elemHide.js:315: createStyleSheet(selectors); On 2018/09/24 11:51:14, Manish Jethani wrote: > On 2018/09/23 18:01:59, Jon Sonesen wrote: > > Regarding the use of this ternary operation, I wonder how performance critical > > this particular block is? > > This function is supposed to be very performance critical, in fact it has been > optimized and micro-optimized over time. > > Let me try to run some tests and get back. > > > I ask because I generally prefer strict equals for readability. Notably our > code > > base tends to do a lot of in-lining which in this case ternary fits the visual > > aspect of this. However, I think using strict if/else is quite a bit more > > readable I have even heard ternary operations referred to as "ninja code" not > > that I think it is, but they have a point. > > Fair enough, makes sense. Let me update the patch, along with some other > changes. > > > Additionally, I got curious about the time differences for processing the two > > types of evaluations and found an interesting jsperf page[1] Here their > results > > tend to show that if/else with specificOnly and !specificOnly could be up to > 77% > > percent faster (at least based on the truthy/falsy section of the posted link) > > > > What do you think? > > > > [1]: https://jsperf.com/ternary-vs-simple-if-else > > The way I'm reading this, it seems we only need to look at the last two here > (truthy/falsy vs. ternary truthy/falsy). On my machine here, truthy/falsy is the > fastest, while ternary truthy/falsy is 5% slower. Nevertheless, let me actually > test this implementation itself. Ah yeah, that's a good point here. My bad, haha I misread the ternary truthy/falsy also: it seems that the result is not 100% reliable performance wise today I ran the jsperf thing a few different times and got different results, also I am aware that jsperf is not the best benchmark as there are many factors at play regarding time efficiency that jsperf is not able to recreate especially since our use case is significantly more complex. So I am basically saying that my heart is not set on this being if/else at this point especially if it is in fact less efficient.
LGTM
Patch Set 2: Rebase, remove getSelectorsForDomain Since Sebastian has agreed to use the new function, we don't need the old function anymore.
https://codereview.adblockplus.org/29886555/diff/29886559/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29886555/diff/29886559/lib/elemHide.js#new... lib/elemHide.js:315: createStyleSheet(selectors); On 2018/09/24 15:09:44, Jon Sonesen wrote: > On 2018/09/24 11:51:14, Manish Jethani wrote: > > On 2018/09/23 18:01:59, Jon Sonesen wrote: > > > Regarding the use of this ternary operation, I wonder how performance > critical > > > this particular block is? > > > > This function is supposed to be very performance critical, in fact it has been > > optimized and micro-optimized over time. > > > > Let me try to run some tests and get back. > > > > > I ask because I generally prefer strict equals for readability. Notably our > > code > > > base tends to do a lot of in-lining which in this case ternary fits the > visual > > > aspect of this. However, I think using strict if/else is quite a bit more > > > readable I have even heard ternary operations referred to as "ninja code" > not > > > that I think it is, but they have a point. > > > > Fair enough, makes sense. Let me update the patch, along with some other > > changes. > > > > > Additionally, I got curious about the time differences for processing the > two > > > types of evaluations and found an interesting jsperf page[1] Here their > > results > > > tend to show that if/else with specificOnly and !specificOnly could be up to > > 77% > > > percent faster (at least based on the truthy/falsy section of the posted > link) > > > > > > What do you think? > > > > > > [1]: https://jsperf.com/ternary-vs-simple-if-else > > > > The way I'm reading this, it seems we only need to look at the last two here > > (truthy/falsy vs. ternary truthy/falsy). On my machine here, truthy/falsy is > the > > fastest, while ternary truthy/falsy is 5% slower. Nevertheless, let me > actually > > test this implementation itself. > > Ah yeah, that's a good point here. My bad, haha I misread the ternary > truthy/falsy also: it seems that the result is not 100% reliable performance > wise today I ran the jsperf thing a few different times and got different > results, also I am aware that jsperf is not the best benchmark as there are many > factors at play regarding time efficiency that jsperf is not able to recreate > especially since our use case is significantly more complex. > > So I am basically saying that my heart is not set on this being if/else at this > point especially if it is in fact less efficient. Acknowledged. https://codereview.adblockplus.org/29886555/diff/29886559/lib/elemHide.js#new... lib/elemHide.js:315: createStyleSheet(selectors); On 2018/09/24 11:51:14, Manish Jethani wrote: > On 2018/09/23 18:01:59, Jon Sonesen wrote: > > What do you think? > > > > [1]: https://jsperf.com/ternary-vs-simple-if-else > > The way I'm reading this, it seems we only need to look at the last two here > (truthy/falsy vs. ternary truthy/falsy). On my machine here, truthy/falsy is the > fastest, while ternary truthy/falsy is 5% slower. Nevertheless, let me actually > test this implementation itself. So I tested this by the way, it seems the Patch Set 2 with ternary operator is the best option.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
