Opened 4 years ago

Closed 3 months ago

#3468 closed change (rejected)

Clean up caches when removing filters

Reported by: greiner Assignee:
Priority: Unknown Milestone:
Module: Core Keywords: closed-in-favor-of-gitlab
Cc: kzar, hfiguiere, tlucas, sergz, erikvold Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by kzar)

Background

We use various caches to optimize operations such as Filter.fromText() but we don't free some of those when filters get removed. Therefore we should ensure that we clean up in such situations without sacrificing optimizations.

Heap snapshot comparisons have shown that cleaning up ElemHide.exceptions, Filter.knownFilters and Subscription.knownSubscriptions (due to their respective references to Filter objects) eliminates all strings, arrays, Filter and Subscription objects that were created when adding new filters or subscriptions.

Note that the approach suggested below does not clean up after filters that don't belong to a subscription. Those ones would still be leaking memory.

What to change

  • Remove all filters from Filter.knownFilters that don't belong to a subscription whenever filters are removed. For that, add a Filter.forget() method and call it in filterListener.js
  • Remove all selectors from ElemHide.exceptions that don't have any filters belonging to them anymore. For that, add this logic to ElemHide.remove().
  • Remove all subscriptions from ElemHide.knownSubscriptions that are not contained in FilterStorage.knownSubscriptions. For that, add a Subscription.forget() method and call it in filterListener.js
  • Add unit tests to test this functionality:
    • Remove all filter lists (including Acceptable Ads), custom filters and whitelisted domains
    • Add the custom filter foo to ensure that the extension doesn't reset when reloading it
    • Reload extension
    • Observe JavaScript heap after garbage collection
  • Testing subscriptions
    • Add "EasyList Germany+EasyList" filter list
    • Remove the filter list again
    • Observe JavaScript heap after garbage collection
  • Testing custom filters
    • Execute the following in the context of the background page: for (var i = 0; i < 10000; i++) FilterStorage.addFilter(Filter.fromText("foo" + i));
    • Execute the following in the context of the background page: for (var i = 0; i < 10000; i++) FilterStorage.removeFilter(Filter.fromText("foo" + i));
    • Observe JavaScript heap after garbage collection

Change History (7)

comment:1 Changed 2 years ago by fhd

  • Cc trev removed

comment:2 Changed 23 months ago by sergz

  • Cc kzar hfiguiere tlucas sergz added

comment:3 follow-up: Changed 23 months ago by kzar

Will this suggestion still be relevant after we switch to emscripten sergz?

comment:4 in reply to: ↑ 3 Changed 23 months ago by sergz

Replying to kzar:

Will this suggestion still be relevant after we switch to emscripten sergz?

Currently in emscripten Filter and Subscription objects are supposed to be removed from such caching structures when they are being destroyed, however IMO it still good to have tests checking that there are at least no bugs (memory leaks). In addition such tests can be useful because I would not exclude the chance that the implementation will be changed there.

comment:5 Changed 23 months ago by kzar

  • Description modified (diff)

...however IMO it still good to have tests checking that there are at least no bugs (memory leaks). In addition such tests can be useful because I would not exclude the chance that the implementation will be changed there.

Alright I've modified the issue description to mention that these tests should be written.

comment:6 Changed 12 months ago by erikvold

  • Cc erikvold added

comment:7 Changed 3 months ago by sebastian

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

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

Note: See TracTickets for help on using tickets.