Issue 29342830: Issue 4057 - Further speedup ElemHide.getSelectorsforDomain (Closed)
DescriptionIssue 4057 - Further speedup ElemHide.getSelectorsforDomain
Patch Set 1 #
Total comments: 2
Patch Set 2 : Take care to remove filters from lookups #
Total comments: 8
Patch Set 3 : Further improvements #Patch Set 4 : Rebased, unconditionalSelectors #
Total comments: 18
Patch Set 5 : Speed up ElemHide.remove #Patch Set 6 : Addressed comments #
Total comments: 9
Patch Set 7 : Avoid extra dirty variable #
Total comments: 4
Patch Set 8 : Addressed nits #MessagesTotal messages: 38
Patch Set 1
https://codereview.adblockplus.org/29342830/diff/29342831/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342831/lib/elemHide.js#new... lib/elemHide.js:178: if (filterId == undefined) I guess we can remove filterIdByFilterText lookup and the related code here if we're sure the same filter won't be added more than once?
Patch Set 2 : Take care to remove filters from lookups https://codereview.adblockplus.org/29342830/diff/29342831/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342831/lib/elemHide.js#new... lib/elemHide.js:178: if (filterId == undefined) On 2016/05/20 08:11:12, kzar wrote: > I guess we can remove filterIdByFilterText lookup and the related code here if > we're sure the same filter won't be added more than once? Nevermind, pretty sure we need it https://codereview.adblockplus.org/29342830/diff/29342834/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342834/lib/elemHide.js#new... lib/elemHide.js:233: if (usingGetSelectorsByDomain) I'm not too happy with this change it's kind of slow. When disabling large subscriptions like EasyList that might be a problem. (Come to think of it we also forgot to remove things from the lookup with the earlier changes for this issue...)
There are two parts here: 1) Referencing filters by numerical ID rather than text. 2) Storing filters matching/disabled on a domain in two arrays rather than a single object. I can see how the latter improves performance, and I already thought of doing something like this. What about the former? It adds significant complexity, is it actually necessary to improve performance? If it does, what about using existing keyByFilter/filterByKey lookup tables? https://codereview.adblockplus.org/29342830/diff/29342834/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342834/lib/elemHide.js#new... lib/elemHide.js:178: if (filterId == undefined) That's one of the few cases where you need ===, because null == undefined evaluates to true. https://codereview.adblockplus.org/29342830/diff/29342834/lib/elemHide.js#new... lib/elemHide.js:241: let domains = Object.keys(filter.domains || defaultDomains); Why use Object.keys() here all the sudden rather than for..in loop?
https://codereview.adblockplus.org/29342830/diff/29342834/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342834/lib/elemHide.js#new... lib/elemHide.js:178: if (filterId == undefined) We always use typeof when checking for undefined. https://codereview.adblockplus.org/29342830/diff/29342834/lib/elemHide.js#new... lib/elemHide.js:185: Nit: It seems the code reads better without the blakn line here.
On 2016/05/20 12:33:27, Wladimir Palant wrote: > There are two parts here: > > 1) Referencing filters by numerical ID rather than text. > 2) Storing filters matching/disabled on a domain in two arrays rather than a > single object. > > I can see how the latter improves performance, and I already thought of doing > something like this. What about the former? It adds significant complexity, is > it actually necessary to improve performance? If it does, what about using > existing keyByFilter/filterByKey lookup tables? OK I gave those ideas a shot: Code before my changes: Self: 27400ms, Total: 33919ms Code as of Patch Set 2: Self: 3389, Total: 9441ms Using two arrays but with filter.text[1]: Self: 17295ms, Total: 23446ms Using two arrays with filterKey[2]: Self: 18898ms, Total: 26066ms I'm not really sure what causes those results. I suspect that the `for (let filterKey of activeFilters)` is the critical part for performance, and so the less work we do there in order to get the selector the better the performance will be. The code as of Patch Set 2 grabs the selector straight from an array, the code using filterKey has to perform an extra lookup like filterByKey[key].selector. That doesn't explain the bad performance of using two arrays but with filter.text however. I'll continue to investigate... [1] https://github.com/kzar/adblockpluscore/commit/c4a5250 [2] https://github.com/kzar/adblockpluscore/commit/965622c
I'm not entirely surprised that the code is quite a bit faster if we have integers instead of strings. The JIT is used to generate much faster code for integers than it does when using strings. In particular when using exclusively integers as properties on an object, even though in interpreted mode they just get converted to strings, in JIT mode a more efficient internal class is used for that object. Also considering that we already identified the seenFilters object previously as a kind of bottleneck, that makes sense to me.
Patch Set 3 : Further improvements https://codereview.adblockplus.org/29342830/diff/29342834/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342834/lib/elemHide.js#new... lib/elemHide.js:178: if (filterId == undefined) On 2016/05/20 13:03:04, Sebastian Noack wrote: > We always use typeof when checking for undefined. Acknowledged. https://codereview.adblockplus.org/29342830/diff/29342834/lib/elemHide.js#new... lib/elemHide.js:185: On 2016/05/20 13:03:03, Sebastian Noack wrote: > Nit: It seems the code reads better without the blakn line here. Done. https://codereview.adblockplus.org/29342830/diff/29342834/lib/elemHide.js#new... lib/elemHide.js:241: let domains = Object.keys(filter.domains || defaultDomains); On 2016/05/20 12:33:27, Wladimir Palant wrote: > Why use Object.keys() here all the sudden rather than for..in loop? Done.
On 2016/05/20 17:16:41, Sebastian Noack wrote: > I'm not entirely surprised that the code is quite a bit faster if we have > integers instead of strings... I think you're right, but I also think it helps that selectorByFilterId is an array. When I adjust my previous attempt at using filterKey to do the same (turning filterByKey into an array) I get similar performance (maybe 10% slower). I'll update the review when I get a chance, I think we can definitely use filterByKey instead of adding a new ID for filters and I guess we can decide if adding a selectorByFilterkey lookup is worth a further ~10% improvement.
On 2016/05/21 06:08:57, kzar wrote: > I'll update the review when I get a chance, I think we can definitely use > filterByKey instead of adding a new ID for filters and I guess we can decide if > adding a selectorByFilterkey lookup is worth a further ~10% improvement. Yes, please use filterByKey. If it has to be numerical then you can still change the way the key is generated. Math.random() * 0xFFFFFFFF will do to generate random numerical IDs (filter keys have to be non-continuous because websites can potentially read them out on Firefox but shouldn't be able to make any conclusions from that).
On 2016/05/21 07:12:28, Wladimir Palant wrote: > On 2016/05/21 06:08:57, kzar wrote: > > I'll update the review when I get a chance, I think we can definitely use > > filterByKey instead of adding a new ID for filters and I guess we can decide > if > > adding a selectorByFilterkey lookup is worth a further ~10% improvement. > > Yes, please use filterByKey. If it has to be numerical then you can still change > the way the key is generated. Math.random() * 0xFFFFFFFF will do to generate > random numerical IDs (filter keys have to be non-continuous because websites can > potentially read them out on Firefox but shouldn't be able to make any > conclusions from that). Ah damn, I didn't realise that. Well I think even when using numerical keys like you described but with an object for the filterByKey lookup instead of an array it will slow down getSelectorsForDomain. I'll double check that when I get a chance though. If my fears are correct and we stick with this selctorByFilterId array, will it be possible for malefactors to abuse the sequential IDs like they could with filterByKey? (I don't really understand how that works.)
Actually, there is another change you should try first. The current
implementation is slow because it has to iterate over lots of filters for the ""
domain. Yet most of these filters apply unconditionally, on any domain. So we
need a separate array of selectors which are returned every time, and
getSelectorsByDomain() merely needs to copy that one before doing the more
complicated logic for all the other filters. Note that we will need this data
structure on Firefox anyway, once we split up the monolithic stylesheet there.
Here is how this can work (pseudo-code for the add function):
if not exception and !filter.domains && !hasExceptions(filter.selector):
if filter.selector in filtersBySelector:
// Selector already in list
filtersBySelector[filter.selector].push(filter)
else:
// New selector
filtersBySelector[filter.selector] = [filter]
unconditionalSelectors.push(filter.selector)
else if exception:
if filter.selector in filtersBySelector:
// This selector is no longer unconditional, move corresponding filters
for f of filtersBySelector[filter.selector]:
filtersByDomain[""][f.text] = f
delete filtersBySelector[filter.selector]
unconditionalSelectors.remove(filter.selector)
else:
// Add filter to usual per-domain data structures
Please try it, this might actually produce a sufficient speed-up that the other
parts (splitting into two arrays and numerical IDs) are no longer necessary.
Cool idea! I just got it working, I found it sped things up but not as much: Code before my changes: Self: 27400ms, Total: 33919ms Code as of Patch Set 2: Self: 3389, Total: 9441ms Code tracking unconditional selectors[1]: Self: 14496ms, Total: 16890ms I'm going to try and combine the approaches, maybe we can get something working that's even faster. [1] https://github.com/kzar/adblockpluscore/commit/1d0e720
Wow, got the fastest result yet by combining your idea with using filterByKey lookup for the seenFilters check[1]. My test ran in Self: 4174ms, Total: 6127ms which is something like a 5.5x speedup. Problem is it only worked when using an array for filterByKey and therefore sequential keys. Using `Math.random() * 0xFFFFFFFF` (or even `Math.floor(Math.random() * 0xFFFFFFFF`) and an Object was still slow. Any idea how we can use an array lookup like this without websites being able to read the filters out in Firefox? [1] https://github.com/adblockplus/adblockpluscore/commit/75b15eb
On 2016/05/23 13:28:27, kzar wrote: > Problem is it only worked when using an array for filterByKey and therefore > sequential keys. Using `Math.random() * 0xFFFFFFFF` (or even > `Math.floor(Math.random() * 0xFFFFFFFF`) and an Object was still slow. What about reducing the number of bits, e.g. multiply with 0x00FFFFFF? I'm not sure whether all 32 bits are really usable efficiently in V8. Also, you probably want `(Math.random() * 0x00FFFFFF) | 0` in order to make sure that the engine is really using integers to represent the number.
On 2016/05/23 13:44:16, Wladimir Palant wrote: > What about ... `(Math.random() * 0x00FFFFFF) | 0` ... Nope, still much slower than if the lookup is an array :(
On 2016/05/23 16:01:35, kzar wrote: > Nope, still much slower than if the lookup is an array :( I guess that replacing seenFilters by a Set instance gives you almost that speedup already, without even using knownFilters. If I combine the unconditionalSelectors() idea above with a Set the function becomes 5 times faster - merely 7ms instead of 35ms, quite acceptable. Only problem is that using a Set instance will be slow on Safari...
On 2016/05/23 18:29:18, Wladimir Palant wrote: > If I combine the unconditionalSelectors() idea above with a Set the > function becomes 5 times faster I can't reproduce that, maybe I'm doing it wrong but I see a much smaller improvement on Chrome 50 at least. Something like Self: 8444ms Total: 10557ms, or a 3x improvement. (I got about 2x improvement with only the unconditionalSelectors() idea.) https://github.com/kzar/adblockpluscore/commit/c7bcc5e
On 2016/05/24 08:42:33, kzar wrote: > On 2016/05/23 18:29:18, Wladimir Palant wrote: > > If I combine the unconditionalSelectors() idea above with a Set the > > function becomes 5 times faster > > I can't reproduce that, maybe I'm doing it wrong but I see a much smaller > improvement on Chrome 50 at least. Something like Self: 8444ms Total: 10557ms, > or a 3x improvement. (I got about 2x improvement with only the > unconditionalSelectors() idea.) > > https://github.com/kzar/adblockpluscore/commit/c7bcc5e It seems that using a Set isn't an option right now anyway because of Safari. So I'm not exactly sure what Wladimir is suggesting here? But it seems that we didn't even benchmark the other approaches on Safari, which we should definitely do.
So I did some new tests without using the profiler like you suggested:
let start = Date.now();
for (var i = 0; i < 1000; i += 1)
ElemHide.getSelectorsForDomain("www.extremetech.com");
Date.now() - start;
Before 235: 30900ms (1x)
235: 33787ms (0.9x)
Unconditional selectors: 14554ms (2.1x)
Unconditional selectors with filterByKey array: 5831ms (5.3x)
Unconditional selectors with seenFilters Set and random number key: 10051ms
(3.1x)
Above tests were for Chrome 50. Now for Safari 9: Before 235: 19912ms (1x) 235: 18198ms (1.1x) Unconditional selectors: 6400ms (3.1x) Unconditional selectors with filterByKey array: 9391ms (2.1x) Unconditional selectors with seenFilters Set and random number key: 13165ms (1.5x)
Patch Set 4 : Rebased, unconditionalSelectors https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#new... lib/elemHide.js:256: if (usingGetSelectorsForDomain) I'm worried about the performance of ElemHide.remove now. There is a sometimes noticeable pause when disabling EasyList. I've tried making unconditioanlSelectors a lookup, which helps a bit but that slows down getSelectorsForDomain lots as it then has to call Object.keys(unconditioanlSelectors) every time. Any ideas? https://codereview.adblockplus.org/29342830/diff/29342997/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342997/test/elemHide.js#ne... test/elemHide.js:32: let addFilter = filterText => ElemHide.add(Filter.fromText(filterText)); `this` was being clobbered for the ElemHide methods.
https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#new... lib/elemHide.js:204: // The new filter's selector is unconditionally applied to all domains This comment belongs inside the if block since it only applies to one branch of it. https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#new... lib/elemHide.js:205: if (!filter.domains && !exceptions[filter.selector]) We don't really care about the value of exceptions[filter.selector] so I'd prefer !(filter.selector in exceptions). https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#new... lib/elemHide.js:218: // The new filter's selector only applies to some domains Same here, comment inside the block please. https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#new... lib/elemHide.js:256: if (usingGetSelectorsForDomain) On 2016/05/24 14:45:22, kzar wrote: > I'm worried about the performance of ElemHide.remove now. There is a sometimes > noticeable pause when disabling EasyList. I've tried making > unconditioanlSelectors a lookup, which helps a bit but that slows down > getSelectorsForDomain lots as it then has to call > Object.keys(unconditioanlSelectors) every time. A remove isn't a very common operation. Did you measure it, how long does it take? In theory, we could make unconditionalSelectors a sorted list which would allow much faster searches. However, this will slow down insert operations considerably (meaning that it makes ElemHide.add() slower). https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#new... lib/elemHide.js:534: return unconditionalSelectors.concat(selectors); I think we should avoid concatenating two arrays here, just initialize the selectors array correctly. let selectors = specificOnly ? [] : unconditionalSelectors.slice();
https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#new... lib/elemHide.js:531: if (specificOnly) Why do you still account for specificOnly inside the loop? As far as I understand the logic, we can bail out now as soon as there is no next dot regardless of specificOnly. https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#new... lib/elemHide.js:533: else Nit: extraneous else
https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#new... lib/elemHide.js:531: if (specificOnly) On 2016/05/24 15:20:07, Sebastian Noack wrote: > Why do you still account for specificOnly inside the loop? As far as I > understand the logic, we can bail out now as soon as there is no next dot > regardless of specificOnly. Not all generic filters are in unconditionalSelectors - some have exception rules applying to them, others exclude particular domains (like ~example.com##foo).
https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#new... lib/elemHide.js:256: if (usingGetSelectorsForDomain) On 2016/05/24 15:16:19, Wladimir Palant wrote: > On 2016/05/24 14:45:22, kzar wrote: > > I'm worried about the performance of ElemHide.remove now. There is a sometimes > > noticeable pause when disabling EasyList. I've tried making > > unconditioanlSelectors a lookup, which helps a bit but that slows down > > getSelectorsForDomain lots as it then has to call > > Object.keys(unconditioanlSelectors) every time. > > A remove isn't a very common operation. Did you measure it, how long does it > take? > > In theory, we could make unconditionalSelectors a sorted list which would allow > much faster searches. However, this will slow down insert operations > considerably (meaning that it makes ElemHide.add() slower). Yea, it's pretty bad - something like 2325ms spent in elemHide.remove when EasyList is disabled. I guess you're right that this is kind of an usual operation. Also I realised the speed is really not too important, the problem is that there is a visible delay for the user. How about we wrap the subscription toggling code in adblockplusui/messageResponder.js in a window.setTimeout of 0ms? Unless the user clicks EasyList on and off repeatedly this fixes the sluggishness in the interface when removing / disabling a big subscription.
https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#new... lib/elemHide.js:256: if (usingGetSelectorsForDomain) On 2016/05/24 17:47:09, kzar wrote: > On 2016/05/24 15:16:19, Wladimir Palant wrote: > > On 2016/05/24 14:45:22, kzar wrote: > > > I'm worried about the performance of ElemHide.remove now. There is a > sometimes > > > noticeable pause when disabling EasyList. I've tried making > > > unconditioanlSelectors a lookup, which helps a bit but that slows down > > > getSelectorsForDomain lots as it then has to call > > > Object.keys(unconditioanlSelectors) every time. > > > > A remove isn't a very common operation. Did you measure it, how long does it > > take? > > > > In theory, we could make unconditionalSelectors a sorted list which would > allow > > much faster searches. However, this will slow down insert operations > > considerably (meaning that it makes ElemHide.add() slower). > > Yea, it's pretty bad - something like 2325ms spent in elemHide.remove when > EasyList is disabled. > > I guess you're right that this is kind of an usual operation. Also I realised > the speed is really not too important, the problem is that there is a visible > delay for the user. How about we wrap the subscription toggling code in > adblockplusui/messageResponder.js in a window.setTimeout of 0ms? Unless the user > clicks EasyList on and off repeatedly this fixes the sluggishness in the > interface when removing / disabling a big subscription. Another idea, maybe crazy... removing is expensive but adding is much cheaper. Perhaps in some situations we should remove everything and then add back in the filters we need?
Patch Set 5 : Speed up ElemHide.remove https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#new... lib/elemHide.js:256: if (usingGetSelectorsForDomain) On 2016/05/24 17:52:12, kzar wrote: > On 2016/05/24 17:47:09, kzar wrote: > > On 2016/05/24 15:16:19, Wladimir Palant wrote: > > > On 2016/05/24 14:45:22, kzar wrote: > > > > I'm worried about the performance of ElemHide.remove now. There is a > > sometimes > > > > noticeable pause when disabling EasyList. I've tried making > > > > unconditioanlSelectors a lookup, which helps a bit but that slows down > > > > getSelectorsForDomain lots as it then has to call > > > > Object.keys(unconditioanlSelectors) every time. > > > > > > A remove isn't a very common operation. Did you measure it, how long does it > > > take? > > > > > > In theory, we could make unconditionalSelectors a sorted list which would > > allow > > > much faster searches. However, this will slow down insert operations > > > considerably (meaning that it makes ElemHide.add() slower). > > > > Yea, it's pretty bad - something like 2325ms spent in elemHide.remove when > > EasyList is disabled. > > > > I guess you're right that this is kind of an usual operation. Also I realised > > the speed is really not too important, the problem is that there is a visible > > delay for the user. How about we wrap the subscription toggling code in > > adblockplusui/messageResponder.js in a window.setTimeout of 0ms? Unless the > user > > clicks EasyList on and off repeatedly this fixes the sluggishness in the > > interface when removing / disabling a big subscription. > > Another idea, maybe crazy... removing is expensive but adding is much cheaper. > Perhaps in some situations we should remove everything and then add back in the > filters we need? With Patch Set 5 ElemHide.remove is now down to about 330ms when disabling EasyList. There is still a pause sometimes but it's far less noticeable.
Patch Set 6 : Addressed comments https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#new... lib/elemHide.js:204: // The new filter's selector is unconditionally applied to all domains On 2016/05/24 15:16:18, Wladimir Palant wrote: > This comment belongs inside the if block since it only applies to one branch of > it. Done. https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#new... lib/elemHide.js:205: if (!filter.domains && !exceptions[filter.selector]) On 2016/05/24 15:16:19, Wladimir Palant wrote: > We don't really care about the value of exceptions[filter.selector] so I'd > prefer !(filter.selector in exceptions). Done. https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#new... lib/elemHide.js:218: // The new filter's selector only applies to some domains On 2016/05/24 15:16:18, Wladimir Palant wrote: > Same here, comment inside the block please. Done. https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#new... lib/elemHide.js:533: else On 2016/05/24 15:20:07, Sebastian Noack wrote: > Nit: extraneous else Done. https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#new... lib/elemHide.js:534: return unconditionalSelectors.concat(selectors); On 2016/05/24 15:16:19, Wladimir Palant wrote: > I think we should avoid concatenating two arrays here, just initialize the > selectors array correctly. > > let selectors = specificOnly ? [] : unconditionalSelectors.slice(); Done.
https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js#new... lib/elemHide.js:509: let selectors = specificOnly ? [] : unconditionalSelectors.slice(); You can skip the unconditionalSelectorsDirty logic in the specificOnly case: let selectors; if (specificOnly) { selectors = []; } else { if (unconditionalSelectorsDirty) { unconditionalSelectors = Object.keys(filtersBySelector); unconditionalSelectorsDirty = false; } selectors = unconditionalSelectors.slice(); } https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js#new... lib/elemHide.js:509: let selectors = specificOnly ? [] : unconditionalSelectors.slice(); Also note that if you'd call Object.keys() unconditionally, you can avoid the isDirty logic and don't have to copy the list. But that would still be notably slower? Just want to make sure, since the way you did it initally, the array was copied anyway by Array.concat().
https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js#new... lib/elemHide.js:509: let selectors = specificOnly ? [] : unconditionalSelectors.slice(); One more thing, if we need that is dirty logic, how about just setting unconditionalSelectors to null instead adding yet another global variable? That way the code is a little shorter, and we won't leak outdated data.
Patch Set 7 : Avoid extra dirty variable https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js#new... lib/elemHide.js:509: let selectors = specificOnly ? [] : unconditionalSelectors.slice(); On 2016/05/25 07:40:26, Sebastian Noack wrote: > One more thing, if we need that is dirty logic, how about just setting > unconditionalSelectors to null instead adding yet another global variable? That > way the code is a little shorter, and we won't leak outdated data. Done. https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js#new... lib/elemHide.js:509: let selectors = specificOnly ? [] : unconditionalSelectors.slice(); On 2016/05/25 07:30:49, Sebastian Noack wrote: > You can skip the unconditionalSelectorsDirty logic in the specificOnly case: > > let selectors; > if (specificOnly) > { > selectors = []; > } > else > { > if (unconditionalSelectorsDirty) > { > unconditionalSelectors = Object.keys(filtersBySelector); > unconditionalSelectorsDirty = false; > } > selectors = unconditionalSelectors.slice(); > } I know I could skip restoring unconditionalSelectors if specificOnly is true but there's not much point. https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js#new... lib/elemHide.js:509: let selectors = specificOnly ? [] : unconditionalSelectors.slice(); On 2016/05/25 07:30:49, Sebastian Noack wrote: > Also note that if you'd call Object.keys() unconditionally, you can avoid the > isDirty logic and don't have to copy the list. But that would still be notably > slower? Just want to make sure, since the way you did it initally, the array was > copied anyway by Array.concat(). Yep, just double checked and it's still quite a bit slower using Object.keys() every time even without the concat.
https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js#new... lib/elemHide.js:509: let selectors = specificOnly ? [] : unconditionalSelectors.slice(); On 2016/05/25 08:05:19, kzar wrote: > On 2016/05/25 07:30:49, Sebastian Noack wrote: > > You can skip the unconditionalSelectorsDirty logic in the specificOnly case: > > > > let selectors; > > if (specificOnly) > > { > > selectors = []; > > } > > else > > { > > if (unconditionalSelectorsDirty) > > { > > unconditionalSelectors = Object.keys(filtersBySelector); > > unconditionalSelectorsDirty = false; > > } > > selectors = unconditionalSelectors.slice(); > > } > > I know I could skip restoring unconditionalSelectors if specificOnly is true but > there's not much point. Besides a speedup in specificOnly case which might be negligible, it also improves the code locality and makes the purpose of that logic slightly easier to understand IMO. Also it will be even shorter now where you only have one variable.
https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js#new... lib/elemHide.js:509: let selectors = specificOnly ? [] : unconditionalSelectors.slice(); On 2016/05/25 08:11:56, Sebastian Noack wrote: > On 2016/05/25 08:05:19, kzar wrote: > > On 2016/05/25 07:30:49, Sebastian Noack wrote: > > > You can skip the unconditionalSelectorsDirty logic in the specificOnly case: > > > > > > let selectors; > > > if (specificOnly) > > > { > > > selectors = []; > > > } > > > else > > > { > > > if (unconditionalSelectorsDirty) > > > { > > > unconditionalSelectors = Object.keys(filtersBySelector); > > > unconditionalSelectorsDirty = false; > > > } > > > selectors = unconditionalSelectors.slice(); > > > } > > > > I know I could skip restoring unconditionalSelectors if specificOnly is true > but > > there's not much point. > > Besides a speedup in specificOnly case which might be negligible, it also > improves the code locality and makes the purpose of that logic slightly easier > to understand IMO. Also it will be even shorter now where you only have one > variable. I disagree on this one, but I see your point. I'll leave it up to Wladimir to decide.
I really like the current version, only a few nits below. https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js#new... lib/elemHide.js:509: let selectors = specificOnly ? [] : unconditionalSelectors.slice(); On 2016/05/25 08:29:04, kzar wrote: > I disagree on this one, but I see your point. I'll leave it up to Wladimir to > decide. Frankly, while I can see the arguments towards only setting unconditionalSelectors when needed, it's not really important enough to be changed. The flow won't necessarily become more readable if we do, and it's already fine as is. https://codereview.adblockplus.org/29342830/diff/29343029/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29343029/lib/elemHide.js#new... lib/elemHide.js:65: * the array needs to be regenerated. Maybe change the description to better indicate what function this array has now, something like: This array caches the keys of filtersBySelector table (selectors which unconditionally apply on all domains). It will be null if the cache needs to be rebuilt. https://codereview.adblockplus.org/29342830/diff/29343029/lib/elemHide.js#new... lib/elemHide.js:67: var unconditionalSelectors = []; Nit: Please initialize with null here and in clear(), this is now purely a cache which is built on demand. While technically initializing with an empty array is correct, in 99.9999% of the cases we will immediately throw away that array again.
Patch Set 8 : Addressed nits https://codereview.adblockplus.org/29342830/diff/29343029/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29343029/lib/elemHide.js#new... lib/elemHide.js:65: * the array needs to be regenerated. On 2016/05/25 10:43:03, Wladimir Palant wrote: > Maybe change the description to better indicate what function this array has > now, something like: > > This array caches the keys of filtersBySelector table (selectors which > unconditionally apply on all domains). It will be null if the cache needs to be > rebuilt. Done. https://codereview.adblockplus.org/29342830/diff/29343029/lib/elemHide.js#new... lib/elemHide.js:67: var unconditionalSelectors = []; On 2016/05/25 10:43:03, Wladimir Palant wrote: > Nit: Please initialize with null here and in clear(), this is now purely a cache > which is built on demand. While technically initializing with an empty array is > correct, in 99.9999% of the cases we will immediately throw away that array > again. Done.
LGTM
LGTM 
|
