Migrate IPCStream to use DataPipe
Categories
(Core :: IPC, enhancement)
Tracking
()
Tracking | Status | |
---|---|---|
firefox102 | --- | fixed |
People
(Reporter: nika, Assigned: nika)
References
(Blocks 1 open bug, Regressed 1 open bug)
Details
(Keywords: perf-alert)
Attachments
(18 files)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
This should have a few benefits, including:
- Data transfer of piped data won't occur on the sending thread (e.g. main thread), meaning it won't block normal IPC and can occur faster.
- In situations which support it, it will be possible to apply backpressure to the streamed data.
- Data is transferred through a shared memory ring buffer, avoiding the large data streams using up message pipe capacity.
- Serializing IPCStream will be possible on any actor or over any port as it won't require IPDL actors anymore.
Assignee | ||
Comment 1•3 years ago
|
||
Adding bug 1647892 as a dependency because my local patches for these changes speed up form submission enough that the test now permafails in opt builds.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 2•3 years ago
|
||
This gives us various positive benefits, such as using a shared memory ring
buffer for faster communication, not having data streaming being bound to the
thread which transferred the nsIInputStream (which is often the main thread),
and the ability for some backpressure to be applied to data streaming.
After this change, the "delayed start" parameter for IPCStream serialization is
less relevant, as backpressure will serve a similar purpose. It will still be
used to determine whether or not to use RemoteLazyInputStream when serializing
from the parent process.
Depends on D141037
Assignee | ||
Comment 3•3 years ago
|
||
This interface should no longer be required due to the changes in part 1
limiting the complexity of IPCStream instances and limiting the number of file
descriptors which a single stream can attach to a message.
Removing this interface is necessary to serialize nsIInputStream instances over
arbitrary toplevel protocols and non-protocol IPC in the future.
Depends on D141038
Assignee | ||
Comment 4•3 years ago
|
||
This is a complete rewrite of RemoteLazyInputStream to run off of its own
toplevel protocol, rather than being managed by other protocols like
PBackground or PContent. This should improve performance thanks to no longer
needing to operate on a main or worker thread, and due to no longer needing the
migration step for the stream actor.
This also acts as a step towards no longer requiring a manager actor to
serialize input streams, as the type is now actor-agnostic, and should support
being sent over IPC between any pair of processes.
Depends on D141039
Assignee | ||
Comment 5•3 years ago
|
||
This type was originally made to be required to be seekable because a
non-seekable stream would lose the nsMIMEInputStream metadata information when
it was wrapped by the HTTP channel using PartiallySeekableInputStream, leading
to potential bugs.
Changes in a later part will change HTTP channel wrapping to ensure that this
metadata is preserved in all relevant cases, so this requirement can be
relaxed. This is important, as the check was previously being bypassed for
RemoteLazyInputStream in the content process, as nsBufferedInputStream would
incorrectly report that it implemented nsISeekableStream even if the underlying
stream wasn't seekable.
Depends on D141040
Assignee | ||
Comment 6•3 years ago
|
||
Previously, nsBufferedStreams would always implement nsISeekableStream, whether
or not the underlying stream did, and would only fail when trying to seek. This
makes it much more difficult to compensate and correctly transform streams when
they need to be seekable.
Depends on D141041
Assignee | ||
Comment 7•3 years ago
|
||
the nsIMIMEInputStream type contains extra metadata header information
which shouldn't be lost when serializing the type over IPC. This patch
changes the LazyStream serialization to take this into account and only
serialize the value within the nsMIMEInputStream when sending a lazy
stream over IPC.
This information is specifically used by HTTP channels in order to
populate POST request headers.
Depends on D141042
Assignee | ||
Comment 8•3 years ago
|
||
Unfortunately, upload streams used by necko have various odd behaviours
and requirements which happened to be usually preserved by the previous
IPC serialization logic, but were not consistently preserved. This
includes requiring the stream to be synchronous (as some consumers such
as WebExtensions and DevTools appear to read it assuming Available() is
the stream length), seekable (as it needs to be rewound in various
places), and cloneable (as the stream information is often handed out to
other components).
In addition, the WebExtension WebRequest code makes assumptions about
the specific topology of the input stream for optimization purposes,
meaning that nsMultiplexInputStreams need to be preserved.
The way this was previously handled was by copying the entire payload
into a nsStorageStream as an async operation. This happened very
infrequently in out test suite, however, and had some issues. It could
lead to data loss if the stream was a nsMIMEInputStream (as the metadata
would be lost), and would destroy the topology required by WebRequest.
This patch changes the code to instead manually walk and replace streams
in the input stream's data structure, to efficiently copy only the
required data, preserve the invariants, and make the type seekable
before AsyncOpen continues. This helps keep the complexity of the
invariants HTTPChannel depends on out of generic input stream handling
code.
In addition, due to how early this happens, it replaces the need for
PartiallySeekableInputStream which will be removed a later part.
Depends on D141043
Assignee | ||
Comment 9•3 years ago
|
||
This type is no longer necessary, and has various issues due to behaving
incorrectly when used with async streams or streams which are not nsIPipe.
Depends on D141044
Assignee | ||
Comment 10•3 years ago
|
||
This interface is misleading, as it doesn't allow seeking the entire type
despite providing a nsISeekableStream
interface, and is no longer necessary
due to the changes in an earlier part.
Depends on D141045
Assignee | ||
Comment 11•3 years ago
|
||
Previously the SessionHistoryInfo would hold onto and hand out the
original nsIInputStream objects which were provided by the
nsDocShellLoadState and used to create the underlying channel. This
could cause issues in edge cases, as input streams when serialized over
IPC have their logical owner transferred to the IPC layer so that it can
copy the data to the peer process.
This patch changes the logic to instead clone the input stream to and
from the history info. This means that the history info has its own
instance of the stream type and interacting with it shouldn't interfere
with other consumers of the post data stream.
The behaviour for non-SHIP session history is not changed, as it doesn't
serialize the relevant streams over IPC in the same way, and is on track to be
removed.
Depends on D141046
Assignee | ||
Comment 12•3 years ago
|
||
As serializing IPCStream no longer requires a manager or FileDescriptor array,
the arguments are no longer necessary, and can be removed. The AutoIPCStream
helper can also be removed, as managed actors are no longer used for
serialization, so a delayed start callback is not necessary.
The delayed start parameter is also removed from nsIIPCSerializableInputStream
instances, but is still present as aAllowLazy
on the toplevel serialization
methods.
Depends on D141047
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 13•3 years ago
|
||
Previously this test was failing due to issues with upload streams not
being seekable. With the other changes in this bug, we now always
ensure that upload streams are consistently cloneable and seekable,
meaning that this issue should no longer occur, and we don't need to
re-start the request for it to succeed.
I also removed various pieces of dead code, as well as some logic
disabling bfcacheInParent, as that seems to no longer be required.
Depends on D141048
Comment 14•3 years ago
|
||
I searchfox indexed yesterday's try push of https://treeherder.mozilla.org/jobs?repo=try&revision=ec4eb04206d7e583095de286c3fc4b8714ca098c and it's available at:
https://streams.searchfox.org/mozilla-central/source/
Assignee | ||
Comment 15•3 years ago
|
||
Previously, the check when reporting progress from a nsHttpTransaction would
always report no progress when a non-seekable input stream is used as the
request data stream. Before Part 5, we incorrectly always reported the
nsBufferedStream which we wrap request streams with as seekable, meaning that
this check would pass and the progress reporting would work.
This change relaxes the check to instead check for nsITellableStream which is
actually guaranteed by the nsBufferedStream wrapper, and provides the Tell
method being used.
Depends on D141777
Assignee | ||
Comment 16•3 years ago
|
||
Before this change, we could run into issues when an async stream
accepts new data and queues a runnable to notify the stream listener,
but before that stream listener's runnable is run, the target thread
reads data from the async stream, emptying it of any available data.
This would mean that the stream appears empty in
nsMultiplexInputStream::OnInputStreamReady, and the rest of the stream
is skipped.
This patch changes the logic in OnInputStreamReady to re-call AsyncWait
in those situations and avoid skipping over the empty stream, preventing
data loss.
This situation came up when running some HTTP tests under rr with the
socket process enabled, as the upload stream can both be read due to an
OnInputStreamReady callback and due to other calls in the HTTP
machinery, leading to this potential race. With the socket process
enabled, it is possible for the upload stream to be an async pipe, which
doesn't happen in the parent process due to stream normalization.
This change makes an assumption about the behaviour of streams
implementing nsIAsyncInputStream
which I believe is correct right now,
and has been documented. I originally had other patches which modified
Available()
's definition to add an extra outparameter or added extra
methods, but this seemed much simpler and accurate based on my memory of
having read the Available() implementations for async streams in-tree.
A gtest was added for the failing situation using both nsPipe and DataPipe.
Depends on D144450
Comment 17•3 years ago
|
||
Comment 18•3 years ago
|
||
Backed out for causing wpt failures.
Backout link: https://hg.mozilla.org/integration/autoland/rev/31b84a1b36f6b81d4dc24fe88c57322febde21c6
Failure log:
https://treeherder.mozilla.org/logviewer?job_id=376554093&repo=autoland&lineNumber=6920
https://treeherder.mozilla.org/logviewer?job_id=376554055&repo=autoland&lineNumber=3296
Assignee | ||
Comment 19•3 years ago
|
||
As discovered by TSan, this member could be raced on in some edge-cases if
cloned pipe streams raced on multiple threads to be destroyed. This change
instead moves the nsIPipe
implementation to be on a seperate type from
nsPipe
, allowing mOriginalInput
to no longer need to be tracked.
This technically has slightly different behaviour than the previous
implementation for JS code, as code keeping the nsIPipe
alive will now also
be keeping the output stream reference alive directly, rather than only holding
the nsIPipe
, meaning that pipe.outputStream
can be read multiple times
independently without implicitly closing the underlying stream. Based on a
quick read of the few remaining uses of nsIPipe
in JS code, I don't think
this will be an issue, and it may actually be more intuitive and consistent.
Depends on D142127
Assignee | ||
Comment 20•3 years ago
|
||
These tests would create 100 blob URIs in a loop, meaning that the next time a
process was started, we'd need to lock over 64 mutexes at once in order to
transfer all of the ports implementing those blobs to the other process,
crashing TSan due to it's maximum lock limit.
See bug 1767483 for more information on the TSan issue this is working around.
Depends on D145362
Assignee | ||
Updated•3 years ago
|
Comment 21•3 years ago
|
||
Comment 22•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2059c2d0d880
https://hg.mozilla.org/mozilla-central/rev/6d8e51b3e8d7
https://hg.mozilla.org/mozilla-central/rev/0a6cf0817bf5
https://hg.mozilla.org/mozilla-central/rev/82b38c12b298
https://hg.mozilla.org/mozilla-central/rev/e40e810e18fc
https://hg.mozilla.org/mozilla-central/rev/6802a4326963
https://hg.mozilla.org/mozilla-central/rev/35a69828091c
https://hg.mozilla.org/mozilla-central/rev/d95e7ad0eafa
https://hg.mozilla.org/mozilla-central/rev/51bd50b57dd5
https://hg.mozilla.org/mozilla-central/rev/c4f1e86992b6
https://hg.mozilla.org/mozilla-central/rev/234ff9e092c2
https://hg.mozilla.org/mozilla-central/rev/9018dd592230
https://hg.mozilla.org/mozilla-central/rev/a147df47a0e3
https://hg.mozilla.org/mozilla-central/rev/96a39c2ba7a3
https://hg.mozilla.org/mozilla-central/rev/017e1552d549
https://hg.mozilla.org/mozilla-central/rev/63f17a06eba9
Comment 23•3 years ago
|
||
Backed out for causing crashes e.g. Bug 1767808, and hanging Gmail (Bug 1767918)
https://hg.mozilla.org/mozilla-central/rev/b6dca56b56ccf7002c5a222a109898a3c32c82a8
Assignee | ||
Comment 24•3 years ago
|
||
If we don't do this, we can encounter issues where we'll spuriously close the
stream when the last reference to the input stream is dropped while an
AsyncWait is still pending.
Depends on D145363
Comment 25•3 years ago
|
||
Comment 26•3 years ago
|
||
Backed out 22 changesets (Bug 1759572, Bug 1759563, Bug 1754004, Bug 1754031, Bug 1759569, Bug 1696894) for causing multiple failures CLOSED TREE
Log: https://treeherder.mozilla.org/logviewer?job_id=377070975&repo=autoland&lineNumber=3645
https://treeherder.mozilla.org/logviewer?job_id=377069392&repo=autoland&lineNumber=3841
https://treeherder.mozilla.org/logviewer?job_id=377068595&repo=autoland&lineNumber=4833
https://treeherder.mozilla.org/logviewer?job_id=377071130&repo=autoland&lineNumber=2287
https://treeherder.mozilla.org/logviewer?job_id=377070617&repo=autoland&lineNumber=8173
https://treeherder.mozilla.org/logviewer?job_id=377068816&repo=autoland&lineNumber=37681
https://treeherder.mozilla.org/logviewer?job_id=377070241&repo=autoland&lineNumber=9079
Backout: https://hg.mozilla.org/integration/autoland/rev/36e3802d69934bed836ce1131d2de0fcb97984ae
Assignee | ||
Updated•3 years ago
|
Comment 27•3 years ago
|
||
(In reply to Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout) from comment #23)
Backed out for causing crashes e.g. Bug 1767808, and hanging Gmail (Bug 1767918)
https://hg.mozilla.org/mozilla-central/rev/b6dca56b56ccf7002c5a222a109898a3c32c82a8
== Change summary for alert #34077 (as of Sat, 07 May 2022 09:28:05 GMT) ==
Regressions:
Ratio | Test | Platform | Options | Absolute values (old vs new) |
---|---|---|---|---|
0.09% | installer size | osx-shippable | instrumented | 119,141,021.38 -> 119,250,088.83 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=34077
Comment 28•3 years ago
|
||
(In reply to Pulsebot from comment #25)
Pushed by nlayzell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/10c3683b998b
Part 1: Switch IPCStream to use DataPipe instead of
P{ChildToParent,ParentToChild}Stream, r=asuth,necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/0ca1856b5fc9
Part 2: Remove the PFileDescriptorSet interface,
r=asuth,necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/000fb99de0cf
Part 3: Move RemoteLazyInputStream to its own toplevel protocol,
r=asuth,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/44dfd76472aa
Part 4: Stop requiring nsMIMEInputStream to be seekable,
r=asuth,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/2374420d9fed
Part 5: Only implement nsISeekableStream on nsBufferedStreams if it's
implemented by the underlying stream, r=asuth,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/c6388d682d48
Part 6: Preserve MIME information when serializing a lazy stream,
r=asuth,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/fe90db9158ac
Part 7: Consistently normalize upload streams passed to HTTP channels,
r=asuth,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/e515b7edd1c8
Part 8: Remove SeekableStreamWrapper, r=asuth,necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/0f480fc861c0
Part 9: Remove PartiallySeekableInputStream, r=asuth,necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/e8649de9dd22
Part 10: Clone PostData when writing into & reading from SessionHistoryInfo,
r=smaug
https://hg.mozilla.org/integration/autoland/rev/55b4b0c9a504
Part 11: Simplify the IPCStream serialization API,
r=asuth,necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/cebe8d5dda34
Part 12: Update browser_post_auth.js assertions, r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/dfabce587a56
Part 13: Relax interface check in nsHttpTransaction,
r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/b08475bbfbd9
Part 14: Fix potential skipping of async streams in nsMultiplexInputStream,
r=asuth
https://hg.mozilla.org/integration/autoland/rev/648e3dd4b62f
Part 15: Don't track mOriginalInput in nsPipe, r=asuth
https://hg.mozilla.org/integration/autoland/rev/09fc506865d7
Part 16: Disable tests which create too many blob URIs with TSan, r=asuth
https://hg.mozilla.org/integration/autoland/rev/d69647a725a1
Part 17: Keep pipe streams alive so long as there's a callback registered,
r=asuth
== Change summary for alert #34050 (as of Thu, 05 May 2022 11:18:13 GMT) ==
Improvements:
Ratio | Test | Platform | Options | Absolute values (old vs new) |
---|---|---|---|---|
28% | pinterest ContentfulSpeedIndex | windows10-64-shippable-qr | fission warm webrender | 1,164.71 -> 843.58 |
20% | pinterest SpeedIndex | windows10-64-shippable-qr | fission warm webrender | 929.00 -> 740.42 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=34050
Updated•3 years ago
|
Comment 29•3 years ago
|
||
(In reply to Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout) from comment #23)
Backed out for causing crashes e.g. Bug 1767808, and hanging Gmail (Bug 1767918)
https://hg.mozilla.org/mozilla-central/rev/b6dca56b56ccf7002c5a222a109898a3c32c82a8
== Change summary for alert #34080 (as of Sat, 07 May 2022 15:08:01 GMT) ==
Improvements:
Ratio | Test | Platform | Options | Absolute values (old vs new) |
---|---|---|---|---|
5% | pinterest PerceptualSpeedIndex | windows10-64-shippable-qr | fission warm webrender | 1,862.75 -> 1,776.00 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=34080
Comment 30•3 years ago
|
||
Comment 31•3 years ago
|
||
Backed out 22 changesets (Bug 1696894, Bug 1759569, Bug 1754031, Bug 1759563, Bug 1759572, Bug 1754004) for causing leack failures CLOSED TREE
Log: https://treeherder.mozilla.org/logviewer?job_id=377632893&repo=autoland&lineNumber=7392
https://treeherder.mozilla.org/logviewer?job_id=377629411&repo=autoland&lineNumber=32257
Backout: https://hg.mozilla.org/integration/autoland/rev/ce1bb7d083b9428f4a0985216baddd883c54e62a
Assignee | ||
Comment 32•3 years ago
|
||
When the NS_AsyncCopy completes, there may still be outstanding
AsyncWait callbacks which have not been invoked yet, due to two
AsyncWait callbacks being registered each time Process() yields (one to
wait for the blocked stream, and the other with WAIT_CLOSURE_ONLY to
handle errors), and only one callback being needed to resume processing.
This change ensures that any outstanding AsyncWait callbacks are
cancelled, breaking references from the streams to the
nsAStreamCallback. Some streams (such as nsPipe and DataPipe) may also
leak if there are oustanding AsyncWait callbacks, due to the need to
keep the stream alive so it can be passed into the callback, which this
helps avoid.
Previously leaks were largely avoided due to the call to Close()
cancelling the callbacks for us, however not all callsites specify to
close the source/sink.
This should also help avoid an unnecessary dispatch after the copy
completes due to our AsyncWait callback being invoked when the
source/sink stream is closed.
Depends on D145672
Comment 33•3 years ago
|
||
Assignee | ||
Updated•3 years ago
|
Comment 34•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2660731a9aea
https://hg.mozilla.org/mozilla-central/rev/c4f201a829c9
https://hg.mozilla.org/mozilla-central/rev/c3d8d84d01b9
https://hg.mozilla.org/mozilla-central/rev/d07a20fa2794
https://hg.mozilla.org/mozilla-central/rev/a546742d3f11
https://hg.mozilla.org/mozilla-central/rev/e54aff74d256
https://hg.mozilla.org/mozilla-central/rev/0b2df3479481
https://hg.mozilla.org/mozilla-central/rev/249922740433
https://hg.mozilla.org/mozilla-central/rev/4578d30460cb
https://hg.mozilla.org/mozilla-central/rev/185b61fd584a
https://hg.mozilla.org/mozilla-central/rev/baa047853223
https://hg.mozilla.org/mozilla-central/rev/08e8e64cf522
https://hg.mozilla.org/mozilla-central/rev/6396c9396eb7
https://hg.mozilla.org/mozilla-central/rev/6efd4589b337
https://hg.mozilla.org/mozilla-central/rev/8de4bb94de16
https://hg.mozilla.org/mozilla-central/rev/4f0ce6d040be
https://hg.mozilla.org/mozilla-central/rev/c1bb4f38919f
https://hg.mozilla.org/mozilla-central/rev/3f4ebef6a4b1
Comment 35•2 years ago
|
||
A weird problem with URL.createObjectURL() seems to happen now from time to time - namely in multithreaded-download-manager extension.
More details here: https://github.com/jingyu9575/multithreaded-download-manager/issues/179
Description
•