Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(30)

Issue 29850583: Issue 6829 - Remove unused filters from knownFilters

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 days, 13 hours ago by Jon Sonesen
Modified:
2 days, 19 hours ago
Reviewers:
Manish Jethani
CC:
kzar
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

Issue 6829 - Remove unused filters from knownFilters

Patch Set 1 #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -0 lines) Patch
M lib/filterClasses.js View 1 chunk +4 lines, -0 lines 4 comments Download
M lib/filterListener.js View 1 chunk +1 line, -0 lines 1 comment Download

Messages

Total messages: 9
Jon Sonesen
5 days, 13 hours ago (2018-08-08 21:53:30 UTC) #1
Jon Sonesen
Here is the changes I have came up with, it seems to work if I ...
5 days, 13 hours ago (2018-08-08 21:55:06 UTC) #2
Jon Sonesen
https://codereview.adblockplus.org/29850583/diff/29850584/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29850583/diff/29850584/lib/filterClasses.js#newcode86 lib/filterClasses.js:86: Filter.knownFilters.delete(this.text); This is something that I am not sure ...
5 days, 11 hours ago (2018-08-08 23:22:06 UTC) #3
Manish Jethani
https://codereview.adblockplus.org/29850583/diff/29850584/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29850583/diff/29850584/lib/filterClasses.js#newcode86 lib/filterClasses.js:86: Filter.knownFilters.delete(this.text); On 2018/08/08 23:22:06, Jon Sonesen wrote: > This ...
5 days, 1 hour ago (2018-08-09 09:51:05 UTC) #4
Manish Jethani
Including Dave in CC
5 days, 1 hour ago (2018-08-09 09:52:21 UTC) #5
Jon Sonesen
Thanks for looking at this! https://codereview.adblockplus.org/29850583/diff/29850584/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29850583/diff/29850584/lib/filterClasses.js#newcode86 lib/filterClasses.js:86: Filter.knownFilters.delete(this.text); On 2018/08/09 09:51:05, ...
4 days, 15 hours ago (2018-08-09 20:11:42 UTC) #6
Manish Jethani
I have updated #6829 with new information on what to change: https://issues.adblockplus.org/ticket/6829 We should make ...
3 days, 22 hours ago (2018-08-10 12:23:40 UTC) #7
Manish Jethani
On 2018/08/10 12:23:40, Manish Jethani wrote: > I have updated #6829 with new information on ...
3 days, 22 hours ago (2018-08-10 13:15:22 UTC) #8
Manish Jethani
2 days, 19 hours ago (2018-08-11 16:11:40 UTC) #9
OK, let's stick with Patch Set 1 but see my comments inline ...

https://codereview.adblockplus.org/29850583/diff/29850584/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/29850583/diff/29850584/lib/filterListener....
lib/filterListener.js:215: filter.free();
Let's move this call to lib/filterStorage.js.

In the removeSubscription method, right after we call removeSubscriptionFilters,
we should have a loop like this:

  for (let filter of subscription.filters)
  {
    if (filter.subscriptions.length == 0)
      filter.free();
  }

In combination with patch #29853574, this should free up the Filter object
finally since there are no other references to it any more. You can verify this
by taking a heap snapshot in DevTools and looking for the filter (maybe try with
a minimal filter list with only ten filters).

Note that in actual filter lists a Filter object can have multiple references to
the same Subscription object, if the same filter occurs multiple times (see
patch #29852555), and removeSubscriptionFilters only removes the first
reference. Because of this bug you won't see all unused Filter objects removed
from memory, but most should be removed. If you apply both patches #29853574 and
#29852555 then you should see all Filter objects from the removed subscription
gone from the heap snapshot.

The reason we shouldn't free the Filter object when a subscription is disabled:
(1) the Subscription object is still in memory and holds onto its Filter objects
anyway; (2) since the Filter objects are going to linger in memory anyway, we
might as well keep them in Filter.knownFilters so we avoid any possible
duplicates from other filter lists.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5