Closed Bug 1474130 Opened 2 years ago Closed 1 year ago

Convert content-sessionStore to C++ - stage I

Categories

(Firefox :: Session Restore, enhancement, P2)

57 Branch
enhancement

Tracking

()

RESOLVED FIXED
Firefox 68
Fission Milestone M2
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.
Ah, and of course another 12K for Utils.jsm.
Whiteboard: [overhead:120k] → [overhead:135k]
Blocks: ss-perf
Priority: -- → P3
I'm probably going to be working on this a bit for fission in the coming weeks, so self-assigning.
Assignee: nobody → nika
Blocks: 613498
I'd say this is also [qf] work. Session store shows up rather often in the performance profiles.
Blocks: ss-SM
No longer blocks: Session_managers
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?
Flags: needinfo?(nika)
I will take care of this bug.
Assignee: nika → alchen
Flags: needinfo?(nika)
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)
Status: NEW → ASSIGNED
Depends on: 1497144, 1497146, 1497147
Get rid of DocShellCapabilities.jsm and ScrollPosition.jsm
Attachment #9015520 - Attachment is obsolete: true
Depends on: 1507286, 1507287
Blocks: ss-feature

In this rewriting, we will replace the communication between content and parent from message passing to IPC.

Fission Milestone: --- → M2

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).

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);

Now investigating how it works on TabParent::ReceiveMessage().

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.

In current implementation, I cannot observe the behavior of adding a new tab from tabBrowser.
Need to find another way to add "PrivacyTransitionObserver".

(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.

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
Attachment #9050151 - Attachment description: Bug 1474130 - WIP → Bug 1474130 - Implement ScrollPosition/Privacy/DocCapability listeners in C++

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]

  1. Introduce another force update mechanism for our C++ implementation.
  2. 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";
  1. 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.

Priority: P3 → P2
Whiteboard: [overhead:135k] → [4/11] Stage 1 on review. Stage 2 and 3 to follow. [overhead:135k]

(In reply to Jan Henning [:JanH] from comment #20)

  1. 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

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.

Summary: Convert content-sessionStore to C++ → Convert content-sessionStore to C++ - stage I

(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

Blocks: 1544371, 1507287
No longer depends on: 1507287
No longer blocks: Session_managers
Whiteboard: [4/11] Stage 1 on review. Stage 2 and 3 to follow. [overhead:135k] → [4/29] addressing review comments
Whiteboard: [4/29] addressing review comments

Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3288c43195a2
Implement ScrollPosition/Privacy/DocCapability listeners in C++ r=peterv

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Blocks: 1549973
Regressions: 1554512

Is any reason why content-sessionStore is converted to C++, but not to Rust?

Flags: needinfo?(alchen)

(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.

Flags: needinfo?(alchen)

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

Blocks: 1562889
Blocks: 1564412
No longer blocks: ss-SM
No longer blocks: ss-perf
You need to log in before you can comment on or make changes to this bug.