Skip to content

Commit

Permalink
Fix Stream.ReadAtLeast perf regression in DataContractSerializer (#69879
Browse files Browse the repository at this point in the history
)

* Fix Stream.ReadAtLeast perf regression in DataContractSerializer

#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

* Add Span overloads for Stream Read and Write implementations that don't currently override the Span overloads.
  • Loading branch information
eerhardt authored May 28, 2022
1 parent 7c4d1f1 commit 37a0d9f
Show file tree
Hide file tree
Showing 8 changed files with 114 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -84,17 +84,20 @@ public StreamWrapper(Stream stream)

public override void Flush() => _stream.Flush();

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

public override int Read(Span<byte> buffer)
{
Debug.Assert(_stream.Position != 0, "Expected the first byte to be read first");
if (_stream.Position == 1)
{
Debug.Assert(_readFirstByte == true);
// Add the first byte read by ReadByte into buffer here
buffer[offset] = _firstByte;
return _stream.Read(buffer, offset + 1, count - 1) + 1;
buffer[0] = _firstByte;
return _stream.Read(buffer.Slice(1)) + 1;
}
return _stream.Read(buffer, offset, count);
return _stream.Read(buffer);
}

public override long Seek(long offset, SeekOrigin origin) => _stream.Seek(offset, origin);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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).
public override int Read(Span<byte> buffer)

This comment has been minimized.

Copy link
@davidfowl

davidfowl May 29, 2022

Member

Don't our Stream analyzers catch these?

{
ThrowIfStreamClosed(nameof(Read));
ThrowIfStreamCannotRead(nameof(Read));

if (_stream.CanSeek && _stream.Position != _lPosition)
_stream.Seek(_lPosition, SeekOrigin.Begin);

int iBytesRead = _stream.Read(buffer);
_lPosition += iBytesRead;

return iBytesRead;
}

public override void Write(byte[] buffer, int offset, int count)
{
ThrowIfStreamClosed(nameof(Write));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,22 @@ int count
return _baseStream.Read(buffer, offset, count);
}

#if NETCOREAPP
public override void Write(
ReadOnlySpan<byte> buffer
)
{
_baseStream.Write(buffer);
}

public override int Read(
Span<byte> buffer
)
{
return _baseStream.Read(buffer);
}
#endif

public override void SetLength(
long value
)
Expand Down
35 changes: 35 additions & 0 deletions src/libraries/System.Net.Requests/src/System/Net/FtpDataStream.cs
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,27 @@ public override int Read(byte[] buffer, int offset, int size)
return readBytes;
}

public override int Read(Span<byte> buffer)
{
CheckError();
int readBytes;
try
{
readBytes = _networkStream.Read(buffer);
}
catch
{
CheckError();
throw;
}
if (readBytes == 0)
{
_isFullyRead = true;
Close();
}
return readBytes;
}

public override void Write(byte[] buffer, int offset, int size)
{
CheckError();
Expand All @@ -207,6 +228,20 @@ public override void Write(byte[] buffer, int offset, int size)
}
}

public override void Write(ReadOnlySpan<byte> buffer)
{
CheckError();
try
{
_networkStream.Write(buffer);
}
catch
{
CheckError();
throw;
}
}

private void AsyncReadCallback(IAsyncResult ar)
{
LazyAsyncResult userResult = (LazyAsyncResult)ar.AsyncState!;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,15 +174,18 @@ public override void Flush()
_stream.Flush();
}

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

public override int Read(Span<byte> buffer)
{
try
{
if (_byteCount == 0)
{
if (_encodingCode == SupportedEncoding.UTF8)
{
return _stream.Read(buffer, offset, count);
return _stream.Read(buffer);
}

Debug.Assert(_bytes != null);
Expand All @@ -206,11 +209,13 @@ public override int Read(byte[] buffer, int offset, int count)
}

// Give them bytes
int count = buffer.Length;
if (_byteCount < count)
{
count = _byteCount;
}
Buffer.BlockCopy(_bytes!, _byteOffset, buffer, offset, count);

_bytes.AsSpan(_byteOffset, count).CopyTo(buffer);
_byteOffset += count;
_byteCount -= count;
return count;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -593,14 +593,17 @@ public override int ReadByte()
return _byteBuffer[0];
}

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

public override int Read(Span<byte> buffer)
{
try
{
if (_byteCount == 0)
{
if (_encodingCode == SupportedEncoding.UTF8)
return _stream.Read(buffer, offset, count);
return _stream.Read(buffer);

Debug.Assert(_bytes != null);
Debug.Assert(_chars != null);
Expand All @@ -622,9 +625,11 @@ public override int Read(byte[] buffer, int offset, int count)
}

// Give them bytes
int count = buffer.Length;
if (_byteCount < count)
count = _byteCount;
Buffer.BlockCopy(_bytes!, _byteOffset, buffer, offset, count);

_bytes.AsSpan(_byteOffset, count).CopyTo(buffer);
_byteOffset += count;
_byteCount -= count;
return count;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,18 @@ public override int Read(byte[] buffer, int offset, int count)
return result;
}

#if NETCOREAPP
// Duplicate the Read(byte[]) logic here instead of refactoring both to use Spans
// so we don't affect perf on .NET Framework.
public override int Read(Span<byte> buffer)
{
int result = Math.Min(buffer.Length, _array.Length - _position);
_array.AsSpan(_position, result).CopyTo(buffer);
_position += result;
return result;
}
#endif

public override long Seek(long offset, SeekOrigin origin)
{
long target;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,18 @@ public override int Read(byte[] buffer, int offset, int count)
return bytesRead;
}

#if NETCOREAPP
// Duplicate the Read(byte[]) logic here instead of refactoring both to use Spans
// so we don't affect perf on .NET Framework.
public override int Read(Span<byte> buffer)
{
int bytesRead = Math.Min(buffer.Length, _length - _position);
new Span<byte>(_data + _position, bytesRead).CopyTo(buffer);
_position += bytesRead;
return bytesRead;
}
#endif

public override void Flush()
{
}
Expand Down

0 comments on commit 37a0d9f

Please sign in to comment.