Opened 3 months ago

Last modified 3 months ago

#7452 reviewing change

Stop keeping element hiding filter objects in memory

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

https://codereview.adblockplus.org/29935564/

Description (last modified by mjethani)

Background

After #7094 and #7029, the only remaining references to Filter instances are in the "containers" (matcher, element hiding module, etc.). Of these, element hiding filters are the easiest to work with without keeping references to the Filter instances themselves (see patch). We could lose the references to the Filter instances in the element hiding implementation module and then make the objects ephemeral (see idea in #7097).

For this to work however we need to find a solution for the state properties disabled, hitCount, and lastHit (see discussion in #7095). We cannot remove these properties, but it is also true that in most cases these properties are never set (in fact, in the WebExt version of Adblock Plus they are not used at all). It may be possible to implement something where a Filter instance is cached only if one of these properties is set.

What to change

At a high level:

  • In lib/elemHide.js, keep filters by text rather than by Filter instances
  • In lib/filterClasses.js, in the Filter.fromText() function, do not cache the return value in Filter.knownFilters if the object has certain properties; in this case, do not cache it if it's an instance of ElemHideFilter
  • TBD: Find a way to preserve the values of the state properties disabled, hitCount, and lastHit if any of the values are set to anything other than the default value of the property (in which case the state is also serialized) (see #7453)

Change History (6)

comment:1 Changed 3 months ago by mjethani

  • Blocking 7097 added; 7000 removed

comment:2 Changed 3 months ago by mjethani

  • Description modified (diff)

comment:3 Changed 3 months ago by mjethani

  • Owner set to mjethani

comment:4 Changed 3 months ago by mjethani

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

comment:5 Changed 3 months ago by mjethani

  • Blocked By 7453 added

comment:6 Changed 3 months ago by mjethani

  • Description modified (diff)
Note: See TracTickets for help on using tickets.