1956081 - Fenix applink load may wait on Network Connectivity Service's initial checks, introducing additional latency on slower networks

Status: RESOLVED FIXED

(Core :: Networking, task, P2)

Description

[Andrew Creskey [:acreskey]]

The exact mechanism isn't clear to me yet, but from a series of Fenix Applink Startup profiles, the network connectivity service's initial checks look to be blocking the document load.

https://share.firefox.dev/41RhIe4
https://share.firefox.dev/422Duf1
https://share.firefox.dev/4l0HhlK

In all of these cases we see the target url, https://theme-crave-demo.myshopify.com/, start to load immediately after the connectivity service's request of http://detectportal.firefox.com/success.txt?ipv6 completes (possibly DNS check).

(I believe these requests are coming from the connectivity service, network.connectivity-service.enabled because the captive portal service appears to be disabled, i.e. network.captive-portal-service.enabled:false)

Comment 1

[Andrew Creskey [:acreskey]]

Perhaps somewhere up the stack we are blocking on the connectivity service completing initial tests?

I've disabled the connectivity services in this PerfCompare, currently showing a 10% improvement but the test may be noisy so taking with a grain of salt
https://perf.compare/compare-results?baseRev=f6fa6f1d572aed73aaddd82dfda787331616bea5&newRev=c0d19018e3413053fe5a13f18d2e032f8725c671&baseRepo=try&newRepo=try&framework=15

In this profile I've disabled the connectivity service. Still a large delay from the first network requests (location services, wallpers) to the applink target, but that is likely separate issue.
https://share.firefox.dev/4j5pckJ

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

This may be on purpose, since a pageload if we're in a captive portal situation will get hijacked.
If it isn't, this could be a win

Comment 3

[Andrew Creskey [:acreskey]]

The CI runs on the shopify test did not pickup any improvements and I've also since learned that those tests are not stable enough to draw conclusions from.

From a local test, disabling network.connectivity-service.enabled did not yield any improvements.
It looks like load is waiting for the browser to be created, mozilla::dom::ContentParent::CreateBrowser.
See also, bug to create applink host speculative connection, bug 1956356.

Comment 4

[Valentin Gosu [:valentin]]

Looking at the profile from comment 1 I did notice that we do two connectivity checks, one in response to the a connectivity change, then one in response to the idle-startup-finished link
I think the good fix here would be to just avoid doing any checks before idle-startup-finished.

Comment 5

[Valentin Gosu [:valentin]]

Attachment: Bug 1956081 - Don't perform any connectivity checks before idle-startup-finished r=#necko

Comment 6

[Valentin Gosu [:valentin]]

!!!NOTE!!!
You'll be able to find a performance comparison here once the tests are complete (ensure you select the right framework):
https://perf.compare/compare-results?baseRev=40b49e9bb1737d03e791e4b6517a9a500f265abd&newRev=bb99068e0bc7149592465d2630628365b85d1566&baseRepo=try&newRepo=try&framework=15

The old comparison tool is still available at this URL:
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=40b49e9bb1737d03e791e4b6517a9a500f265abd&newProject=try&newRevision=bb99068e0bc7149592465d2630628365b85d1566&framework=15

---

•      2 commits/try-runs are created...          *
  

---

Base revision's try run: https://treeherder.mozilla.org/jobs?repo=try&revision=40b49e9bb1737d03e791e4b6517a9a500f265abd
Local revision's try run: https://treeherder.mozilla.org/jobs?repo=try&revision=bb99068e0bc7149592465d2630628365b85d1566

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5106137ef312
Don't perform any connectivity checks before idle-startup-finished r=necko-reviewers,kershaw

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02695117ae0e
Revert "Bug 1956081 - Don't perform any connectivity checks before idle-startup-finished r=necko-reviewers,kershaw" for causing failures at test_network_connectivity_service.js.

Comment 9

[Atila Butkovits]

Backed out for causing failures at test_network_connectivity_service.js.

Backout link: https://hg-edge.mozilla.org/integration/autoland/rev/02695117ae0e5c4b7ac21e07eb9575051303e494

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&searchStr=linux%2C18.04%2Cx64%2Cwebrender%2Cdebug%2Cxpcshell%2Ctests%2Ctest-linux1804-64-qr%2Fdebug-xpcshell%2Cx3&revision=5106137ef312aee70a53a0684d3d7b01a34e7f23&selectedTaskRun=P4B1_gQRQmqqEra54jqogA.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=507026979&repo=autoland&lineNumber=3743

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc91e5d29183
Don't perform any connectivity checks before idle-startup-finished r=necko-reviewers,kershaw

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/bc91e5d29183

Comment 12

[Andrew Creskey [:acreskey]]

Even though it has already landed, I compared this patch on the newssite-applink-startup applink_startup (without profiling runs) by disabling the option.

Possibly it's leading to improvements of up to ~2.6% on the test. But I'm adding more retriggers to gain confidence.

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

I'll try to run it live in a large batch.
If we have evidence that removing the additional requests is measurably beneficial, that could help in deferring all of the other launch request loads.

Comment 13

[Andrew Creskey [:acreskey]]

Attachment: valentin_fix.txt

Dataset with applink_startup results from Valentin's fix.

Comment 14

[Andrew Creskey [:acreskey]]

Attachment: revert_valentin_fix.txt

Dataset with applink_startup results where Valentin's fix was reverted via pref

Comment 15

[Andrew Creskey [:acreskey]]

I compared this fix on a long run of the shopify applink test (250 iterations instead of 5).
It didn't pickup any measurable improvements, but this test is relatively noisy, even locally, standard deviation of 140ms.

We may need to disable all of the requests that preceed the applink URL to measure the impact in a local test.