Closed Bug 1754004 Opened 3 years ago Closed 3 years ago

Migrate IPCStream to use DataPipe

Categories

(Core :: IPC, enhancement)

Product:
Core
Component:
IPC
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
102 Branch
Tracking Flags:
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)

Bug 1754004 - Part 1: Switch IPCStream to use DataPipe instead of P{ChildToParent,ParentToChild}Stream, r=asuth!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1754004 - Part 2: Remove the PFileDescriptorSet interface, r=asuth!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1754004 - Part 3: Move RemoteLazyInputStream to its own toplevel protocol, r=asuth!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1754004 - Part 4: Stop requiring nsMIMEInputStream to be seekable, r=asuth!,#necko-reviewers!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1754004 - Part 5: Only implement nsISeekableStream on nsBufferedStreams if it's implemented by the underlying stream, r=asuth!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1754004 - Part 6: Preserve MIME information when serializing a lazy stream, r=asuth!,#necko-reviewers!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1754004 - Part 7: Consistently normalize upload streams passed to HTTP channels, r=asuth!,#necko-reviewers!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1754004 - Part 8: Remove SeekableStreamWrapper, r=asuth!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1754004 - Part 9: Remove PartiallySeekableInputStream, r=asuth!,#necko-reviewers!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1754004 - Part 10: Clone PostData when writing into & reading from SessionHistoryInfo, r=smaug!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1754004 - Part 11: Simplify the IPCStream serialization API, r=asuth!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1754004 - Part 12: Update browser_post_auth.js assertions, r=#necko-reviewers!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1754004 - Part 13: Relax interface check in nsHttpTransaction, r=#necko-reviewers!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1754004 - Part 14: Fix potential skipping of async streams in nsMultiplexInputStream, r=asuth!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1754004 - Part 15: Don't track mOriginalInput in nsPipe, r=asuth!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1754004 - Part 16: Disable tests which create too many blob URIs with TSan, r=asuth!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1754004 - Part 17: Keep pipe streams alive so long as there's a callback registered, r=asuth!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1754004 - Part 18: Ensure AsyncWait callbacks are cleared when NS_AsyncCopy completes, r=#xpcom-reviewers
48 bytes, text/x-phabricator-request
Details | Review

This should have a few benefits, including:

  1. 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.
  2. In situations which support it, it will be possible to apply backpressure to the streamed data.
  3. Data is transferred through a shared memory ring buffer, avoiding the large data streams using up message pipe capacity.
  4. Serializing IPCStream will be possible on any actor or over any port as it won't require IPDL actors anymore.

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.

Depends on: 1647892
Depends on: 1754017
Depends on: 1754031
See Also: → 1754031
See Also: 1754031 →
Depends on: 1759563
Depends on: 1759569
Depends on: 1759572
Depends on: 1759609

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

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

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

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

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

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

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

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

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

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

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

See Also: → 1696386

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

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

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

Pushed by nlayzell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a3f3f9761ada 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/df5ce1f6712c Part 2: Remove the PFileDescriptorSet interface, r=asuth,necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/c7966d91f89e Part 3: Move RemoteLazyInputStream to its own toplevel protocol, r=asuth,necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/1923922be138 Part 4: Stop requiring nsMIMEInputStream to be seekable, r=asuth,necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/473cad5b22ab 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/1ac1908ed3d0 Part 6: Preserve MIME information when serializing a lazy stream, r=asuth,necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/eb6de0b1ab8e Part 7: Consistently normalize upload streams passed to HTTP channels, r=asuth,necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/5e1fad91e935 Part 8: Remove SeekableStreamWrapper, r=asuth,necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/eace8d4756ef Part 9: Remove PartiallySeekableInputStream, r=asuth,necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/452fe0a15c99 Part 10: Clone PostData when writing into & reading from SessionHistoryInfo, r=smaug https://hg.mozilla.org/integration/autoland/rev/cde16b2cdbfb Part 11: Simplify the IPCStream serialization API, r=asuth,necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/2aa9a0a9adbc Part 12: Update browser_post_auth.js assertions, r=necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/7cf122f6e19f Part 13: Relax interface check in nsHttpTransaction, r=necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/a62dab05c6cd Part 14: Fix potential skipping of async streams in nsMultiplexInputStream, r=asuth

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

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

Flags: needinfo?(nika)
Pushed by nlayzell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2059c2d0d880 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/6d8e51b3e8d7 Part 2: Remove the PFileDescriptorSet interface, r=asuth,necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/0a6cf0817bf5 Part 3: Move RemoteLazyInputStream to its own toplevel protocol, r=asuth,necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/82b38c12b298 Part 4: Stop requiring nsMIMEInputStream to be seekable, r=asuth,necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/e40e810e18fc 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/6802a4326963 Part 6: Preserve MIME information when serializing a lazy stream, r=asuth,necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/35a69828091c Part 7: Consistently normalize upload streams passed to HTTP channels, r=asuth,necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/d95e7ad0eafa Part 8: Remove SeekableStreamWrapper, r=asuth,necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/51bd50b57dd5 Part 9: Remove PartiallySeekableInputStream, r=asuth,necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/c4f1e86992b6 Part 10: Clone PostData when writing into & reading from SessionHistoryInfo, r=smaug https://hg.mozilla.org/integration/autoland/rev/234ff9e092c2 Part 11: Simplify the IPCStream serialization API, r=asuth,necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/9018dd592230 Part 12: Update browser_post_auth.js assertions, r=necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/a147df47a0e3 Part 13: Relax interface check in nsHttpTransaction, r=necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/96a39c2ba7a3 Part 14: Fix potential skipping of async streams in nsMultiplexInputStream, r=asuth https://hg.mozilla.org/integration/autoland/rev/017e1552d549 Part 15: Don't track mOriginalInput in nsPipe, r=asuth https://hg.mozilla.org/integration/autoland/rev/63f17a06eba9 Part 16: Disable tests which create too many blob URIs with TSan, r=asuth
Regressions: 1767894

Backed out for causing crashes e.g. Bug 1767808, and hanging Gmail (Bug 1767918)
https://hg.mozilla.org/mozilla-central/rev/b6dca56b56ccf7002c5a222a109898a3c32c82a8

Status: RESOLVED → REOPENED
status-firefox102: fixed → ---
Flags: needinfo?(nika)
Resolution: FIXED → ---
Target Milestone: 102 Branch → ---

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

Regressions: 1767922
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
Flags: needinfo?(nika)

(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

(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

(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

Pushed by nlayzell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/611ea76d7a9c 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/659581d4159b Part 2: Remove the PFileDescriptorSet interface, r=asuth,necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/ccb4be659107 Part 3: Move RemoteLazyInputStream to its own toplevel protocol, r=asuth,necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/36563e3b1e04 Part 4: Stop requiring nsMIMEInputStream to be seekable, r=asuth,necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/849b63707d7f 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/e1ae48f30d2c Part 6: Preserve MIME information when serializing a lazy stream, r=asuth,necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/5c2872ad6912 Part 7: Consistently normalize upload streams passed to HTTP channels, r=asuth,necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/d66aacfcf983 Part 8: Remove SeekableStreamWrapper, r=asuth,necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/ee7aeb631cd9 Part 9: Remove PartiallySeekableInputStream, r=asuth,necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/f9abe4fbf501 Part 10: Clone PostData when writing into & reading from SessionHistoryInfo, r=smaug https://hg.mozilla.org/integration/autoland/rev/e73abb071bb3 Part 11: Simplify the IPCStream serialization API, r=asuth,necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/1dfbf61ff5dd Part 12: Update browser_post_auth.js assertions, r=necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/b7723490f025 Part 13: Relax interface check in nsHttpTransaction, r=necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/2e56c89cf55d Part 14: Fix potential skipping of async streams in nsMultiplexInputStream, r=asuth https://hg.mozilla.org/integration/autoland/rev/fc6c39f56d21 Part 15: Don't track mOriginalInput in nsPipe, r=asuth https://hg.mozilla.org/integration/autoland/rev/ecbf5f3c51de Part 16: Disable tests which create too many blob URIs with TSan, r=asuth https://hg.mozilla.org/integration/autoland/rev/673ecd5337e1 Part 17: Keep pipe streams alive so long as there's a callback registered, r=asuth

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

Pushed by nlayzell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2660731a9aea 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/c4f201a829c9 Part 2: Remove the PFileDescriptorSet interface, r=asuth,necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/c3d8d84d01b9 Part 3: Move RemoteLazyInputStream to its own toplevel protocol, r=asuth,necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/d07a20fa2794 Part 4: Stop requiring nsMIMEInputStream to be seekable, r=asuth,necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/a546742d3f11 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/e54aff74d256 Part 6: Preserve MIME information when serializing a lazy stream, r=asuth,necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/0b2df3479481 Part 7: Consistently normalize upload streams passed to HTTP channels, r=asuth,necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/249922740433 Part 8: Remove SeekableStreamWrapper, r=asuth,necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/4578d30460cb Part 9: Remove PartiallySeekableInputStream, r=asuth,necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/185b61fd584a Part 10: Clone PostData when writing into & reading from SessionHistoryInfo, r=smaug https://hg.mozilla.org/integration/autoland/rev/baa047853223 Part 11: Simplify the IPCStream serialization API, r=asuth,necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/08e8e64cf522 Part 12: Update browser_post_auth.js assertions, r=necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/6396c9396eb7 Part 13: Relax interface check in nsHttpTransaction, r=necko-reviewers,kershaw https://hg.mozilla.org/integration/autoland/rev/6efd4589b337 Part 14: Fix potential skipping of async streams in nsMultiplexInputStream, r=asuth https://hg.mozilla.org/integration/autoland/rev/8de4bb94de16 Part 15: Don't track mOriginalInput in nsPipe, r=asuth https://hg.mozilla.org/integration/autoland/rev/4f0ce6d040be Part 16: Disable tests which create too many blob URIs with TSan, r=asuth https://hg.mozilla.org/integration/autoland/rev/c1bb4f38919f Part 17: Keep pipe streams alive so long as there's a callback registered, r=asuth https://hg.mozilla.org/integration/autoland/rev/3f4ebef6a4b1 Part 18: Ensure AsyncWait callbacks are cleared when NS_AsyncCopy completes, r=xpcom-reviewers,mccr8
Flags: needinfo?(nika)
Blocks: 1693155
Regressions: 1769504
Regressions: 1767808
Regressions: 1770927

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

Regressions: 1780054
Regressions: 1806264
Regressions: 1875621
Regressions: 1881964
See Also: → 751552
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: