From 22cbc629a245db9cbe4777d1d04d5a3b3edc5b02 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sat, 7 Jan 2023 18:57:35 +0200 Subject: [PATCH 01/15] Use stack-allocated memory in `BinaryReader`'s `Read***` methods. --- .../src/System/IO/BinaryReader.cs | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs index 0033453e93e0eb..7054ca94165fbb 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs @@ -72,7 +72,7 @@ public BinaryReader(Stream input, Encoding encoding, bool leaveOpen) _2BytesPerChar = encoding is UnicodeEncoding; // check if BinaryReader is based on MemoryStream, and keep this for it's life // we cannot use "as" operator, since derived classes are not allowed - _isMemoryStream = (_stream.GetType() == typeof(MemoryStream)); + _isMemoryStream = _stream.GetType() == typeof(MemoryStream); _leaveOpen = leaveOpen; Debug.Assert(_decoder != null, "[BinaryReader.ctor]_decoder!=null"); @@ -229,24 +229,24 @@ public virtual char ReadChar() return (char)value; } - public virtual short ReadInt16() => BinaryPrimitives.ReadInt16LittleEndian(InternalRead(2)); + public virtual short ReadInt16() => BinaryPrimitives.ReadInt16LittleEndian(InternalRead(stackalloc byte[2])); [CLSCompliant(false)] - public virtual ushort ReadUInt16() => BinaryPrimitives.ReadUInt16LittleEndian(InternalRead(2)); + public virtual ushort ReadUInt16() => BinaryPrimitives.ReadUInt16LittleEndian(InternalRead(stackalloc byte[2])); - public virtual int ReadInt32() => BinaryPrimitives.ReadInt32LittleEndian(InternalRead(4)); + public virtual int ReadInt32() => BinaryPrimitives.ReadInt32LittleEndian(InternalRead(stackalloc byte[4])); [CLSCompliant(false)] - public virtual uint ReadUInt32() => BinaryPrimitives.ReadUInt32LittleEndian(InternalRead(4)); - public virtual long ReadInt64() => BinaryPrimitives.ReadInt64LittleEndian(InternalRead(8)); + public virtual uint ReadUInt32() => BinaryPrimitives.ReadUInt32LittleEndian(InternalRead(stackalloc byte[4])); + public virtual long ReadInt64() => BinaryPrimitives.ReadInt64LittleEndian(InternalRead(stackalloc byte[8])); [CLSCompliant(false)] - public virtual ulong ReadUInt64() => BinaryPrimitives.ReadUInt64LittleEndian(InternalRead(8)); - public virtual Half ReadHalf() => BitConverter.Int16BitsToHalf(BinaryPrimitives.ReadInt16LittleEndian(InternalRead(2))); - public virtual unsafe float ReadSingle() => BitConverter.Int32BitsToSingle(BinaryPrimitives.ReadInt32LittleEndian(InternalRead(4))); - public virtual unsafe double ReadDouble() => BitConverter.Int64BitsToDouble(BinaryPrimitives.ReadInt64LittleEndian(InternalRead(8))); + public virtual ulong ReadUInt64() => BinaryPrimitives.ReadUInt64LittleEndian(InternalRead(stackalloc byte[8])); + public virtual Half ReadHalf() => BinaryPrimitives.ReadHalfLittleEndian(InternalRead(stackalloc byte[2])); + public virtual unsafe float ReadSingle() => BinaryPrimitives.ReadSingleLittleEndian(InternalRead(stackalloc byte[4])); + public virtual unsafe double ReadDouble() => BinaryPrimitives.ReadDoubleLittleEndian(InternalRead(stackalloc byte[8])); public virtual decimal ReadDecimal() { - ReadOnlySpan span = InternalRead(16); + ReadOnlySpan span = InternalRead(stackalloc byte[16]); try { return decimal.ToDecimal(span); @@ -478,23 +478,23 @@ public virtual byte[] ReadBytes(int count) return result; } - private ReadOnlySpan InternalRead(int numBytes) + private ReadOnlySpan InternalRead(Span buffer) { - Debug.Assert(numBytes >= 2 && numBytes <= 16, "value of 1 should use ReadByte. value > 16 requires to change the minimal _buffer size"); + Debug.Assert(buffer.Length != 1, "length of 1 should use ReadByte."); if (_isMemoryStream) { // read directly from MemoryStream buffer Debug.Assert(_stream is MemoryStream); - return ((MemoryStream)_stream).InternalReadSpan(numBytes); + return ((MemoryStream)_stream).InternalReadSpan(buffer.Length); } else { ThrowIfDisposed(); - _stream.ReadExactly(_buffer.AsSpan(0, numBytes)); + _stream.ReadExactly(buffer); - return _buffer; + return buffer; } } From 1203af684985593750c1d2d23e46d6a1b559fc52 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sat, 7 Jan 2023 18:26:41 +0200 Subject: [PATCH 02/15] Avoid allocating `_buffer`. It now gets allocated only when calling the obscure `FillBuffer` method. --- .../src/System/IO/BinaryReader.cs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs index 7054ca94165fbb..a2486aa92d028c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs @@ -25,11 +25,12 @@ public class BinaryReader : IDisposable private const int MaxCharBytesSize = 128; private readonly Stream _stream; - private readonly byte[] _buffer; + private byte[]? _buffer; private readonly Decoder _decoder; private byte[]? _charBytes; private char[]? _charBuffer; private readonly int _maxCharsSize; // From MaxCharBytesSize & Encoding + private readonly int _bufferSize; // Performance optimization for Read() w/ Unicode. Speeds us up by ~40% private readonly bool _2BytesPerChar; @@ -63,9 +64,7 @@ public BinaryReader(Stream input, Encoding encoding, bool leaveOpen) { minBufferSize = 16; } - - _buffer = new byte[minBufferSize]; - // _charBuffer and _charBytes will be left null. + _bufferSize = minBufferSize; // For Encodings that always use 2 bytes per char (or more), // special case them here to make Read() & Peek() faster. @@ -504,13 +503,15 @@ private ReadOnlySpan InternalRead(Span buffer) // reasons. More about the subject in: https://github.com/dotnet/coreclr/pull/22102 protected virtual void FillBuffer(int numBytes) { - if (numBytes < 0 || numBytes > _buffer.Length) + if (numBytes < 0 || numBytes > _bufferSize) { throw new ArgumentOutOfRangeException(nameof(numBytes), SR.ArgumentOutOfRange_BinaryReaderFillBuffer); } ThrowIfDisposed(); + _buffer ??= new byte[_bufferSize]; + // Need to find a good threshold for calling ReadByte() repeatedly // vs. calling Read(byte[], int, int) for both buffered & unbuffered // streams. From 65979b837e8b07ac8b6efa16a42c2ae63de66bf7 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sat, 7 Jan 2023 22:58:17 +0200 Subject: [PATCH 03/15] Use array slicing syntax. --- .../System.Private.CoreLib/src/System/IO/BinaryReader.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs index a2486aa92d028c..f96309842033fa 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs @@ -468,10 +468,8 @@ public virtual byte[] ReadBytes(int count) if (numRead != result.Length) { - // Trim array. This should happen on EOF & possibly net streams. - byte[] copy = new byte[numRead]; - Buffer.BlockCopy(result, 0, copy, 0, numRead); - result = copy; + // Trim array. This should happen on EOF & possibly net streams. + result = result[..numRead]; } return result; From a705ce098f59d06dccc10d5c8da3f7d1e7c75e2d Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sat, 7 Jan 2023 22:39:15 +0200 Subject: [PATCH 04/15] Lazily allocate the `Decoder`. --- .../src/System/IO/BinaryReader.cs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs index f96309842033fa..113a5fd10a77b1 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs @@ -26,7 +26,8 @@ public class BinaryReader : IDisposable private readonly Stream _stream; private byte[]? _buffer; - private readonly Decoder _decoder; + private readonly Encoding _encoding; + private Decoder? _decoder; private byte[]? _charBytes; private char[]? _charBuffer; private readonly int _maxCharsSize; // From MaxCharBytesSize & Encoding @@ -57,7 +58,7 @@ public BinaryReader(Stream input, Encoding encoding, bool leaveOpen) } _stream = input; - _decoder = encoding.GetDecoder(); + _encoding = encoding; _maxCharsSize = encoding.GetMaxCharCount(MaxCharBytesSize); int minBufferSize = encoding.GetMaxByteCount(1); // max bytes per one char if (minBufferSize < 16) @@ -73,8 +74,6 @@ public BinaryReader(Stream input, Encoding encoding, bool leaveOpen) // we cannot use "as" operator, since derived classes are not allowed _isMemoryStream = _stream.GetType() == typeof(MemoryStream); _leaveOpen = leaveOpen; - - Debug.Assert(_decoder != null, "[BinaryReader.ctor]_decoder!=null"); } public virtual Stream BaseStream => _stream; @@ -140,6 +139,7 @@ public virtual int Read() posSav = _stream.Position; } + _decoder ??= _encoding.GetDecoder(); _charBytes ??= new byte[MaxCharBytesSize]; char singleChar = '\0'; @@ -279,6 +279,7 @@ public virtual string ReadString() return string.Empty; } + _decoder ??= _encoding.GetDecoder(); _charBytes ??= new byte[MaxCharBytesSize]; _charBuffer ??= new char[_maxCharsSize]; @@ -336,6 +337,8 @@ private int InternalReadChars(Span buffer) { Debug.Assert(!_disposed); + _decoder ??= _encoding.GetDecoder(); + int totalCharsRead = 0; while (!buffer.IsEmpty) From 92b0113b3726e7e0a055f6c08851eba7c3f1dafd Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sat, 7 Jan 2023 23:03:13 +0200 Subject: [PATCH 05/15] Entirely remove the `_buffer` field in favor of array pool allocations. --- .../src/System/IO/BinaryReader.cs | 72 +++++++++---------- 1 file changed, 32 insertions(+), 40 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs index 113a5fd10a77b1..8c8076d6095901 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs @@ -13,6 +13,7 @@ ** ============================================================*/ +using System.Buffers; using System.Buffers.Binary; using System.Diagnostics; using System.Runtime.CompilerServices; @@ -25,13 +26,11 @@ public class BinaryReader : IDisposable private const int MaxCharBytesSize = 128; private readonly Stream _stream; - private byte[]? _buffer; private readonly Encoding _encoding; private Decoder? _decoder; private byte[]? _charBytes; private char[]? _charBuffer; private readonly int _maxCharsSize; // From MaxCharBytesSize & Encoding - private readonly int _bufferSize; // Performance optimization for Read() w/ Unicode. Speeds us up by ~40% private readonly bool _2BytesPerChar; @@ -60,12 +59,6 @@ public BinaryReader(Stream input, Encoding encoding, bool leaveOpen) _stream = input; _encoding = encoding; _maxCharsSize = encoding.GetMaxCharCount(MaxCharBytesSize); - int minBufferSize = encoding.GetMaxByteCount(1); // max bytes per one char - if (minBufferSize < 16) - { - minBufferSize = 16; - } - _bufferSize = minBufferSize; // For Encodings that always use 2 bytes per char (or more), // special case them here to make Read() & Peek() faster. @@ -504,42 +497,41 @@ private ReadOnlySpan InternalRead(Span buffer) // reasons. More about the subject in: https://github.com/dotnet/coreclr/pull/22102 protected virtual void FillBuffer(int numBytes) { - if (numBytes < 0 || numBytes > _bufferSize) - { - throw new ArgumentOutOfRangeException(nameof(numBytes), SR.ArgumentOutOfRange_BinaryReaderFillBuffer); - } + ArgumentOutOfRangeException.ThrowIfNegative(numBytes); ThrowIfDisposed(); - _buffer ??= new byte[_bufferSize]; - - // Need to find a good threshold for calling ReadByte() repeatedly - // vs. calling Read(byte[], int, int) for both buffered & unbuffered - // streams. - if (numBytes == 1) - { - int n = _stream.ReadByte(); - if (n == -1) - { - ThrowHelper.ThrowEndOfFileException(); - } - - _buffer[0] = (byte)n; - return; - } - - if (numBytes > 0) - { - _stream.ReadExactly(_buffer.AsSpan(0, numBytes)); - } - else + switch (numBytes) { - // ReadExactly no-ops for empty buffers, so special case numBytes == 0 to preserve existing behavior. - int n = _stream.Read(_buffer, 0, 0); - if (n == 0) - { - ThrowHelper.ThrowEndOfFileException(); - } + case 0: + // ReadExactly no-ops for empty buffers, so special case numBytes == 0 to preserve existing behavior. + int n = _stream.Read(Array.Empty(), 0, 0); + if (n == 0) + { + ThrowHelper.ThrowEndOfFileException(); + } + break; + case 1: + // Need to find a good threshold for calling ReadByte() repeatedly + // vs. calling Read(byte[], int, int) for both buffered & unbuffered + // streams. + n = _stream.ReadByte(); + if (n == -1) + { + ThrowHelper.ThrowEndOfFileException(); + } + break; + default: + byte[] buffer = ArrayPool.Shared.Rent(numBytes); + try + { + _stream.ReadExactly(buffer.AsSpan(0, numBytes)); + } + finally + { + ArrayPool.Shared.Return(buffer); + } + break; } } From 6912f4d83b9c9b11b6dd0b628fde2e28b9e2f673 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sat, 7 Jan 2023 23:12:52 +0200 Subject: [PATCH 06/15] Use array slicing syntax once more. --- .../System.Private.CoreLib/src/System/IO/BinaryReader.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs index 8c8076d6095901..e0ce2f7c6ade3a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs @@ -420,9 +420,7 @@ public virtual char[] ReadChars(int count) int n = InternalReadChars(new Span(chars)); if (n != count) { - char[] copy = new char[n]; - Buffer.BlockCopy(chars, 0, copy, 0, 2 * n); // sizeof(char) - chars = copy; + chars = chars[..n]; } return chars; From d45047333a4fdb594277564e5b1bffa07c96e014 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sat, 7 Jan 2023 23:31:31 +0200 Subject: [PATCH 07/15] Remove the `_charBytes` field in favor of stack allocations. --- .../src/System/IO/BinaryReader.cs | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs index e0ce2f7c6ade3a..998158cfe388ae 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs @@ -28,7 +28,6 @@ public class BinaryReader : IDisposable private readonly Stream _stream; private readonly Encoding _encoding; private Decoder? _decoder; - private byte[]? _charBytes; private char[]? _charBuffer; private readonly int _maxCharsSize; // From MaxCharBytesSize & Encoding @@ -133,7 +132,7 @@ public virtual int Read() } _decoder ??= _encoding.GetDecoder(); - _charBytes ??= new byte[MaxCharBytesSize]; + Span charBytes = stackalloc byte[MaxCharBytesSize]; char singleChar = '\0'; @@ -146,7 +145,7 @@ public virtual int Read() numBytes = _2BytesPerChar ? 2 : 1; int r = _stream.ReadByte(); - _charBytes[0] = (byte)r; + charBytes[0] = (byte)r; if (r == -1) { numBytes = 0; @@ -154,7 +153,7 @@ public virtual int Read() if (numBytes == 2) { r = _stream.ReadByte(); - _charBytes[1] = (byte)r; + charBytes[1] = (byte)r; if (r == -1) { numBytes = 1; @@ -166,11 +165,11 @@ public virtual int Read() return -1; } - Debug.Assert(numBytes == 1 || numBytes == 2, "BinaryReader::ReadOneChar assumes it's reading one or 2 bytes only."); + Debug.Assert(numBytes is 1 or 2, "BinaryReader::ReadOneChar assumes it's reading one or two bytes only."); try { - charsRead = _decoder.GetChars(new ReadOnlySpan(_charBytes, 0, numBytes), new Span(ref singleChar), flush: false); + charsRead = _decoder.GetChars(charBytes[..numBytes], new Span(ref singleChar), flush: false); } catch { @@ -273,21 +272,22 @@ public virtual string ReadString() } _decoder ??= _encoding.GetDecoder(); - _charBytes ??= new byte[MaxCharBytesSize]; _charBuffer ??= new char[_maxCharsSize]; + Span charBytes = stackalloc byte[MaxCharBytesSize]; + StringBuilder? sb = null; do { readLength = ((stringLength - currPos) > MaxCharBytesSize) ? MaxCharBytesSize : (stringLength - currPos); - n = _stream.Read(_charBytes, 0, readLength); + n = _stream.Read(charBytes[..readLength]); if (n == 0) { ThrowHelper.ThrowEndOfFileException(); } - charsRead = _decoder.GetChars(_charBytes, 0, n, _charBuffer, 0); + charsRead = _decoder.GetChars(charBytes[..n], _charBuffer, flush: false); if (currPos == 0 && n == stringLength) { @@ -333,6 +333,7 @@ private int InternalReadChars(Span buffer) _decoder ??= _encoding.GetDecoder(); int totalCharsRead = 0; + Span charBytes = stackalloc byte[MaxCharBytesSize]; while (!buffer.IsEmpty) { @@ -353,10 +354,9 @@ private int InternalReadChars(Span buffer) // a custom replacement sequence may violate this assumption. if (numBytes > 1) { - DecoderNLS? decoder = _decoder as DecoderNLS; // For internal decoders, we can check whether the decoder has any pending state. // For custom decoders, assume that the decoder has pending state. - if (decoder == null || decoder.HasState) + if (_decoder is not DecoderNLS decoder || decoder.HasState) { numBytes--; @@ -367,7 +367,7 @@ private int InternalReadChars(Span buffer) } } - ReadOnlySpan byteBuffer; + scoped ReadOnlySpan byteBuffer; if (_isMemoryStream) { Debug.Assert(_stream is MemoryStream); @@ -379,15 +379,13 @@ private int InternalReadChars(Span buffer) } else { - _charBytes ??= new byte[MaxCharBytesSize]; - if (numBytes > MaxCharBytesSize) { numBytes = MaxCharBytesSize; } - numBytes = _stream.Read(_charBytes, 0, numBytes); - byteBuffer = new ReadOnlySpan(_charBytes, 0, numBytes); + numBytes = _stream.Read(charBytes[..numBytes]); + byteBuffer = charBytes[..numBytes]; } if (byteBuffer.IsEmpty) From 48e27cccfcdf5cc09a37dae20acb5e8ddb6417df Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sat, 7 Jan 2023 23:37:27 +0200 Subject: [PATCH 08/15] Use `Unsafe.As` to cast to memory streams. We checked it on the constructor and already had asserts right before the casts. --- .../System.Private.CoreLib/src/System/IO/BinaryReader.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs index 998158cfe388ae..b9fad37025bbea 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs @@ -371,7 +371,7 @@ private int InternalReadChars(Span buffer) if (_isMemoryStream) { Debug.Assert(_stream is MemoryStream); - MemoryStream mStream = (MemoryStream)_stream; + MemoryStream mStream = Unsafe.As(_stream); int position = mStream.InternalGetPosition(); numBytes = mStream.InternalEmulateRead(numBytes); @@ -475,7 +475,7 @@ private ReadOnlySpan InternalRead(Span buffer) { // read directly from MemoryStream buffer Debug.Assert(_stream is MemoryStream); - return ((MemoryStream)_stream).InternalReadSpan(buffer.Length); + return Unsafe.As(_stream).InternalReadSpan(buffer.Length); } else { From d6d3bc80924c97f6cba9f95f0f98bf965a48d282 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sun, 8 Jan 2023 00:03:41 +0200 Subject: [PATCH 09/15] Optimize `GetString` to directly decode the bytes into a string if it is small. --- .../System.Private.CoreLib/src/System/IO/BinaryReader.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs index b9fad37025bbea..21a9f97adc8bf2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs @@ -287,13 +287,13 @@ public virtual string ReadString() ThrowHelper.ThrowEndOfFileException(); } - charsRead = _decoder.GetChars(charBytes[..n], _charBuffer, flush: false); - if (currPos == 0 && n == stringLength) { - return new string(_charBuffer, 0, charsRead); + return _encoding.GetString(charBytes[..n]); } + charsRead = _decoder.GetChars(charBytes[..n], _charBuffer, flush: false); + // Since we could be reading from an untrusted data source, limit the initial size of the // StringBuilder instance we're about to get or create. It'll expand automatically as needed. From f7a2043a8d136c53b18106c1a6069d99808342bd Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 13 Feb 2023 21:22:51 +0200 Subject: [PATCH 10/15] Use `sizeof`. --- .../src/System/IO/BinaryReader.cs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs index 21a9f97adc8bf2..351caf230e3ca3 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs @@ -220,24 +220,24 @@ public virtual char ReadChar() return (char)value; } - public virtual short ReadInt16() => BinaryPrimitives.ReadInt16LittleEndian(InternalRead(stackalloc byte[2])); + public virtual short ReadInt16() => BinaryPrimitives.ReadInt16LittleEndian(InternalRead(stackalloc byte[sizeof(short)])); [CLSCompliant(false)] - public virtual ushort ReadUInt16() => BinaryPrimitives.ReadUInt16LittleEndian(InternalRead(stackalloc byte[2])); + public virtual ushort ReadUInt16() => BinaryPrimitives.ReadUInt16LittleEndian(InternalRead(stackalloc byte[sizeof(ushort)])); - public virtual int ReadInt32() => BinaryPrimitives.ReadInt32LittleEndian(InternalRead(stackalloc byte[4])); + public virtual int ReadInt32() => BinaryPrimitives.ReadInt32LittleEndian(InternalRead(stackalloc byte[sizeof(int)])); [CLSCompliant(false)] - public virtual uint ReadUInt32() => BinaryPrimitives.ReadUInt32LittleEndian(InternalRead(stackalloc byte[4])); - public virtual long ReadInt64() => BinaryPrimitives.ReadInt64LittleEndian(InternalRead(stackalloc byte[8])); + public virtual uint ReadUInt32() => BinaryPrimitives.ReadUInt32LittleEndian(InternalRead(stackalloc byte[sizeof(uint)])); + public virtual long ReadInt64() => BinaryPrimitives.ReadInt64LittleEndian(InternalRead(stackalloc byte[sizeof(long)])); [CLSCompliant(false)] - public virtual ulong ReadUInt64() => BinaryPrimitives.ReadUInt64LittleEndian(InternalRead(stackalloc byte[8])); - public virtual Half ReadHalf() => BinaryPrimitives.ReadHalfLittleEndian(InternalRead(stackalloc byte[2])); - public virtual unsafe float ReadSingle() => BinaryPrimitives.ReadSingleLittleEndian(InternalRead(stackalloc byte[4])); - public virtual unsafe double ReadDouble() => BinaryPrimitives.ReadDoubleLittleEndian(InternalRead(stackalloc byte[8])); + public virtual ulong ReadUInt64() => BinaryPrimitives.ReadUInt64LittleEndian(InternalRead(stackalloc byte[sizeof(ulong)])); + public virtual unsafe Half ReadHalf() => BinaryPrimitives.ReadHalfLittleEndian(InternalRead(stackalloc byte[sizeof(Half)])); + public virtual unsafe float ReadSingle() => BinaryPrimitives.ReadSingleLittleEndian(InternalRead(stackalloc byte[sizeof(float)])); + public virtual unsafe double ReadDouble() => BinaryPrimitives.ReadDoubleLittleEndian(InternalRead(stackalloc byte[sizeof(double)])); public virtual decimal ReadDecimal() { - ReadOnlySpan span = InternalRead(stackalloc byte[16]); + ReadOnlySpan span = InternalRead(stackalloc byte[sizeof(decimal)]); try { return decimal.ToDecimal(span); From 205fbd5eba7cc4363c997009fadb0819f29a5fe1 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 12 Jun 2023 22:50:44 +0300 Subject: [PATCH 11/15] Address review feedback on `FillBuffer`. --- .../src/System/IO/BinaryReader.cs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs index 351caf230e3ca3..5a3f59a406c852 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs @@ -518,15 +518,14 @@ protected virtual void FillBuffer(int numBytes) } break; default: - byte[] buffer = ArrayPool.Shared.Rent(numBytes); - try - { - _stream.ReadExactly(buffer.AsSpan(0, numBytes)); - } - finally + if (_stream.CanSeek) { - ArrayPool.Shared.Return(buffer); + _stream.Seek(numBytes, SeekOrigin.Current); + return; } + byte[] buffer = ArrayPool.Shared.Rent(numBytes); + _stream.ReadExactly(buffer.AsSpan(0, numBytes)); + ArrayPool.Shared.Return(buffer); break; } } From a4a02efc1c812f157652c4da0f42fefb93c710bf Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 12 Jun 2023 23:07:56 +0300 Subject: [PATCH 12/15] Assign `_decoder` and `_charBuffer` only if the string is big. --- .../System.Private.CoreLib/src/System/IO/BinaryReader.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs index 5a3f59a406c852..72728d8793bccf 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs @@ -271,9 +271,6 @@ public virtual string ReadString() return string.Empty; } - _decoder ??= _encoding.GetDecoder(); - _charBuffer ??= new char[_maxCharsSize]; - Span charBytes = stackalloc byte[MaxCharBytesSize]; StringBuilder? sb = null; @@ -292,6 +289,9 @@ public virtual string ReadString() return _encoding.GetString(charBytes[..n]); } + _decoder ??= _encoding.GetDecoder(); + _charBuffer ??= new char[_maxCharsSize]; + charsRead = _decoder.GetChars(charBytes[..n], _charBuffer, flush: false); // Since we could be reading from an untrusted data source, limit the initial size of the From 3e3f51cd0a76edd8ac35997b0634b6266bb516b8 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sun, 9 Jul 2023 18:48:20 +0300 Subject: [PATCH 13/15] Use Math.Min. --- .../System.Private.CoreLib/src/System/IO/BinaryReader.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs index 72728d8793bccf..ed5a91a89e9080 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs @@ -276,7 +276,7 @@ public virtual string ReadString() StringBuilder? sb = null; do { - readLength = ((stringLength - currPos) > MaxCharBytesSize) ? MaxCharBytesSize : (stringLength - currPos); + readLength = Math.Min(MaxCharBytesSize, stringLength - currPos); n = _stream.Read(charBytes[..readLength]); if (n == 0) From ac368a07f8caabda2f2030f91df81af0375dbd98 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sun, 9 Jul 2023 21:13:01 +0300 Subject: [PATCH 14/15] Declare variables where they are initialized. --- .../src/System/IO/BinaryReader.cs | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs index ed5a91a89e9080..b4f899edad2648 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs @@ -253,14 +253,8 @@ public virtual string ReadString() { ThrowIfDisposed(); - int currPos = 0; - int n; - int stringLength; - int readLength; - int charsRead; - // Length of the string in bytes, not chars - stringLength = Read7BitEncodedInt(); + int stringLength = Read7BitEncodedInt(); if (stringLength < 0) { throw new IOException(SR.Format(SR.IO_InvalidStringLen_Len, stringLength)); @@ -273,12 +267,13 @@ public virtual string ReadString() Span charBytes = stackalloc byte[MaxCharBytesSize]; + int currPos = 0; StringBuilder? sb = null; do { - readLength = Math.Min(MaxCharBytesSize, stringLength - currPos); + int readLength = Math.Min(MaxCharBytesSize, stringLength - currPos); - n = _stream.Read(charBytes[..readLength]); + int n = _stream.Read(charBytes[..readLength]); if (n == 0) { ThrowHelper.ThrowEndOfFileException(); @@ -292,7 +287,7 @@ public virtual string ReadString() _decoder ??= _encoding.GetDecoder(); _charBuffer ??= new char[_maxCharsSize]; - charsRead = _decoder.GetChars(charBytes[..n], _charBuffer, flush: false); + int charsRead = _decoder.GetChars(charBytes[..n], _charBuffer, flush: false); // Since we could be reading from an untrusted data source, limit the initial size of the // StringBuilder instance we're about to get or create. It'll expand automatically as needed. From e212ed6166d0a70d8926b1bcac1dfafc720fb072 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Fri, 4 Aug 2023 12:55:30 +0300 Subject: [PATCH 15/15] Address review feedback. --- .../System.Private.CoreLib/src/Resources/Strings.resx | 3 --- .../System.Private.CoreLib/src/System/IO/BinaryReader.cs | 3 --- 2 files changed, 6 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx index bd016b716a1617..554baa6bf1a004 100644 --- a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx +++ b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx @@ -1732,9 +1732,6 @@ Larger than collection size. - - The number of bytes requested does not fit into BinaryReader's internal buffer. - Argument must be between {0} and {1}. diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs index dd1cddecb5e689..37e0d1ffebf05c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs @@ -494,9 +494,6 @@ protected virtual void FillBuffer(int numBytes) } break; case 1: - // Need to find a good threshold for calling ReadByte() repeatedly - // vs. calling Read(byte[], int, int) for both buffered & unbuffered - // streams. n = _stream.ReadByte(); if (n == -1) {