Closed
Bug 1462808
Opened 6 years ago
Closed 6 years ago
Convert PageStyleHandler to a singleton
Categories
(Firefox :: Menus, enhancement, P1)
Firefox
Menus
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•6 years ago
|
||
mozreview-review |
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 4•6 years ago
|
||
mozreview-review |
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+
Updated•6 years ago
|
Priority: -- → P1
Comment hidden (mozreview-request) |
Comment 6•6 years ago
|
||
mozreview-review |
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
Comment 8•6 years ago
|
||
bugherder |
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
Comment 9•6 years ago
|
||
This push caused a pretty significant content memory usage improvement: https://screenshots.firefox.com/hkTnF3iyXrIdvJdC/treeherder.mozilla.org
Assignee | ||
Comment 10•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Whiteboard: [fxperf:p1]
Assignee | ||
Updated•6 years ago
|
Blocks: memshrink-content
Comment 11•6 years ago
|
||
(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
Comment 12•6 years ago
|
||
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
Comment 13•6 years ago
|
||
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.
Description
•