Closed Bug 1462808 Opened 6 years ago Closed 6 years ago

Convert PageStyleHandler to a singleton

Categories

(Firefox :: Menus, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: Felipe, Assigned: Felipe)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxperf:p1])

Attachments

(2 files)

PageStyleHandler currently runs once per tab because it directly accesses things like `content`, `docShell`, etc.

But it's easy to make it get these references through the event listeners and messages that it receives.

There's no point into making this one a lazy proxy, since it runs for every tab to gather stylesheet info. But moving the code to a singleton obtained from a .jsm should still help by reducing the per-tab memory usage and compilation of the frame script.
Comment on attachment 8977120 [details]
Bug 1462808 - Adjust PageStyleHandler code to handle its events and messages as a singleton.

https://reviewboard.mozilla.org/r/245202/#review251228

FWIW, tempted to suggest this should all go away anyway. If we really need to keep it, maybe we should implement in compiled code (rust?), that would avoid the process memory overhead (assuming we can share the program bytecode trivially).

::: browser/modules/PageStyleHandler.jsm:45
(Diff revision 1)
>        case "PageStyle:Disable":
> -        this.markupDocumentViewer.authorStyleDisabled = true;
> +        this.getViewer(content).authorStyleDisabled = true;
>          break;
>      }
>  
> -    this.sendStyleSheetInfo();
> +    this.sendStyleSheetInfo(content);

So this is all a bit messy. My understanding is as follows:

1) we get the message on the message manager for the toplevel content anyway (because that's where the parent sends it)
2) `msg.target` is actually a message manager instance (!)


So I believe you could refactor this code so that `sendStyleSheetInfo` takes a message manager directly. From the message listener, you could just pass `msg.target`.

::: browser/modules/PageStyleHandler.jsm:49
(Diff revision 1)
>  
> -    this.sendStyleSheetInfo();
> +    this.sendStyleSheetInfo(content);
> +  },
> +
> +  handleEvent(event) {
> +    this.sendStyleSheetInfo(event.target.ownerGlobal);

From here, you could do:

```js
let win = event.target.ownerGlobal;
if (win != win.top) {
  return; // We don't support changing stylesheets for subframes anyway, I think?
}
let {docShell} = event.view.document;
this.sendStyleSheetInfo(docShell.tabChild.messageManager);
```
Attachment #8977120 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8977119 [details]
Bug 1462808 - Part 0. Copy PageStyleHandler code from tab-content.js to a new file.

https://reviewboard.mozilla.org/r/245200/#review251232

Thanks for splitting this out!
Attachment #8977119 - Flags: review?(gijskruitbosch+bugs) → review+
Priority: -- → P1
Comment on attachment 8977120 [details]
Bug 1462808 - Adjust PageStyleHandler code to handle its events and messages as a singleton.

https://reviewboard.mozilla.org/r/245202/#review251584
Attachment #8977120 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d4d8797e848
Part 0. Copy PageStyleHandler code from tab-content.js to a new file. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/abb1759701b3
Adjust PageStyleHandler code to handle its events and messages as a singleton. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/2d4d8797e848
https://hg.mozilla.org/mozilla-central/rev/abb1759701b3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
This push caused a pretty significant content memory usage improvement:

https://screenshots.firefox.com/hkTnF3iyXrIdvJdC/treeherder.mozilla.org
I believe it's also related with this cpstartup improvement:
https://treeherder.mozilla.org/perf.html#/alerts?id=13468
https://screenshots.firefox.com/oZ25YJ5xqZEl6a8d/treeherder.mozilla.org

There are more pushes in the alert's range, but it matches what I saw running this test locally
Whiteboard: [fxperf:p1]
(In reply to :Felipe Gomes (needinfo me!) from comment #10)
> I believe it's also related with this cpstartup improvement:
> https://treeherder.mozilla.org/perf.html#/alerts?id=13468
> https://screenshots.firefox.com/oZ25YJ5xqZEl6a8d/treeherder.mozilla.org
> 
> There are more pushes in the alert's range, but it matches what I saw
> running this test locally

I can confirm that :)

== Change summary for alert #13446 (as of Fri, 25 May 2018 17:16:35 GMT) ==

Improvements:

  3%  cpstartup content-process-startup windows7-32 opt e10s stylo     183.67 -> 178.33
  3%  cpstartup content-process-startup windows10-64 opt e10s stylo    172.77 -> 168.08
  2%  cpstartup content-process-startup windows10-64 pgo e10s stylo    165.12 -> 161.08

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=13446
I also noticed this perf win on OS X, though I'm not really sure about the originating bug. Either this bug or one of bug  1461548, bug 1462673, bug 1461444, bug 1461247 or bug 1461248 brought it.

== Change summary for alert #13466 (as of Fri, 25 May 2018 14:22:32 GMT) ==

Improvements:

  6%  tpaint osx-10-10 opt e10s stylo     321.00 -> 302.99

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=13466
Depends on: 1466256
Extra perf wins!

== Change summary for alert #13443 (as of Fri, 25 May 2018 14:22:32 GMT) ==

Improvements:

  5%  Base Content JS linux64 opt stylo     8,306,431.67 -> 7,865,931.33
  5%  Base Content JS windows7-32 opt stylo 6,166,721.67 -> 5,868,834.67
  4%  Base Content JS osx-10-10 opt stylo   8,378,031.67 -> 8,013,384.67
  3%  Base Content JS windows10-64 opt stylo7,910,716.00 -> 7,704,074.67

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=13443
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: