Opened on 08/02/2018 at 01:20:36 PM

Closed on 08/29/2019 at 05:43:52 PM

#6829 closed change (rejected)

Remove filters that don't belong to any subscription

Reported by: mjethani Assignee: jsonesen
Priority: P3 Milestone:
Module: Core Keywords: closed-in-favor-of-gitlab
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):

Description (last modified by mjethani)


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.

Attachments (0)

Change History (20)

comment:1 Changed on 08/02/2018 at 01:24:38 PM by mjethani

  • Description modified (diff)
  • Ready set

comment:2 Changed on 08/02/2018 at 09:20:43 PM by jsonesen

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

comment:3 Changed on 08/03/2018 at 09:26:52 AM by mjethani

  • Cc jsonesen added

comment:4 follow-up: Changed on 08/03/2018 at 09:29:22 AM 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 on 08/03/2018 at 09:30:57 AM 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 on 08/03/2018 at 03:29:46 PM 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 on 08/03/2018 at 03:33:36 PM 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 on 08/03/2018 at 03:36:35 PM by kzar

Cool, OK.

comment:9 Changed on 08/06/2018 at 10:21:24 PM by jsonesen

Thanks for the direction :)

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

comment:10 Changed on 08/08/2018 at 10:10:47 PM by jsonesen

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

comment:11 Changed on 08/10/2018 at 12:15:12 PM by mjethani

  • Description modified (diff)

comment:12 Changed on 08/10/2018 at 01:07:06 PM 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 on 08/11/2018 at 03:02:51 PM by mjethani

  • Blocked By 6855 added

comment:14 Changed on 08/12/2018 at 07:54:53 AM by mjethani

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

comment:15 Changed on 08/12/2018 at 08:00:40 AM 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 on 08/21/2018 at 07:26:28 PM by jsonesen

  • Owner set to jsonesen

comment:17 Changed on 08/21/2018 at 09:53:20 PM by jsonesen

  • Review URL(s) modified (diff)

comment:18 Changed on 08/21/2018 at 09:58:59 PM by jsonesen

  • Review URL(s) modified (diff)

comment:19 Changed on 01/29/2019 at 03:54:40 PM by jsonesen

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

comment:20 Changed on 08/29/2019 at 05:43:52 PM by sebastian

  • Keywords closed-in-favor-of-gitlab added
  • Resolution set to rejected
  • Status changed from reviewing to closed

Sorry, but we switched to GitLab. If this issue is still relevant, please file it again in the new issue tracker.

Add Comment

Modify Ticket

Change Properties
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from jsonesen.
Note: See TracTickets for help on using tickets.