Closed Bug 1633935 Opened 2 months ago Closed 10 days ago

Allow sending onStartRequest via pHttpBackgroundChannel

Categories

(Core :: Networking: HTTP, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla80
Tracking Status
firefox80 --- fixed

People

(Reporter: junior, Assigned: junior)

References

(Depends on 1 open bug, Blocks 2 open bugs, Regressed 1 open bug)

Details

(Keywords: perf-alert, Whiteboard: [necko-triaged])

Attachments

(14 files, 5 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
279.81 KB, image/png
Details
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
No description provided.
Depends on: 1633942

Upload a WIP to see if we suffer from buzy pContent
We dispatch the OnStartRequest to background thread, thru pBackground and dispatch back to main thread content child

Talos: not significantly better or worse. Let's fix the oranges and have more samples on that patch.

Depends on: 1636572
Attachment #9146400 - Attachment is obsolete: true

Now OnStartRequest is way faster than other sink events which is thru pHttpChannel.
I'm cooking a patch to handle this.

OnProgress/OnStatus are allowed to be sent before OnStartRequest. However, when we send
OnStartRequest through pHttpBackgroundChannel, the order won't reserve since OnProgress/OnStatus
are sent via pHttpChannel.

For performance, HttpChannelChild::ProgressOnStartRequest will call the OnProgress/OnStatus if
those events haven't received, indicated by PHttpBackgroundChannel::OnStartRequest.

Attached image Illustration of Patch 1

The yellow circle is where the patch tried to change sending OnStartRequest from PHttpChannel to PHttpBackgroundChannel
The white arrow shows we have performance gain since the ODA/OnStop is not bound by PHttpChannel (except AsyncOpen) anymore.

Attached image Illustration of Patch 2 (obsolete) —

Now OnStartRequest is sent thru pBg (behind a pref)
Therefore, OnStartRequest could be faster than OnProgress/OnStatus for upload.

Per this graph, we can see we can let our consumer receive OnProgress/OnStatus earlier and keep the order as well.

Comment on attachment 9148235 [details]
Illustration of Patch 2

After discussion with :mayhemer, we'd like to disaptch `OnProgree`/`OnStatus` through pBackground.
Attachment #9148235 - Attachment is obsolete: true

For the OnProgress/OnStatus through pHttpBackgroundChannel,
need another patch to let marti-part channel send all IPC(OnS*R, ODA) through pHttpBackgroundChannel as well (now pHttpChannel.)

Attachment #9148228 - Attachment is obsolete: true
Attachment #9145173 - Attachment description: Bug 1633935 - P1 allow OnStartRequest go thru pBg IPC → Bug 1633935 - P1 allow OnStartRequest go thru pBg IPC, r=mayhemer

We move OnStartRequest from PHttpChannel to PHttpBackgroundChannel, thus adjusting
message-metadata.ini

Depends on D76970

Depends on D76973

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0a56a68d72b542717f026072794b2a5f02def0a

Orange tests/dom/canvas/test/webgl-conf/generated/test_2_conformance__textures__image_bitmap_from_video*

Blocks: 1641336

(In reply to Junior from comment #19)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0a56a68d72b542717f026072794b2a5f02def0a

Orange tests/dom/canvas/test/webgl-conf/generated/test_2_conformance__textures__image_bitmap_from_video*

This patches works well
https://hg.mozilla.org/try/rev/637d0f99e475c4305a2847600eda1a8ee25cf5d9
in treeherder
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5d593ad3518f8bd1d338f449c6a0e89828cba74
given we don’t support options in createBitMap, we’re having the same test coverage.

Hello Ehsan,
This bug is to sending OnStartRequest via PHttpBackgroundChannel instead of PHttpChannel.
Hence, the child process will receive OnStartRequest way earlier than before.

I have a question about permission:

We're hitting this assertion after rebasing from May 22 to May 26.
https://searchfox.org/mozilla-central/rev/bc3600def806859c31b2c7ac06e3d69271052a89/extensions/permissions/PermissionManager.cpp#2207

Here is an example.
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304017626&repo=try&lineNumber=18856

My question is, should we wait for anything in child process like this to avoid this assertion?

Flags: needinfo?(ehsan)

Hello Matt,
I'm hitting this assertion for RecvRedirectToRealChannel case.
I believe it's a switch process redirect instead of 301/302/... HTTP redirect, right?

Looks like OnStartRequest happens before CompleteRedirectSetup
For document channel case, should we wait for CompleteRedirectSetup once HttpChannelChild::ConnectParent is called?
Is it possible that there's a cancel from DocumentChannel between the two?

Flags: needinfo?(matt.woodrow)

(In reply to Junior from comment #21)

Hello Ehsan,
This bug is to sending OnStartRequest via PHttpBackgroundChannel instead of PHttpChannel.
Hence, the child process will receive OnStartRequest way earlier than before.

I have a question about permission:

We're hitting this assertion after rebasing from May 22 to May 26.
https://searchfox.org/mozilla-central/rev/bc3600def806859c31b2c7ac06e3d69271052a89/extensions/permissions/PermissionManager.cpp#2207

Here is an example.
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304017626&repo=try&lineNumber=18856

My question is, should we wait for anything in child process like this to avoid this assertion?

Here's the pernosco link
https://pernos.co/debug/AjAdWj5veN999C48yQXyLQ/index.html

Attachment #9151951 - Attachment is obsolete: true
Attachment #9151952 - Attachment is obsolete: true

(In reply to Junior from comment #22)

Hello Matt,
I'm hitting this assertion for RecvRedirectToRealChannel case.
I believe it's a switch process redirect instead of 301/302/... HTTP redirect, right?

Looks like OnStartRequest happens before CompleteRedirectSetup
For document channel case, should we wait for CompleteRedirectSetup once HttpChannelChild::ConnectParent is called?
Is it possible that there's a cancel from DocumentChannel between the two?

It works to suspend EventQ in HttpChannelChild::ConnectParent and resume in HttpChannelChild::CompleteRedirectSetup`
SHould be fine even there's a cancel between them. Aborter would generate the OnStart/OnStop.

Flags: needinfo?(matt.woodrow)

(In reply to Junior from comment #23)

(In reply to Junior from comment #21)

Hello Ehsan,
This bug is to sending OnStartRequest via PHttpBackgroundChannel instead of PHttpChannel.
Hence, the child process will receive OnStartRequest way earlier than before.

I have a question about permission:

We're hitting this assertion after rebasing from May 22 to May 26.
https://searchfox.org/mozilla-central/rev/bc3600def806859c31b2c7ac06e3d69271052a89/extensions/permissions/PermissionManager.cpp#2207

Here is an example.
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304017626&repo=try&lineNumber=18856

My question is, should we wait for anything in child process like this to avoid this assertion?

Here's the pernosco link
https://pernos.co/debug/AjAdWj5veN999C48yQXyLQ/index.html

Here's my theory:
https://searchfox.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp#1404-1413

Per comment, "Send down any permissions which are relevant to this URL if we are performing a document load. " which is just happen before SendOnStartRequest.
The permission update sequence is

HttpChannelParent::OnStartRequest
ContentParent::AboutToLoadHttpFtpDocumentForChild
ContentParent::TransmitPermissionsForPrincipal
ContentParent::EnsurePermissionsByKey
PContentParent::SendSetPermissionsWithKey

However, SendSetPermissionsWithKey is pContent, which could be later than SendOnStartRequest via PHttpChannelBackgroud.
I don't have a good solution at this moment. Ideally we should bring the permission information through SendOnStartRequest to update the Permission manager in content process.

Keep ni? to see if Ehsan has some idea.

Let's wait for the PContent::SendSetPermissionsWithKey given lots of orange are still there and do the optimization later.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a275586f4e5258e255d7081149ed4d14a2a3e02

Flags: needinfo?(ehsan)
HttpChannelParent::OnStartRequest
ContentParent::AboutToLoadHttpFtpDocumentForChild
ContentParent::TransmitPermissionsForPrincipal
ContentParent::EnsurePermissionsByKey
PContentParent::SendSetPermissionsWithKey

However, SendSetPermissionsWithKey is pContent, which could be later than SendOnStartRequest via PHttpChannelBackgroud.
I don't have a good solution at this moment. Ideally we should bring the permission information through SendOnStartRequest to update the Permission manager in content process.

Similiar situation with cookie

ContentParent::AboutToLoadHttpFtpDocumentForChild
ContentParent::UpdateCookieStatus
CookieServiceParent::TrackCookieLoad
PCookieServiceParent::SendTrackCookiesLoad

Therefore, let's wait for another main thread event for a document load.

However, what's worse is Set-Cookie response header is another PContent IPC

nsHttpChannel::ContinueProcessResponse1
HttpBaseChannel::SetCookie
HttpBaseChannel::NotifySetCookie
CookieStorage::AddCookie
CookieStorage::NotifyChange("cookie-changed")
ContentParent::Observe
CookieServiceParent::AddCookie
PCookieServiceParent::SendAddCookie

Would need a decent amount of work to OMT Set-Cookie

Given options in createImageBitmap is not supported, the test coverage is the same.

Depends on D76973

Attachment #9153317 - Attachment description: Bug 1633935 - P10 wait PHttpChannel::OnStartRequestSent for main document and Set-Cookie response, r=mayhemer → Bug 1633935 - P10 wait PHttpChannel::OnStartRequestSent for permission/cookie update from parent, r=mayhemer

ni? for asking review in comment 28

Flags: needinfo?(jgilbert)
Flags: needinfo?(jgilbert)
Depends on: 1645527

Bug 1015466 move them from PContent to Pbg since it believes those events are fine
to received before OnStopRequest. Bug 1588241 move it back to PContent since those
events are before OnStartRequest.

Now we're moving OnStartRequest to pBg, so here's another ping-pong time :)

Depends on D77752

Depends on: 1645941

The WebRequest will suspend the channel in nsHttpChannel::PrepareToConnect(), apply the stream filter, send the main thread IPC and resume.
That is, PHttpChannel::AttachStreamFilter should be handled in Child before OnStartRequest goes to listener.

Depends on D79587

One line fix with a bunch of explanation.

When we perform networking on the socket process, OnDataAvailable could be sent directly from socket process to child process.
Therefore, it could be racy with OnStartRequest which is from parent process to child process by nature.

This patch is to resolve the failure (cancel) by parent process.
For example, https://searchfox.org/mozilla-central/rev/027893497316897b8f292bde48dbb6da2391a331/netwerk/protocol/http/nsHttpChannel.cpp#2713

Bug 1641496 introduce a status check to avoid ODA message being process
[2] https://searchfox.org/mozilla-central/rev/027893497316897b8f292bde48dbb6da2391a331/netwerk/protocol/http/HttpBackgroundChannelChild.cpp#163-171

It prevents putting ODA to the channel event queue in child process.

However, this is based on that OnStartRequest already finished excution, which is usually true if OnStartRequest is sent via PHttpChannel.
https://searchfox.org/mozilla-central/rev/027893497316897b8f292bde48dbb6da2391a331/netwerk/protocol/http/HttpChannelChild.cpp#417-422
ChannelEventQueue::RunOrEnque simply runs the OnStartRequest given there on the same thread and the queue is empty, since no blocking events from different thread are before OnStartRequest.

Now we are moving the next chapter. OnStartRequest is queued given we want to switch thread.
The failure status, which is initiated in parent process, is set in HttpChannelChild::OnStartRequest which could be later than the status check [2]

Therefore, we need another check to prevent ODA, which is already in the event queue, being processed.
We intentionally keep [2] for performance.

Depends on D79770

Depends on: 1645254

Unfortunately, we force the main thread IPC for responses with Set-Cookie, so we won't have a good impact on talos.
Bug 1645901 is the key follow-up to lift the restriction.

Pushed by juhsu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/19dcca77a1cf
P1 allow OnStartRequest go thru pBg IPC, r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/df5abc2c0734
P2 remove OnStartRequestSent since OnStartRequest is passed thru pBg, r=mayhemer,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/be03e0e7b645
P3 send OS*R and ODA via pHttpChannel for multipart channel, r=mayhemer,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/2a9873ad6856
P4 Remove On[Start|Stop]Request/OnTransportAndData in PHttpChannel, r=mayhemer,nika,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/73b51a139c91
P5 allow OnProgress/OnStatus going thru pBg IPC, r=mayhemer,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/6251c22cc8f1
P6 Skip one media track failure caused by performance gain, r=alwu
https://hg.mozilla.org/integration/autoland/rev/292a5b34e9f7
P7 refactor bitmap test, r=jgilbert
https://hg.mozilla.org/integration/autoland/rev/ac9c5c1e5162
P8 suspend event queue in HttpChannelChild which needs listener for redirection, r=mayhemer,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/3b268ac50692
P9 send OnAfterLastPart via pBg IPC to avoid race between OS*R, r=mayhemer,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/709f8d50aa33
P10 wait PHttpChannel::OnStartRequestSent for permission/cookie update from parent, r=mayhemer,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/e87b11c172b9
P11 Move classification IPC methods back to PHttpChannel. r=mayhemer,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/25822279ba5b
P12 top-level IPC stream filter should happen before OnStartRequest, r=mayhemer,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/398fc19f5f6a
P13 Avoid ODA message being processed when the channel is already failed, r=mayhemer,kershaw,necko-reviewers
Regressed by: 1648035
Regressions: 1648132
No longer regressed by: 1648035
Regressions: 1648035

We decide to backout and land it back in the beginning of next cycle.

For the performance impact:
this is a comparison without cookie and stream filter constraint, i.e., the best case after solving Bug 1645901 and Bug 1646592
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=f6c83e4b25dc29a1ded4cc10ab895c81c3dc9c0d&framework=1&selectedTimeRange=172800

this is comparison with autoland in comment 38. The result should come up hours later.
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=autoland&newRevision=398fc19f5f6adf0a58ddb43e2efbaac571db320b&framework=1&selectedTimeRange=172800

Here's the reason I don't put this behind a pref even it could be a risky patch.
(a) it's not blocked by anything
(b) code complexity. Before the patch:

  • normal channels send OnStart via PHttpChannel and ODA/OnStop via PHttpBackgroundChannel
    multipart channels send everything (OS*R, ODA) thru PHttpChannel.

If we add a pref, we will have another code path for sending everything via PBg.

  • What worse, lots of dependent IPC should happen before OnStart and the order is very important.
    That includes
  1. OnProgress/OnStatus
  2. OnAfterLastPart for multipart channel
  3. These notification for uri classifier
    https://searchfox.org/mozilla-central/rev/cfaa250d14e344834932de4c2eed0061701654da/netwerk/protocol/http/PHttpBackgroundChannel.ipdl#63-74

If we have a pref, we should send them in both PHttpChannel and PBg, depending on the pref.

  • What's even worse, we would maintain two types of OnStartRequestSent to synchronized another part of IPC sequences.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by juhsu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b8f55064bfeb
P1 allow OnStartRequest go thru pBg IPC, r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/cbcd3ee87d28
P2 remove OnStartRequestSent since OnStartRequest is passed thru pBg, r=mayhemer,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/d59b42f58f86
P3 send OS*R and ODA via pHttpChannel for multipart channel, r=mayhemer,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/e6c12eec1cef
P4 Remove On[Start|Stop]Request/OnTransportAndData in PHttpChannel, r=mayhemer,nika,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/4214217c588c
P5 allow OnProgress/OnStatus going thru pBg IPC, r=mayhemer,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/10382bbd8c8f
P6 Skip one media track failure caused by performance gain, r=alwu
https://hg.mozilla.org/integration/autoland/rev/b1cdccfb383d
P7 refactor bitmap test, r=jgilbert
https://hg.mozilla.org/integration/autoland/rev/045271cd2ca6
P8 suspend event queue in HttpChannelChild which needs listener for redirection, r=mayhemer,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/673030046319
P9 send OnAfterLastPart via pBg IPC to avoid race between OS*R, r=mayhemer,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/2289e685031e
P10 wait PHttpChannel::OnStartRequestSent for permission/cookie update from parent, r=mayhemer,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/f75cc7d5f40e
P11 Move classification IPC methods back to PHttpChannel. r=mayhemer,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/cad764eb15a4
P12 top-level IPC stream filter should happen before OnStartRequest, r=mayhemer,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/d529ccb6638b
P13 Avoid ODA message being processed when the channel is already failed, r=mayhemer,kershaw,necko-reviewers

== Change summary for alert #26387 (as of Tue, 30 Jun 2020 17:06:31 GMT) ==

Improvements:

3% Heap Unclassified macosx1014-64-shippable opt tp6 126,126,554.64 -> 121,879,789.89

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=26387

Keywords: perf-alert

== Change summary for alert #26397 (as of Wed, 01 Jul 2020 11:30:14 GMT) ==

Improvements:

21% espn fcp android-hw-g5-7-0-arm7-api-16-shippable opt cold 7,059.58 -> 5,592.17
7% espn PerceptualSpeedIndex android-hw-g5-7-0-arm7-api-16-shippable opt cold 6,941.92 -> 6,422.58
7% espn android-hw-g5-7-0-arm7-api-16-shippable opt cold 3,309.02 -> 3,081.74
7% espn SpeedIndex android-hw-g5-7-0-arm7-api-16-shippable opt cold 6,875.46 -> 6,411.25

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=26397

You need to log in before you can comment on or make changes to this bug.