1960712 - Loading geckoview.xhtml goes through redirect machinery - can we eliminate the redirect?

Status: NEW

(Core :: DOM: Navigation, enhancement, P2)

Description

[Markus Stange [:mstange]]

This profile shows that the request for chrome://geckoview/content/geckoview.xhtml goes through the following runnables:

• nsJarChannel::ContinueOpenLocalFile
• nsPipeInputStream AsyncWaitCallback for the stream copier
• nsAsyncRedirectVerifyHelper
• onRedirectVerifyCallback (via runLater in WebRequest.sys.mjs)
• nsAsyncVerifyRedirectCallbackEvent
• two RedirectToRealChannel promises
• another nsPipeInputStream AsyncWaitCallback (from DocumentLoadListener::ResumeSuspendedChannel)

What's the redirect for? Can we eliminate it?

geckoview.xhtml is Android's equivalent to browser.xhtml - it's the "window" document in the parent process. Loading it is on the critical path in the applink startup scenario (bug 1945906). We should try very hard to cut down the number of main thread round trips that are involved in loading it.

Comment 1

[Kershaw Chang [:kershaw]]

My apologies that I'm not an expert in document loading.
I believe the redirect calls are coming from the document channel, but I'm not sure how we could avoid them. I think Nika would be the best person to advise here.

Comment 2

[Nika Layzell [:nika] (ooo, ni? for response)]

(In reply to Markus Stange [:mstange] from comment #0)

What's the redirect for? Can we eliminate it?

geckoview.xhtml is Android's equivalent to browser.xhtml - it's the "window" document in the parent process. Loading it is on the critical path in the applink startup scenario (bug 1945906). We should try very hard to cut down the number of main thread round trips that are involved in loading it.

I expect we also will encounter effectively exactly the same runnables when loading browser.xhtml on Desktop, as I don't think Android is doing anything different from Desktop in this situation. On desktop we load from chrome://browser/content/browser.xhtml.

When we load a URI with the chrome:// protocol, it is loaded by first resolving the chrome:// URI to the underlying URI (https://searchfox.org/mozilla-central/rev/931de234363586a45ea9ab66d281960ccf1c3597/chrome/nsChromeProtocolHandler.cpp#102-104), and then opening that URI. In a packaged build of Firefox, this will be a jar URI like: jar:file:///Path/To/browser/omni.ja!/chrome/browser/content/browser/browser.xhtml. On Android I expect this will be a double-nested jar URI like: jar:jar:file:///Path/To/Firefox.apk!/assets/omni.ja!/chrome/geckoview/geckoview.xhtml.

Because this is an async load, we try to open the jar OMT - the jar in this case will already be opened though (as it's the omnijar). The existing omnijar will be returned from CreateLocalJarInput here: https://searchfox.org/mozilla-central/rev/931de234363586a45ea9ab66d281960ccf1c3597/modules/libjar/nsJARChannel.cpp#404-405. There might be some optimization which could be done to detect that the zip to be opened is the omnijar and short-circuit one OMT dispatch there, but I don't know how tricky that would be ottomh.
The RedirectToRealChannel promises and other stuff there are part of how document loads are processed since Fission for DocumentLoadListener. All document loads are processed through the parent process in DocumentLoadListener, and then redirected to the final docshell once the process etc. which handles the load has been determined. Getting rid of that redirect will be probably painful, and require additional special casing for parent-process only loads which I don't think we want.

Comment 3

[Nika Layzell [:nika] (ooo, ni? for response)]

We might be able to do something a bit hacky to avoid using the DocumentChannel loading codepath for chrome docshells, which might improve the number of runnables we're dispatching here.

Something simple to try would be to change https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10568 such that you're also checking mItemType != typeChrome before checking if we should use DocumentChannel, as we can never process-switch a chrome docshell right now. Not sure what that might break.

Comment 4

[Andrew Creskey [:acreskey]]

Logged a bug on increasing the priorities of the runnables in this codepath: https://bugzilla.mozilla.org/show_bug.cgi?id=1966069

Comment 5

[Andrew Creskey [:acreskey]]

Ed, we're not able to evaluate your push of the test from Comment 3 because there's a bug where results from the much slower simpleperf profiling runs are included with other runs (bug 1965803).

Can I ask you to make a new performance comparison, only selecting these jobs from ./mach try perf ?

"perftest-android-hw-a55-aarch64-shippable-startup-fenix-newssite-applink-startup",
"perftest-android-hw-p6-aarch64-shippable-startup-fenix-newssite-applink-startup",
"perftest-android-hw-s24-aarch64-shippable-startup-fenix-newssite-applink-startup"

Comment 6

[Andrew Creskey [:acreskey]]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #3)

We might be able to do something a bit hacky to avoid using the DocumentChannel loading codepath for chrome docshells, which might improve the number of runnables we're dispatching here.

Something simple to try would be to change https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10568 such that you're also checking mItemType != typeChrome before checking if we should use DocumentChannel, as we can never process-switch a chrome docshell right now. Not sure what that might break.

I compared this patch again on the newssite-applink-startup applink_startup (without profiling runs) but it did not pick up any improvements.

https://perf.compare/compare-results?baseRev=77ac96ad1ce81db0107b8ab6082b147499eeb01b&newRev=12940438e49f5bc0b1b87640ec029f3127ddfe2d&baseRepo=try&newRepo=try&framework=15