Opened 10 months ago

Last modified 4 months ago

#6829 reviewing change

Remove filters that don't belong to any subscription

Reported by: mjethani Assignee: jsonesen
Priority: P3 Milestone:
Module: Core Keywords:
Cc: kzar, hfiguiere, sergz, jsonesen Blocked By: #6855
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29860589/

Description (last modified by mjethani)

Background

When you subscribe to a filter list and then unsubscribe from the filter list, the filters from that subscription still remain in memory in the Filter.knownFilters object. This is not ideal. We already have issues with memory usage. When a subscription is removed, any filters that belong to that subscription that don't also belong to any other subscription should be removed.

What to change

To be determined: Tentatively, call Filter.knownFilters.delete in FilterStorage.removeSubscription, but let's wait for #6855 because this would be a very complex and risky change.

Change History (19)

comment:1 Changed 10 months ago by mjethani

  • Description modified (diff)
  • Ready set

comment:2 Changed 10 months ago by jsonesen

Hey, I am open to start working on this :)

comment:3 Changed 10 months ago by mjethani

  • Cc jsonesen added

comment:4 follow-up: Changed 10 months ago by mjethani

That is awesome, Jon!

You could start by implementing the changes I have described in the issue description. These may not be the only changes required. For example, I see that Filter.knownFilters is accessed in lib/filterStorage.js. Maybe we need to call the free method over there? I don't know. You can dig into it and find out. Once you have a patch, assign it to me for review.

comment:5 in reply to: ↑ 4 Changed 10 months ago by mjethani

Replying to mjethani:

Once you have a patch, assign it to me for review.

Also be sure to add Dave in CC.

comment:6 Changed 10 months ago by kzar

Perhaps a dumb question, but if two subscriptions have the same filter wouldn't removing the filter from the knownFilters lookup be suboptimal?

comment:7 Changed 10 months ago by mjethani

The code in the removeFilter function in lib/filterListener.js already checks if the filter has any enabled subscriptions, only after that it removes it from matcher, ElemHide, ElemHideEmulation, or Snippets. It is here that we can insert our call to free up the filter. But good thing you raised this question, we should double check to be sure that we're handling it correctly.

comment:8 Changed 10 months ago by kzar

Cool, OK.

comment:9 Changed 10 months ago by jsonesen

Thanks for the direction :)

I will check to make sure things are handled right as well!

comment:10 Changed 10 months ago by jsonesen

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:11 Changed 9 months ago by mjethani

  • Description modified (diff)

comment:12 Changed 9 months ago by mjethani

  • Description modified (diff)

I'm not sure that the removeFilter function in lib/filterListener.jsis the right place to call the free method so let's just put that part on hold for now.

More likely this will have to be done in lib/filterStorage.js.

comment:13 Changed 9 months ago by mjethani

  • Blocked By 6855 added

comment:14 Changed 9 months ago by mjethani

  • Description modified (diff)
  • Priority changed from P2 to P3
  • Ready unset

comment:15 Changed 9 months ago by mjethani

@jsonesen sorry I was way out of my depth when I got into this issue. It seems this is quite complex. Let's do this step by step. Once we've lost the reference to the Subscription object (#6855), Filter.knownFilters should then have the last remaining reference to a Filter object that has no other subscriptions.

Let's wait until #6855 is sorted out.

comment:16 Changed 9 months ago by jsonesen

  • Owner set to jsonesen

comment:17 Changed 9 months ago by jsonesen

  • Review URL(s) modified (diff)

comment:18 Changed 9 months ago by jsonesen

  • Review URL(s) modified (diff)

comment:19 Changed 4 months ago by jsonesen

Looks like we may be able to start looking into this again?

Note: See TracTickets for help on using tickets.