Issue 29860589: Issue 6829 - Free filters when subscriptions are empty (Closed)
|
Created:
Aug. 21, 2018, 9:44 p.m. by Jon Sonesen Modified:
Jan. 29, 2019, 3:55 p.m. CC:
kzar Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 6829 - Free filters when subscriptions are empty
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address PS1 comment #
Total comments: 1
MessagesTotal messages: 11
Once I saw this method removeSubscriptionFilters which is iterating over all the filters in a given subscription I figured this would be a good place to check if there are any subscriptions remaining in filter.subscriptions remaining and if not just free the filter from knownFilters. Not sure about my commit message though https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.j... lib/filterStorage.js:684: Filter.knownFilters.delete(filter.text); I found that there is a pretty good reduction in the memory footprint of the extension. With this change; after removing all subscriptions from the options page it would return to around 60mb with nothing but the options page open in the browser. Whereas if you test without this, the footprint never really seems to return to as small of a footprint as when first starting the extension with no subscriptions.
Similar to what we did here: https://github.com/adblockplus/adblockpluscore/commit/5aca7b994bd142bf44da691... I think we should add some tests for this. https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.j... lib/filterStorage.js:148: We should delete the filters here. https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.j... lib/filterStorage.js:201: addSubscriptionFilters(subscription); After calling addSubscriptionFilters, we should delete any filters in the oldFilters list that don't have any subscriptions (but most of the old filters will also be in the new filters list, so they will have a subscription). https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.j... lib/filterStorage.js:683: if (filter.subscriptions.size == 0) removeSubscriptionFilters is also called from updateSubscriptionFilters, which means this will also remove any filters that are also in the new list of filters (passed to updateSubscriptionFilters). I think we should do this outside of this function, where it is being called instead.
https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.j... lib/filterStorage.js:683: if (filter.subscriptions.size == 0) On 2018/08/25 15:03:38, Manish Jethani wrote: > removeSubscriptionFilters is also called from updateSubscriptionFilters, which > means this will also remove any filters that are also in the new list of filters > (passed to updateSubscriptionFilters). I think we should do this outside of this > function, where it is being cahe delete should only occur for filters which have no subscriptions. The deletion should I ly occur if there are no subs riptions in the subs set, rather than if there are disabled subscriptions so I would think it shouldn't effect the update and in fact should only occur when the filter has no subscriptions (hence the logical if) also the filters are already being iterated over here so we can avoid looking at all the filters again.
https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.j... lib/filterStorage.js:683: if (filter.subscriptions.size == 0) On 2018/08/25 15:18:46, jsonesen wrote: > On 2018/08/25 15:03:38, Manish Jethani wrote: > > removeSubscriptionFilters is also called from updateSubscriptionFilters, which > > means this will also remove any filters that are also in the new list of > filters > > (passed to updateSubscriptionFilters). I think we should do this outside of > this > > function, where it is being cahe delete should only occur for filters which > have no subscriptions. > > The deletion should I ly occur if there are no subs riptions in the subs set, > rather than if there are disabled subscriptions so I would think it shouldn't > effect the update and in fact should only occur when the filter has no > subscriptions (hence the logical if) also the filters are already being iterated > over here so we can avoid looking at all the filters again. Oh shoot nevermind, since it removes first then adds the chance of having a filter with 0 subscriptions that is meant to be added to the update subscription is basically 100%?
https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.j... lib/filterStorage.js:683: if (filter.subscriptions.size == 0) On 2018/08/25 15:32:08, jsonesen wrote: > On 2018/08/25 15:18:46, jsonesen wrote: > > On 2018/08/25 15:03:38, Manish Jethani wrote: > > > removeSubscriptionFilters is also called from updateSubscriptionFilters, > which > > > means this will also remove any filters that are also in the new list of > > filters > > > (passed to updateSubscriptionFilters). I think we should do this outside of > > this > > > function, where it is being cahe delete should only occur for filters which > > have no subscriptions. > > > > The deletion should I ly occur if there are no subs riptions in the subs set, > > rather than if there are disabled subscriptions so I would think it shouldn't > > effect the update and in fact should only occur when the filter has no > > subscriptions (hence the logical if) also the filters are already being > iterated > > over here so we can avoid looking at all the filters again. > > > Oh shoot nevermind, since it removes first then adds the chance of having a > filter with 0 subscriptions that is meant to be added to the update subscription > is basically 100%? Yes, exactly. If a filter is also in a different subscription then the count will be greater than 0, but if it's only in this subscription then it'll be 0.
https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.j... lib/filterStorage.js:148: On 2018/08/25 15:03:38, Manish Jethani wrote: > We should delete the filters here. Hm, because of the update function calling removSubscriptionFilters I know I need to change it, but do you think it is possible to just change where the update method calls remove? This way it only happens after the subscriptions finish updating to avoid the delete/readd case? https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.j... lib/filterStorage.js:201: addSubscriptionFilters(subscription); On 2018/08/25 15:03:38, Manish Jethani wrote: > After calling addSubscriptionFilters, we should delete any filters in the > oldFilters list that don't have any subscriptions (but most of the old filters > will also be in the new filters list, so they will have a subscription). See comment above
https://codereview.adblockplus.org/29860589/diff/29869582/test/filterStorage.js File test/filterStorage.js (right): https://codereview.adblockplus.org/29860589/diff/29869582/test/filterStorage.... test/filterStorage.js:498: test.equal(Filter.knownFilters[filter1.text], undefined); This test is currently a noop (removing changes does not effect outcome) Not sure exactly how to test this change here but I realize it should be done.
I'm sorry to keep this one waiting. I think this one is really tricky and we could really screw it up, let's get back to this after the 3.4 release.
Word, thanks for the follow up I figured we would shelve this for a bit. I agree, this is very sensitive code and there is other work being performed on this area as well.
Message was sent while issue was closed.
Closing. This ticket was postponed and by now I would like to use gitlab 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
