Allow sending onStartRequest via pHttpBackgroundChannel
Categories
(Core :: Networking: HTTP, enhancement, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox80 | --- | fixed |
People
(Reporter: CuveeHsu, Assigned: CuveeHsu)
References
(Depends on 1 open bug, Blocks 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 |
Assignee | ||
Comment 1•4 years ago
|
||
Assignee | ||
Comment 2•4 years ago
|
||
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
Assignee | ||
Comment 3•4 years ago
|
||
Assignee | ||
Comment 4•4 years ago
|
||
Comment hidden (obsolete) |
Updated•4 years ago
|
Assignee | ||
Comment 6•4 years ago
|
||
Now OnStartRequest is way faster than other sink events which is thru pHttpChannel.
I'm cooking a patch to handle this.
Assignee | ||
Comment 7•4 years ago
|
||
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
.
Assignee | ||
Comment 8•4 years ago
•
|
||
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.
Assignee | ||
Comment 9•4 years ago
|
||
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.
Assignee | ||
Comment 10•4 years ago
|
||
Comment on attachment 9148235 [details]
Illustration of Patch 2
After discussion with :mayhemer, we'd like to disaptch `OnProgree`/`OnStatus` through pBackground.
Assignee | ||
Comment 11•4 years ago
|
||
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.)
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 12•4 years ago
|
||
Depends on D73529
Assignee | ||
Comment 13•4 years ago
|
||
Depends on D76969
Assignee | ||
Comment 14•4 years ago
|
||
We move OnStartRequest from PHttpChannel to PHttpBackgroundChannel, thus adjusting
message-metadata.ini
Depends on D76970
Assignee | ||
Comment 15•4 years ago
|
||
Depends on D76971
Assignee | ||
Comment 16•4 years ago
|
||
Depends on D76972
Assignee | ||
Comment 17•4 years ago
|
||
Depends on D76973
Assignee | ||
Comment 18•4 years ago
|
||
Depends on D76974
Assignee | ||
Comment 19•4 years ago
|
||
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*
Assignee | ||
Comment 20•4 years ago
|
||
(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.
Assignee | ||
Comment 21•4 years ago
|
||
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?
Assignee | ||
Comment 22•4 years ago
|
||
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?
Assignee | ||
Comment 23•4 years ago
|
||
(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#2207Here is an example.
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304017626&repo=try&lineNumber=18856My 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
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 24•4 years ago
|
||
(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.
Assignee | ||
Comment 25•4 years ago
|
||
(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#2207Here is an example.
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304017626&repo=try&lineNumber=18856My 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.
Assignee | ||
Comment 26•4 years ago
•
|
||
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
Assignee | ||
Comment 27•4 years ago
•
|
||
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
Assignee | ||
Comment 28•4 years ago
|
||
Given options in createImageBitmap is not supported, the test coverage is the same.
Depends on D76973
Assignee | ||
Comment 29•4 years ago
|
||
Depends on D77749
Assignee | ||
Comment 30•4 years ago
|
||
Depends on D77750
Assignee | ||
Comment 31•4 years ago
|
||
Depends on D77751
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 33•4 years ago
|
||
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
Assignee | ||
Comment 34•4 years ago
|
||
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
Assignee | ||
Comment 35•4 years ago
|
||
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
Assignee | ||
Comment 36•4 years ago
|
||
Assignee | ||
Comment 37•4 years ago
|
||
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.
Comment 38•4 years ago
|
||
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
Comment 39•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/19dcca77a1cf
https://hg.mozilla.org/mozilla-central/rev/df5abc2c0734
https://hg.mozilla.org/mozilla-central/rev/be03e0e7b645
https://hg.mozilla.org/mozilla-central/rev/2a9873ad6856
https://hg.mozilla.org/mozilla-central/rev/73b51a139c91
https://hg.mozilla.org/mozilla-central/rev/6251c22cc8f1
https://hg.mozilla.org/mozilla-central/rev/292a5b34e9f7
https://hg.mozilla.org/mozilla-central/rev/ac9c5c1e5162
https://hg.mozilla.org/mozilla-central/rev/3b268ac50692
https://hg.mozilla.org/mozilla-central/rev/709f8d50aa33
https://hg.mozilla.org/mozilla-central/rev/e87b11c172b9
https://hg.mozilla.org/mozilla-central/rev/25822279ba5b
https://hg.mozilla.org/mozilla-central/rev/398fc19f5f6a
Assignee | ||
Updated•4 years ago
|
Comment 40•4 years ago
|
||
Backed out as requested.
Backout link: https://hg.mozilla.org/integration/autoland/rev/9d89fc475692b20011929c1dc8bb8f5ff8fe8809
Assignee | ||
Comment 41•4 years ago
|
||
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
Assignee | ||
Comment 42•4 years ago
|
||
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
- OnProgress/OnStatus
- OnAfterLastPart for multipart channel
- 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.
Updated•4 years ago
|
Comment 43•4 years ago
|
||
Backout merged: https://hg.mozilla.org/mozilla-central/rev/9d89fc475692
Comment 44•4 years ago
|
||
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
Comment 45•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b8f55064bfeb
https://hg.mozilla.org/mozilla-central/rev/cbcd3ee87d28
https://hg.mozilla.org/mozilla-central/rev/d59b42f58f86
https://hg.mozilla.org/mozilla-central/rev/e6c12eec1cef
https://hg.mozilla.org/mozilla-central/rev/4214217c588c
https://hg.mozilla.org/mozilla-central/rev/10382bbd8c8f
https://hg.mozilla.org/mozilla-central/rev/b1cdccfb383d
https://hg.mozilla.org/mozilla-central/rev/045271cd2ca6
https://hg.mozilla.org/mozilla-central/rev/673030046319
https://hg.mozilla.org/mozilla-central/rev/2289e685031e
https://hg.mozilla.org/mozilla-central/rev/f75cc7d5f40e
https://hg.mozilla.org/mozilla-central/rev/cad764eb15a4
https://hg.mozilla.org/mozilla-central/rev/d529ccb6638b
Comment 46•4 years ago
|
||
== 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
Comment 47•4 years ago
|
||
== 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
Description
•