Opened 7 months ago

Closed 7 months ago

Last modified 3 months ago

#7046 closed change (fixed)

Defer caching of domain maps for element hiding filters and exceptions

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

https://codereview.adblockplus.org/29909555/

Description (last modified by mjethani)

Background

The ActiveFilter class caches the generated Map object for the domains property on first access. For element hiding filters, this property is accessed only twice on initial load (and we can reduce this to once because it's in the same function), and never thereafter, since in lib/elemHide.js the filters are already indexed by domain. For element hiding exceptions, this property is accessed once on initial load, and then once each time there is a filter that the exception might apply to, which in practice means that for most domain-specific exceptions it is never accessed again unless the user visits a page that activates the corresponding filter.

The values of the domains property for element hiding filters and exceptions together take up ~2.5 MB on the heap. This includes the Map objects and the corresponding entries in the knownDomainMaps object. We could easily save this memory by caching the generated Map objects not on first but on second access and avoid accessing the property a second time in most cases.

What to change

In lib/filterClasses.js, cache the value of the domains property in the getter, for instances of ElemHideFilter and ElemHideException, not on first but on second access, by maintaining a boolean flag or a counter that is updated on first access.

In lib/elemHide.js, avoid accessing the domains property more than once.

Hints for testers

This change is an implementation detail and is already covered by unit tests.

Change History (7)

comment:1 Changed 7 months ago by mjethani

  • Cc kzar hfiguiere added
  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:2 Changed 7 months ago by mjethani

  • Blocking 7000 added

comment:3 Changed 7 months ago by abpbot

A commit referencing this issue has landed:
Issue 7046 - Defer caching of domain maps

comment:4 Changed 7 months ago by mjethani

  • Description modified (diff)

comment:5 Changed 7 months ago by mjethani

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:6 Changed 3 months ago by abpbot

A commit referencing this issue has landed:
Issue 7046 - Defer caching of domain maps

comment:7 Changed 3 months ago by ukacar

  • Verified working set
Note: See TracTickets for help on using tickets.