Opened 14 months ago

Last modified 9 months ago

#6957 closed change

Combine element hiding selector matching with style sheet generation — at Version 13

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

https://codereview.adblockplus.org/29886555/
https://codereview.adblockplus.org/29893618/
https://codereview.adblockplus.org/29893628/
https://codereview.adblockplus.org/29897558/

Description (last modified by mjethani)

Background

After #6956, it will be possible for core to generate the style sheet on its own. In order to optimize style sheet generation in practice, it could be combined with the code for looking up selectors for a domain. The "default" style sheet, based on the list of "unconditional" selectors, would have to be generated only once and cached. Then each call to the style sheet generation code would only have to generate a second style sheet, based on some ~1,000 selectors that don't apply to all domains, and concatenate it with the default style sheet.

This would make the combination of selector lookup and style sheet generation much faster. It might also have benefits for memory usage, though this may no longer be a concern after #6967.

What to change

In lib/elemHide.js, add a new generateStyleSheetForDomain function that performs the same task as getSelectorsForDomain but also returns a generated style sheet (CSS code to be injected directly into the document). This function should generate the default style sheet, based on the list of "unconditional" selectors, only once and then always use the cached value for subsequent calls.

The cached default style sheet should be invalidated when the set of filters is updated.

Change History (13)

comment:1 Changed 14 months ago by mjethani

  • Blocked By 6956 added
  • Cc jsonesen hfiguiere added
  • Description modified (diff)
  • Ready set
  • Review URL(s) modified (diff)
  • Summary changed from Return cached string object for unconditional style sheet to Combine element hiding selector matching with style sheet generation

comment:2 Changed 14 months ago by mjethani

  • Owner set to mjethani

comment:3 Changed 14 months ago by mjethani

  • Status changed from new to reviewing

comment:4 Changed 14 months ago by mjethani

  • Cc sebastian added

comment:5 Changed 14 months ago by mjethani

@sebastian what do you think about this idea?

The ElemHide module could return the style sheet along with the selectors. Internally it would have a cached "default" style sheet (see patch), based on the list of selectors that are applied unconditionally on all domains. This would slow down the selector lookup slightly, but this would be more than offset by the fact that the extension wouldn't have to generate the style sheet separately.

The only downside is that this would be a performance hit for platforms that don't support user style sheets, but the extension could just call the old getSelectorsForDomain instead on those platforms.

Last edited 14 months ago by mjethani (previous) (diff)

comment:6 follow-up: Changed 14 months ago by sebastian

In order to tell whether this is a good idea we should compare the complexity added, performance gain and memory usage after the change.

At the current point I don't see yet how it might improve the memory usage, but would rather assume that the caching will add up to the memory footprint?

How would that make things worse than it's now on platforms that don't support user stylesheets?

Can you specify the cache invalidation? I assume we'd regenerate the stylesheet text for the unconditional element hiding rules every time element hiding filters are added/removed/disabled/enabled. What performance implications do you expect this to have on filter changes?

comment:7 in reply to: ↑ 6 Changed 14 months ago by mjethani

Replying to sebastian:

At the current point I don't see yet how it might improve the memory usage, but would rather assume that the caching will add up to the memory footprint?

After #6967, it won't help with the memory usage. It should instead add ~600 KB to the footprint.

How would that make things worse than it's now on platforms that don't support user stylesheets?

The additional cost here would be the cost of generating the style sheet for about ~1,500 selectors.

On platforms that do support user style sheets, this is a net gain, because the extension doesn't have to generate the style sheet at all.

Can you specify the cache invalidation? I assume we'd regenerate the stylesheet text for the unconditional element hiding rules every time element hiding filters are added/removed/disabled/enabled. What performance implications do you expect this to have on filter changes?

Filters are added all in the beginning when the extension is loaded, before the style sheet is ever generated, or when a new copy of a subscription is downloaded. Invalidating the style sheet should be similar to the invalidation of the "unconditional" selectors, which also happens when the current set of filters is modified. We're setting one variable to null, now we have to set two variables to null. I don't expect this to have any impact on performance.

comment:8 Changed 14 months ago by mjethani

  • Priority changed from P2 to P3

comment:9 follow-up: Changed 14 months ago by sebastian

Microsoft Edge (the only WebExtension platform that doesn't support user stylesheets) doesn't support Shadow DOM v0 either (and never will since it has been superseded by Shadow DOM v1 which is useless for our purpose). That said we could now get rid of the whole logic injecting stylesheets in Shadow DOM altogether. Also this means the same CSS we inject as a user stylesheet on modern versions of Chrome and Firefox, could just be injected as-is into an author stylesheet in Microsoft Edge (and consequently older versions of Chrome and Firefox). Maybe I miss something, but otherwise it seems to me that browsers that don't support user stylesheets could benefit from this change in the same way.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 14 months ago by mjethani

Replying to sebastian:

Microsoft Edge (the only WebExtension platform that doesn't support user stylesheets) doesn't support Shadow DOM v0 either (and never will since it has been superseded by Shadow DOM v1 which is useless for our purpose). That said we could now get rid of the whole logic injecting stylesheets in Shadow DOM altogether. Also this means the same CSS we inject as a user stylesheet on modern versions of Chrome and Firefox, could just be injected as-is into an author stylesheet in Microsoft Edge (and consequently older versions of Chrome and Firefox). Maybe I miss something, but otherwise it seems to me that browsers that don't support user stylesheets could benefit from this change in the same way.

Yes, the only question is the cost of transmitting a 660 KB style sheet from the background page to the content script. This would be in addition to the list of selectors (which is also needed).

If you mean that we should use tabs.insertCSS on Edge without the cssOrigin option, that's fine. In that case, I guess we get rid of all the style sheet logic in the content script. It sounds like a good idea to me.

Last edited 14 months ago by mjethani (previous) (diff)

comment:11 in reply to: ↑ 10 Changed 14 months ago by sebastian

Replying to mjethani:

If you mean that we should use tabs.insertCSS on Edge without the cssOrigin option, that's fine. In that case, I guess we get rid of all the style sheet logic in the content script. It sounds like a good idea to me.

Agreed, let's do that.

comment:12 Changed 14 months ago by abpbot

comment:13 Changed 14 months ago by mjethani

  • Review URL(s) modified (diff)
Note: See TracTickets for help on using tickets.