Opened 13 months ago

Closed 13 months ago

Last modified 13 months ago

#6955 closed change (rejected)

Avoid making copies of common selector list

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

https://codereview.adblockplus.org/29882558/

Description (last modified by mjethani)

Background

Most domains have no domain-specific selectors (#6652), yet the getSelectorsForDomain function in lib/elemHide.js always makes a new copy of the selector list, even if it's the same list of ~18,000 generic selectors. What's worse is that the extension then generates an entirely new string for the style sheet and then holds on to it until the tab is closed. This is too wasteful.

The reason for creating a new array is that we don't want the caller to modify the original cache of selectors maintained internally, but this can easily be done now by freezing the array object instead.

The real reason for creating a new array is that not all generic filters and exceptions apply on all domains. We have the list of "unconditional" selectors, but we must compute the conditional selectors out of the generic filters for any given domain. #6652 would take care of this in a better way, but the solution over there is complex and has been in review for more than half a year. An alternate solution, just for reducing the memory consumption (at the cost of a slight performance hit) would be to keep track of whether there are any domain-specific filters or exceptions for the given domain, or whether any filters are excluded specifically from the domain, and return a cached list of common selectors if this is not the case.

Resolution

The issue here got addressed as part of #6957, which instead of caching the list of common selectors instead caches the common style sheet, which is much more effective. The list of common selectors could be cached as well, but it would be pointless after #6957.

Closed as rejected.

Change History (11)

comment:1 Changed 13 months ago by mjethani

  • Cc hfiguiere jsonesen added

comment:2 Changed 13 months ago by mjethani

  • Description modified (diff)

comment:3 Changed 13 months ago by mjethani

  • Blocked By 6652 added

comment:4 Changed 13 months ago by mjethani

I think this depends on #6652 because right now we always have some selectors to add to the unconditional selector list.

comment:5 Changed 13 months ago by mjethani

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

comment:6 Changed 13 months ago by mjethani

  • Description modified (diff)

comment:7 Changed 13 months ago by mjethani

  • Description modified (diff)

comment:8 Changed 13 months ago by mjethani

  • Summary changed from Avoid making copies of generic selector lists to Avoid making copies of common selector list

comment:9 Changed 13 months ago by mjethani

  • Priority changed from P2 to P3

comment:10 Changed 13 months ago by mjethani

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

comment:11 Changed 13 months ago by mjethani

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