Fix Stream.ReadAtLeast perf regression in DataContractSerializer #69879

eerhardt · 2022-05-26T21:34:13Z

#69272 changed DCS to no longer call Stream.Read inside a loop, but instead call the new ReadAtLeast method. ReadAtLeast only takes a Span<byte>, and not a byte[]. This caused a regression because the internal encoding stream wrapper classes don't override Read(Span<byte>), so the base implementation is used. The base implementation is slower because it needs to rent a byte[] from the pool, and do 2 copies.

Overriding Read(Span<byte>) on the internal encoding stream wrapper classes allows ReadAtLeast to be just as fast.

Fix #69730

dotnet#69272 changed DCS to no longer call Stream.Read inside a loop, but instead call the new ReadAtLeast method. ReadAtLeast only takes a Span<byte>, and not a byte[]. This caused a regression because the internal encoding stream wrapper classes don't override Read(Span<byte>), so the base implementation is used. The base implementation is slower because it needs to rent a byte[] from the pool, and do 2 copies. Overriding Read(Span<byte>) on the internal encoding stream wrapper classes allows ReadAtLeast to be just as fast. Fix dotnet#69730

eerhardt · 2022-05-26T21:35:28Z

...DataContractSerialization/src/System/Runtime/Serialization/Json/JsonEncodingStreamWrapper.cs

        {
            try
            {
                if (_byteCount == 0)
                {
                    if (_encodingCode == SupportedEncoding.UTF8)
                    {
-                        return _stream.Read(buffer, offset, count);
+                        return _stream.Read(buffer);


Note that _stream is always a BufferedStream, which also overrides Read(Span<byte>), so we shouldn't have the same problem here.

eerhardt · 2022-05-26T21:38:13Z

src/libraries/System.Private.DataContractSerialization/src/System/Xml/EncodingStreamWrapper.cs

        {
            try
            {
                if (_byteCount == 0)
                {
                    if (_encodingCode == SupportedEncoding.UTF8)
-                        return _stream.Read(buffer, offset, count);
+                        return _stream.Read(buffer);


However, EncodingStreamWrapper isn't guaranteed to be a BufferedStream - I'm not sure why these two encoding wrappers differ. The _stream here is the Stream that was passed into the public API. So if the caller passes a Stream that doesn't override Read(Span<byte>), this could run into the same problem.

stephentoub · 2022-05-27T00:34:52Z

...DataContractSerialization/src/System/Runtime/Serialization/Json/JsonEncodingStreamWrapper.cs

+        public override int Read(byte[] buffer, int offset, int count) =>
+            Read(new Span<byte>(buffer, offset, count));
+
+        public override int Read(Span<byte> buffer)


Can/should we override Write(span) on each of these streams as well?

Are there other streams in dotnet/runtime for which we should provide these overrides?

Can/should we override Write(span) on each of these streams as well?

Maybe? It could prevent future regressions, like the one I made here.

Looking at where these streams are used for writing, they only get instantiated when the Encoding is not UTF8 (which is different than read - where they always are used). And looking through all the "Write" methods to these streams, they all pass an array, offset, and count. I don't think I should add it since they will never get used. Thoughts?

Are there other streams in dotnet/runtime for which we should provide these overrides?

I looked through all the places that override Read(byte[] ...) and looked to see if they override Read(Span) as well.

Here's are ones I think we should add the additional override to:

Passed to BinaryReader / XmlReader / other place I changed to call ReadAtLeast / ReadExactly:

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/Design/DesigntimeLicenseContextSerializer.cs

Line 87 in 472050d

public override int Read(byte[] buffer, int offset, int count)

runtime/src/libraries/System.Data.Common/src/System/Data/SQLTypes/SqlXml.cs

Line 406 in 472050d

public override int Read(byte[] buffer, int offset, int count)

runtime/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/ImmutableMemoryStream.cs

Line 67 in 472050d

public override int Read(byte[] buffer, int offset, int count)

runtime/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/ReadOnlyUnmanagedMemoryStream.cs

Line 31 in 472050d

public override int Read(byte[] buffer, int offset, int count)

These get returned through public methods, so they probably should:

runtime/src/libraries/System.Data.Common/src/System/Data/SQLTypes/SQLBytes.cs

Line 695 in 472050d

public override int Read(byte[] buffer, int offset, int count)

runtime/src/libraries/System.IO.Packaging/src/System/IO/Packaging/ZipWrappingStream.cs

Line 61 in 472050d

public override int Read(

runtime/src/libraries/System.Net.Requests/src/System/Net/FtpDataStream.cs

Line 175 in 472050d

public override int Read(byte[] buffer, int offset, int size)

Here are the ones I'm not sure we need to:

Only appears to be used by FtpControlStream, I can't tell if anyone ever calls Read with a Span. Looks easy to do though.

runtime/src/libraries/System.Net.Requests/src/System/Net/NetworkStreamWrapper.cs

Line 137 in 472050d

public override int Read(byte[] buffer, int offset, int size)

Has a lot of derived Stream classes, but I don't see them ever getting Read(Span) call on them. This also looks easy to do.

runtime/src/libraries/System.Net.Mail/src/System/Net/DelegatedStream.cs

Line 131 in 472050d

public override int Read(byte[] buffer, int offset, int count)

Here's the ones I don't think we need to:

Only gets BeginRead and EndRead called on it

runtime/src/libraries/System.Net.HttpListener/src/System/Net/HttpRequestStream.cs

Line 16 in 472050d

public override int Read(byte[] buffer, int offset, int count)

Only used for writing

runtime/src/libraries/System.IO.Packaging/src/System/IO/Packaging/IgnoreFlushAndCloseStream.cs

Line 124 in 472050d

public override int Read(byte[] buffer, int offset, int count)

Only do "async" reading, so we can't do anything with a Span

runtime/src/libraries/System.Net.Security/src/System/Net/Security/NegotiateStream.cs

Line 305 in 472050d

public override int Read(byte[] buffer, int offset, int count)

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.cs

Line 741 in 472050d

public override int Read(byte[] buffer, int offset, int count)

runtime/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/CryptoStream.cs

Line 292 in 472050d

public override int Read(byte[] buffer, int offset, int count)

runtime/src/libraries/System.IO.Ports/src/System/IO/Ports/SerialStream.Unix.cs

Line 403 in 472050d

public override int Read(byte[] array, int offset, int count)

I've added the overrides for the first 4 locations above, and 2 of the next 3. The SqlBytes one is a little more involved, so I decided to open a separate issue for it - #69921.

…'t currently override the Span overloads.

stephentoub · 2022-05-27T21:59:06Z

src/libraries/System.Data.Common/src/System/Data/SQLTypes/SqlXml.cs

@@ -424,6 +424,22 @@ public override int Read(byte[] buffer, int offset, int count)
            return iBytesRead;
        }

+        // Duplicate the Read(byte[]) logic here instead of refactoring both to use Spans
+        // in case the backing _stream doesn't override Read(Span).


In most of our library code we've just assumed that any stream that cares about perf will end up overriding the relevant methods, and so for example wrapper streams like DeflateStream just use, say, ReadAsync(Memory) rather than having two code paths. But if we think it's worthwhile here to special-case it, maybe because we expect streams wrapped here to be more legacy, that's ok. Eventually it'd be nice to consolidate, though.

Yeah, that was my thinking - this stream can come from a public constructor parameter, and this type is rather old - so older code could be calling it.

I took a quick look, and the first place I found actually has this problem:

https://github.com/dotnet/SqlClient/blob/9a7d53ea1651b94783c07f37eaa39cd8f728c02e/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCachedBuffer.cs#L130-L134

internal SqlXml ToSqlXml() { SqlXml sx = new(ToStream()); return sx; }

https://github.com/dotnet/SqlClient/blob/9a7d53ea1651b94783c07f37eaa39cd8f728c02e/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCachedBuffer.cs#L99-L102

internal Stream ToStream() { return new SqlCachedStream(this); }

and SqlCachedStream doesn't override the Span-based API.

I've opened dotnet/SqlClient#1628 for the Sql team to consider providing Span-based overrides.

eerhardt added the area-Serialization label May 26, 2022

eerhardt requested review from stephentoub, HongGit and StephenMolloy May 26, 2022 21:34

ghost assigned eerhardt May 26, 2022

eerhardt commented May 26, 2022

View reviewed changes

stephentoub reviewed May 27, 2022

View reviewed changes

stephentoub approved these changes May 27, 2022

View reviewed changes

Add Span overloads for Stream Read and Write implementations that don…

bde8b5c

…'t currently override the Span overloads.

stephentoub reviewed May 27, 2022

View reviewed changes

stephentoub approved these changes May 27, 2022

View reviewed changes

eerhardt mentioned this pull request May 27, 2022

SqlStream and SqlCachedStream should consider overriding Span Read/Write methods dotnet/SqlClient#1628

Open

eerhardt merged commit 37a0d9f into dotnet:main May 28, 2022

eerhardt deleted the FixDCSPerfRegression branch May 28, 2022 00:16

kunalspathak mentioned this pull request May 31, 2022

[Perf] Changes at 5/17/2022 10:44:56 PM dotnet/perf-autofiling-issues#5565

Closed

lewing mentioned this pull request Jun 8, 2022

[Perf][Mono] Regression on Serailizer Benchmarks Changes at 5/20/2022 2:58:35 PM #70219

Closed

dakersnar mentioned this pull request Jun 10, 2022

.NET 7.0 Preview 5 Microbenchmarks Performance Study Report #70590

Closed

14 tasks

ghost locked as resolved and limited conversation to collaborators Jun 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Stream.ReadAtLeast perf regression in DataContractSerializer #69879

Fix Stream.ReadAtLeast perf regression in DataContractSerializer #69879

eerhardt commented May 26, 2022

eerhardt May 26, 2022

eerhardt May 26, 2022

stephentoub May 27, 2022

eerhardt May 27, 2022

eerhardt May 27, 2022

stephentoub May 27, 2022

eerhardt May 27, 2022

Fix Stream.ReadAtLeast perf regression in DataContractSerializer #69879

Fix Stream.ReadAtLeast perf regression in DataContractSerializer #69879

Conversation

eerhardt commented May 26, 2022

eerhardt May 26, 2022

Choose a reason for hiding this comment

eerhardt May 26, 2022

Choose a reason for hiding this comment

stephentoub May 27, 2022

Choose a reason for hiding this comment

eerhardt May 27, 2022

Choose a reason for hiding this comment

Here's are ones I think we should add the additional override to:

Here are the ones I'm not sure we need to:

Here's the ones I don't think we need to:

eerhardt May 27, 2022

Choose a reason for hiding this comment

stephentoub May 27, 2022

Choose a reason for hiding this comment

eerhardt May 27, 2022

Choose a reason for hiding this comment