Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#4038 closed change (fixed)

Increase SELECTOR_GROUP_SIZE used by addElemHideSelectors

Reported by: kzar Assignee: kzar
Priority: P2 Milestone: Adblock-Plus-1.12-for-Chrome-Opera-Safari
Module: Platform Keywords:
Cc: sebastian, trev, Ross, scheer Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29341245/

Description (last modified by sebastian)

Background

The addElemHideSelectors function inside the include.preload.js content script inserts element hiding selectors into the stylesheet. Historically browsers had issue with adding too many selectors at once and so we inserted just 20 at a time. These days addElemHideSelectors is a performance pain point #4036 and it is likely modern browsers allow many rules to be inserted at a time.

What to change

Investigate how many rules can be inserted at a time (or how long the overall length of an inserted selector can be). As far as possible test this in all supported browsers. Once a safe number (or length) has been decided increase the SELECTOR_GROUP_SIZE constant.

Hints for testers

Ensure element hiding still works for all supported platforms. This change is unlikely to cause problems but we should still test it. Most importantly test old supported versions of Chrome and Safari.

Change History (8)

comment:1 Changed 4 years ago by sebastian

  • Priority changed from Unknown to P2
  • Ready set

comment:2 Changed 4 years ago by sebastian

For reference, as discussed on IRC, Wladimir experimented with different number of selectors and couldn't reproduce any limitation on Chrome but believes that it will be at 1M selectors. On Safari, the limit seems to be 8192 selectors, but some selectors might be counted as multiple selectors.

So I suggest to insert selectors in chunks of 100 on Safari, and all selectors in one chunk on other platforms. If we should ever reach 1M selectors the browser will explode for most users anyway.

comment:3 Changed 4 years ago by kzar

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

comment:4 Changed 4 years ago by kzar

  • Owner set to kzar

We found that in practice Chrome had an issue when the limit was more than around 6200. We decided therefore to have a limit of 200 for all browsers for now.

comment:5 Changed 4 years ago by abpbot

A commit referencing this issue has landed:
https://hg.adblockplus.org/adblockpluschrome/rev/0b402b4e978a

comment:6 Changed 4 years ago by kzar

  • Cc Ross scheer added
  • Milestone set to Adblock-Plus-for-Chrome-Opera-Safari-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:7 Changed 4 years ago by kzar

  • Description modified (diff)

comment:8 Changed 4 years ago by sebastian

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