Issue 29342837: Issue 4054 - Remove filters from filtersByDomain (Closed)
DescriptionIssue 4054 - Remove filters from filtersByDomain
Patch Set 1 #
Total comments: 12
Patch Set 2 : Corrected mistake in code ordering #
Total comments: 2
Patch Set 3 : Use for ... in #
Total comments: 2
Patch Set 4 : Rename domainMatches for consistency #MessagesTotal messages: 13
Patch Set 1 https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js#new... lib/elemHide.js:213: delete filters[filter.text]; Note: There is a memory leak here. I don't delete filtersByDomain[domain] Object even if the last filter just got removed for it. I couldn't figure out a way to do that without performance suffering drastically. (As in Chrome thinking the extension had crashed when disabling EasyList!) Open to ideas, but if no one has any and the memory leak is a deal breaker I guess we need to revert the earlier changes :/
LGTM https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js#new... lib/elemHide.js:213: delete filters[filter.text]; On 2016/05/20 11:36:12, kzar wrote: > Note: There is a memory leak here. I don't delete filtersByDomain[domain] Object > even if the last filter just got removed for it. I couldn't figure out a way to > do that without performance suffering drastically. I don't think that this is a big deal. All filters being removed for a domain is a rather unusual case - usually this only happens temporarily if at all.
https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js#new... lib/elemHide.js:208: let domains = Object.keys(filter.domains || defaultDomains); So in the case here for..in would be actually slower? https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js#new... lib/elemHide.js:213: delete filters[filter.text]; On 2016/05/20 11:36:12, kzar wrote: > Note: There is a memory leak here. I don't delete filtersByDomain[domain] Object > even if the last filter just got removed for it. I couldn't figure out a way to > do that without performance suffering drastically. (As in Chrome thinking the > extension had crashed when disabling EasyList!) > > Open to ideas, but if no one has any and the memory leak is a deal breaker I > guess we need to revert the earlier changes :/ How did you try to delete filtersByDomain[domain]? The simplest way I can think of would be like that: if (filters) { delete filters[filter.text]; if (Object.keys(filters).length == 0) delete filtersByDomain[domain]; } Perhaps, it gives some speed up to bail if there filter isn't in there: if (filters && filter.text in filters) { delete filters[filter.text]; if (Object.keys(filters).length == 0) delete filtersByDomain[domain]; } Another thing you could try would be using Object.getOwnPropertyNames() instead Object.keys(). If it's still too slow, I guess I'd be fine with the memory leak, after all we leak removed subscriptions anyway, IIRC.
https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js#new... lib/elemHide.js:213: delete filters[filter.text]; What if an element hiding filter is given in two subscriptions?
https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js#new... lib/elemHide.js:213: delete filters[filter.text]; On 2016/05/20 12:29:24, Sebastian Noack wrote: > What if an element hiding filter is given in two subscriptions? That case is being handled elsewhere (FilterListener). If remove() is called here, then the filter actually needs to be removed. Note also the `if (!(filter.text in keyByFilter))` check about. However, I just noticed that this block is misplaced - it should be inside the `else` block above (meaning: should not apply to exceptions). So NOT LGTM yet after all. https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js#new... lib/elemHide.js:213: delete filters[filter.text]; On 2016/05/20 12:20:44, Sebastian Noack wrote: > How did you try to delete filtersByDomain[domain]? Frankly, I don't care. No matter how you do it, it's not worth doing (looking at both performance and code complexity).
Patch Set 2 : Corrected mistake in code ordering https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js#new... lib/elemHide.js:208: let domains = Object.keys(filter.domains || defaultDomains); On 2016/05/20 12:20:44, Sebastian Noack wrote: > So in the case here for..in would be actually slower? Well I didn't think about it either way to be honest, was just trying to get a fix together quickly. https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js#new... lib/elemHide.js:213: delete filters[filter.text]; On 2016/05/20 12:20:44, Sebastian Noack wrote: > On 2016/05/20 11:36:12, kzar wrote: > > Note: There is a memory leak here. I don't delete filtersByDomain[domain] > Object > > even if the last filter just got removed for it. I couldn't figure out a way > to > > do that without performance suffering drastically. (As in Chrome thinking the > > extension had crashed when disabling EasyList!) > > > > Open to ideas, but if no one has any and the memory leak is a deal breaker I > > guess we need to revert the earlier changes :/ > > How did you try to delete filtersByDomain[domain]? The simplest way I can think > of would be like that: > > if (filters) > { > delete filters[filter.text]; > if (Object.keys(filters).length == 0) > delete filtersByDomain[domain]; > } > > Perhaps, it gives some speed up to bail if there filter isn't in there: > > if (filters && filter.text in filters) > { > delete filters[filter.text]; > if (Object.keys(filters).length == 0) > delete filtersByDomain[domain]; > } > > Another thing you could try would be using Object.getOwnPropertyNames() instead > Object.keys(). If it's still too slow, I guess I'd be fine with the memory leak, > after all we leak removed subscriptions anyway, IIRC. Well I tried those ideas already except using .getOwnPropertyNames() but I don't think that would make much difference. I also tried something like this but it was still hugely (extension crashes) slow: if (Object.keys(filters).length == 1) delete filtersByDomain[domain]; else delete filters[filter.text]; https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js#new... lib/elemHide.js:213: delete filters[filter.text]; On 2016/05/20 12:29:24, Sebastian Noack wrote: > What if an element hiding filter is given in two subscriptions? I didn't consider that. I'm not sure how we can handle that to be honest. https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js#new... lib/elemHide.js:213: delete filters[filter.text]; On 2016/05/20 12:40:05, Wladimir Palant wrote: > On 2016/05/20 12:29:24, Sebastian Noack wrote: > > What if an element hiding filter is given in two subscriptions? > > That case is being handled elsewhere (FilterListener). If remove() is called > here, then the filter actually needs to be removed. Note also the `if > (!(filter.text in keyByFilter))` check about. > > However, I just noticed that this block is misplaced - it should be inside the > `else` block above (meaning: should not apply to exceptions). So NOT LGTM yet > after all. Whoops sorry, I moved this at the last minute and apparently messed it up... Done.
https://codereview.adblockplus.org/29342837/diff/29342840/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342840/lib/elemHide.js#new... lib/elemHide.js:208: for (let domain of domains) So how about making this code consistent to the one in add()? let domainMatches = filter.domains || defaultDomains; for (let domain in domainMatches)
Patch Set 3 : Use for ... in https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js#new... lib/elemHide.js:208: let domains = Object.keys(filter.domains || defaultDomains); On 2016/05/20 12:49:29, kzar wrote: > On 2016/05/20 12:20:44, Sebastian Noack wrote: > > So in the case here for..in would be actually slower? > > Well I didn't think about it either way to be honest, was just trying to get a > fix together quickly. As we discussed in IRC I did a quick test of this in Chrome 50. Seems a bit faster using for ... in, doesn't cause a deopt. Done.
https://codereview.adblockplus.org/29342837/diff/29342840/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342840/lib/elemHide.js#new... lib/elemHide.js:208: for (let domain of domains) On 2016/05/20 13:12:34, Wladimir Palant wrote: > So how about making this code consistent to the one in add()? > > let domainMatches = filter.domains || defaultDomains; > for (let domain in domainMatches) Done.
LGTM
https://codereview.adblockplus.org/29342837/diff/29342842/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342842/lib/elemHide.js#new... lib/elemHide.js:207: let domains = filter.domains || defaultDomains; The same variable is named domainMatches in .add() - you should either change it here or there, for consistency. Personally, I'd rather go with `domains` in both places.
Patch Set 4 : Rename domainMatches for consistency https://codereview.adblockplus.org/29342837/diff/29342842/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342842/lib/elemHide.js#new... lib/elemHide.js:207: let domains = filter.domains || defaultDomains; On 2016/05/20 13:26:06, Wladimir Palant wrote: > The same variable is named domainMatches in .add() - you should either change it > here or there, for consistency. Personally, I'd rather go with `domains` in both > places. Done.
LGTM 
|
