Convert content-sessionStore to C++ - stage I
Categories
(Firefox :: Session Restore, enhancement, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox68 | --- | fixed |
People
(Reporter: kmag, Assigned: alchen)
References
(Blocks 3 open bugs)
Details
Attachments
(2 files, 1 obsolete file)
content-sessionStore.js is currently loaded into every tab frameloader. Which means it gets loaded multiple times per process, which is not great. But even when loaded only once, it uses about 86K. Add to that 17K from ContentRestore.jsm and 12K from SessionHistory.jsm, and we're up to at least 120K per process, if none of the other helper JSMs get loaded. The things that these scripts do can easily be done by C++ (some of them more easily), so there doesn't seem to be a good justification for loading this much JS into every process for the sake of session restore.
Reporter | ||
Comment 1•6 years ago
|
||
Ah, and of course another 12K for Utils.jsm.
Comment 2•6 years ago
|
||
I'm probably going to be working on this a bit for fission in the coming weeks, so self-assigning.
Comment 3•6 years ago
|
||
I'd say this is also [qf] work. Session store shows up rather often in the performance profiles.
Bug 1474130 might end up blocking much work here, but for now bug 1378651 and bug 1364019 are possibly reasonable to do. There are other priorities in the way. [Source: https://bugzilla.mozilla.org/show_bug.cgi?id=1413525#c55]
Fixing bug 1474130 is needed before the work on session management API (bug 1427928) can be continued: https://bugzilla.mozilla.org/show_bug.cgi?id=1427928#c48
(In reply to Robert Ab from comment #5) > Fixing bug 1474130 is needed before the work on session management API (bug > 1427928) can be continued: > https://bugzilla.mozilla.org/show_bug.cgi?id=1427928#c48 Do you know when this bug will be fixed?
Updated•6 years ago
|
Assignee | ||
Comment 8•6 years ago
|
||
Before starting the rewriting, I would like to rewrite the js modules which are used in ContentSessionStore.jsm. Bug 1497144: DocShellCapabilities.jsm and ScrollPosition.jsm Bug 1497146: FormData.jsm Bug 1497147: Utils.jsm (rewrite mapFrameTree)
Assignee | ||
Comment 9•6 years ago
|
||
Get rid of DocShellCapabilities.jsm and ScrollPosition.jsm
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 10•5 years ago
|
||
In this rewriting, we will replace the communication between content and parent from message passing to IPC.
Updated•5 years ago
|
Assignee | ||
Comment 11•5 years ago
|
||
The attached file is the proposal for this rewriting.
There are several stages:
[Stage I] Rewrite several listeners into C++ and using IPDL to communicate from the content process to the parent process. For compatibility, we will construct the original data format(JSON format) in the parent process.
[Stage II] After we make sure the compatibility and stability of the stage I mechanism. In stage II, rewrite sessionStorageListener by using the same mechanism.
[Stage III] Rewrite sessionHistorListener if needed. (related bugs: Bug 1438272, Bug 1445459, Bug 1507287)
After "stage III", the rest of contentSessionStore.jsm is "restore" functionality. We will re-evaluate the rewriting again. Currently, "ContentRestore.jsm" is loaded lazily(bug 1474131).
Assignee | ||
Comment 12•5 years ago
|
||
Assignee | ||
Comment 13•5 years ago
|
||
In the current patch, I add three functions in the "nsIXULBrowserWindow.idl".
The three functions are implemented in "browser/base/content/browser.js".
The function "updateSessionStore()" will call SessionStore.updateSessionStoreFromCPP() with "tab" and "data".
Currently, I cannot update TabState correctly. The tab I get seems not the right one.
@TabParent.cpp
mozilla::ipc::IPCResult TabParent::RecvSessionStoreUpdate()
browser = mFrameElement->AsBrowser
@browser/base/content/browser.js
updateSessionStore: function XWB_updateSessionStore(aBrowser)
tab = gBrowser.getTabForBrowser(aBrowser)
@sessionStore.jsm
updateSessionStoreFromCPP(aTab, aData) {
let browser = aTab.linkedBrowser;
let win = browser.ownerGlobal;
TabState.update(browser, aData);
this.saveStateDelayed(win);
}
Originally, the sessionStore get "browser" from "aMessage.target".
https://searchfox.org/mozilla-central/rev/aae527894a97ee3bbe0c2cfce9c67c59e8b8fcb9/browser/components/sessionstore/SessionStore.jsm#829
receiveMessage(aMessage) {
// If we got here, that means we're dealing with a frame message
// manager message, so the target will be a <xul:browser>.
var browser = aMessage.target;
let win = browser.ownerGlobal;
....
switch (aMessage.name) {
case "SessionStore:update":
....
TabState.update(browser, aMessage.data);
this.saveStateDelayed(win);
Assignee | ||
Comment 14•5 years ago
|
||
Now investigating how it works on TabParent::ReceiveMessage().
Assignee | ||
Comment 15•5 years ago
|
||
The current patch is failed on "browser_privatetab.js".
In the test, it creates a new window and attaches a frame script.
Then create a new tab in the new window that will load the frame script.
https://searchfox.org/mozilla-central/rev/4763b8d576ce52625d245d1ab6d9404ea025b026/browser/components/sessionstore/test/browser_privatetabs.js#62
After "BrowserTestUtils.addTab", TabChid observe a "privateModeChanged" event and do an IPC to parent for updating the TabState. However, the tab by gBrowser.getTabForBrowser(aBrowser) is null.
Assignee | ||
Comment 16•5 years ago
|
||
In current implementation, I cannot observe the behavior of adding a new tab from tabBrowser.
Need to find another way to add "PrivacyTransitionObserver".
Assignee | ||
Comment 17•5 years ago
|
||
(In reply to Alphan Chen [:alchen] from comment #16)
In current implementation, I cannot observe the behavior of adding a new tab from tabBrowser.
Need to find another way to add "PrivacyTransitionObserver".
In the end, it is caused by opening a "about:mozilla" tab.
We loaded it in the parent.
Assignee | ||
Comment 18•5 years ago
|
||
For the case of opening "about:mozilla" tab, we find a way to add the listener.
By this way, we can observe "PrivateModeChanged" and collect the value correctly.
However, I don't get the notification for the WebProgress state change.
Also, we need to observe the following events in our listener.
- Input event for formdata
- Mozvisualscroll event for scroll position
- MozSessionStorageChanged for SessionStorage
Updated•5 years ago
|
Assignee | ||
Comment 19•5 years ago
|
||
In the latest revision, three listeners are implemented with all tests pass.
- ScrollPosition/Privacy/DocCapability listeners
However, JSM implementation has a force update mechanism to update the value quickly.
The mochitest will use it before checking the Scroll position.
/*
async function checkScroll(tab, expected, msg) {
let browser = tab.linkedBrowser;
await TabStateFlusher.flush(browser);
...
*/
Ideally, we should also flush the changes from C++ listener at the same time.
Need to think about how to do it.
[Other possibilities]
- Introduce another force update mechanism for our C++ implementation.
- Use the existing pref for sessionStore, set it as false before doing the test.
// This pref controls whether or not we send updates to the parent on a timeout
// or not, and should only be used for tests or debugging.
const TIMEOUT_DISABLED_PREF = "browser.sessionstore.debug.no_auto_updates";
Comment 20•5 years ago
|
||
- Use the existing pref for sessionStore, set it as false before doing the test.
a) Wouldn't that be inherently racy?
b) I suspect that there could be scenarios where we'd want to flush the state outside of tests, too.
Updated•5 years ago
|
Assignee | ||
Comment 21•5 years ago
|
||
(In reply to Jan Henning [:JanH] from comment #20)
- Use the existing pref for sessionStore, set it as false before doing the test.
a) Wouldn't that be inherently racy?
b) I suspect that there could be scenarios where we'd want to flush the state outside of tests, too.
In the latest revision, we have added support for the flush function.
Try result with Current patch.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f653f4580d462eadb3b6074757468ac115407f18
There are two testcases in which we observed leakage of 1 docshell.
devtools/client/aboutdebugging-new/test/browser/browser_aboutdebugging_devtoolstoolbox_focus.js | leaked 1 docShell(s) until shutdown
browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | leaked 1 docShell(s) until shutdown
If we don't add systemEventListener in parent only case, the leakage will disappear.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaeb4be71e3fe3f17b897d0cc5a7870021ffa65c
Assignee | ||
Comment 22•5 years ago
|
||
Rename this bug as "Convert content-sessionStore to C++ - Stage I"
There are some new mechanisms introduced in the current rewriting.
I would like to evaluate those mechanisms with three native listeners: PrivacyListener, DocShellCapabilitiesListener and ScrollPositionListener.
Stage II includes FormDataListener and SessionStorageListener rewriting.
Stage III will be the rewriting for SessionHistoryListener.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 23•5 years ago
|
||
(In reply to Alphan Chen [:alchen] from comment #22)
Stage II includes FormDataListener and SessionStorageListener rewriting.
Bug 1544371
Stage III will be the rewriting for SessionHistoryListener.
Bug 1507287
Assignee | ||
Comment 24•5 years ago
|
||
Try result with the latest revision.
It looks fine.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e7551b304bafc1e90c1da565ac1a5b910a05d76
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 25•5 years ago
|
||
Try is fine.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a4374a0e43f4bacede7e7d0f79c4082bd1730c4
Comment 26•5 years ago
|
||
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3288c43195a2
Implement ScrollPosition/Privacy/DocCapability listeners in C++ r=peterv
Comment 27•5 years ago
|
||
bugherder |
Comment 28•5 years ago
|
||
Is any reason why content-sessionStore is converted to C++, but not to Rust?
Assignee | ||
Comment 29•5 years ago
|
||
(In reply to Robert Ab from comment #28)
Is any reason why content-sessionStore is converted to C++, but not to Rust?
In my opinion, there is no obvious advantage to do this rewriting in RUST.
Besides, it is not an independent module.
We need lots of interactions(with dom events/IPC communications...) in this rewriting.
Using C++ here is suitable.
Comment 30•5 years ago
|
||
Hi! Looks like the following improvements came with your patch:
== Change summary for alert #20840 (as of Tue, 07 May 2019 12:27:35 GMT) ==
Improvements:
1% Base Content JS windows7-32-shippable opt 3,255,557.33 -> 3,224,362.67
0.26% Base Content JS windows10-64-shippable opt 4,142,121.33 -> 4,131,218.67
0.26% Base Content JS windows10-64-shippable-qr opt 4,142,052.00 -> 4,131,280.00
For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=20840
Description
•