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)
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.
Attachments (0)
Change History (20)
comment:2 Changed on 08/02/2018 at 09:20:43 PM by jsonesen
comment:3 Changed on 08/03/2018 at 09:26:52 AM by mjethani
- Cc jsonesen added
comment:4 follow-up: ↓ 5 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.
Hey, I am open to start working on this :)