Opened 6 months ago

Closed 6 months ago

Last modified 2 weeks ago

#6956 closed change (fixed)

Move the extension's style sheet generation logic into core

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

https://codereview.adblockplus.org/29882562/

Description (last modified by mjethani)

Background

The extension generates the style sheet to be injected into each frame on its own, but this is the kind of logic that would ideally belong in core (similar to how code generation for snippets is done by core). Keeping this logic in the extension keeps us from doing certain optimizations. Mainly, after #6955, we should be able to maintain only one copy of the generic style sheet that is injected into every frame, thus saving a significant amount of memory in the extension.

See also #6957.

What to change

Move the createStyleSheet utility function from adblockpluschrome's lib/contentFiltering.js into lib/elemHide.js.

Integration notes

While this would be a non-breaking change, adblockpluschrome should be updated soon after to use the core version of createStyleSheet.

Hints for testers

The change here is still dead code as far as the extension is concerned and is covered by unit tests.

Change History (13)

comment:1 Changed 6 months ago by mjethani

  • Description modified (diff)
  • Owner set to mjethani

comment:2 Changed 6 months ago by mjethani

  • Description modified (diff)

comment:3 Changed 6 months ago by mjethani

  • Cc hfiguiere jsonesen added

comment:4 Changed 6 months ago by mjethani

  • Review URL(s) modified (diff)

comment:5 Changed 6 months ago by mjethani

  • Cc sebastian added

comment:6 Changed 6 months ago by mjethani

Sebastian raised the question of whether generators should be used in this code.

I took some measurements with #ps29882563. First, I modified the createStyleSheet function to the following:

  createStyleSheet(selectors)
  {
    let s = performance.now();
    let styleSheet = [...createRules(selectors)].join("\n");
    window.n += performance.now() - s;
    return styleSheet;
  }

And I set window.n to 0 at the end of the file.

Then I closed all the windows in the browser and rebuilt the extension. After loading the rebuilt extension, I waited for some time for the first round of garbage collection to run (so there's no processing going on in the background), and then I executed the following on the Bash command prompt:

for i in $(seq 25); do open -a 'Google Chrome Canary' "http://example.com/?n=$i"; done

Then I examined the value of window.n.

I did the above 5 times (starting from reloading the extension) and here are the 5 values I got for window.n (rounded):

334
460
213
270
450

Average with generators: 345.4

Next, I modified the createRules and splitSelectors functions to return arrays instead, using this process:

function* foo()
{
  for (...)
  {
    yield x;
  }
}

Changed to:

function foo()
{
  let list = [];

  for (...)
  {
    list.push(x);
  }

  return list;
}

Then I modified one line in createStyleSheet to do createRules(selectors).join("\n") instead of [...createRules(selectors)].join("\n").

I then ran the above test 5 times again and got these values for window.n (rounded):

499
300
550
342
501

Average with arrays: 438.4

It looks like the performance with generators is at least comparable here if not better.

comment:7 Changed 6 months ago by mjethani

  • Blocking 6957 added

comment:8 Changed 6 months ago by mjethani

  • Status changed from new to reviewing

comment:9 Changed 6 months ago by abpbot

A commit referencing this issue has landed:
Issue 6956 - Move extension's style sheet generation into core

comment:10 Changed 6 months ago by mjethani

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

comment:11 Changed 6 months ago by mjethani

While the extension could start using this function right away, let's wait for #6957: it may not have to call this function directly after all, if we can return the required CSS along with the list of selectors in a single function call.

comment:12 Changed 6 weeks ago by abpbot

A commit referencing this issue has landed:
Issue 6956 - Move extension's style sheet generation into core

comment:13 Changed 2 weeks ago by ukacar

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