From 73ee4c7ed9cf0ac51c4032686bd5193f6e737bfe Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Thu, 1 Oct 2020 09:49:58 -0500 Subject: [PATCH 1/3] Improve perf of allocations between 1GB and 2GB --- .../src/System/Buffers/ArrayBufferWriter.cs | 15 +++++++++++++-- .../ArrayBufferWriterTests.Byte.cs | 10 ++++++++-- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs b/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs index b6271ba17e09b3..fc9699a4ddf294 100644 --- a/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs +++ b/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs @@ -167,6 +167,8 @@ private void CheckAndResizeBuffer(int sizeHint) if (sizeHint > FreeCapacity) { int currentLength = _buffer.Length; + + // Attempt to grow by the larger of the minimum size and double the current size. int growBy = Math.Max(sizeHint, currentLength); if (currentLength == 0) @@ -178,10 +180,19 @@ private void CheckAndResizeBuffer(int sizeHint) if ((uint)newSize > int.MaxValue) { - newSize = currentLength + sizeHint; + // Attempt to grow by the larger of the minimum size and half of the available size. + growBy = Math.Max(sizeHint, (int.MaxValue - currentLength) / 2 + 1); + newSize = currentLength + growBy; + if ((uint)newSize > int.MaxValue) { - ThrowOutOfMemoryException((uint)newSize); + // Attempt to grow by the minimum size. + newSize = currentLength + sizeHint; + + if ((uint)newSize > int.MaxValue) + { + ThrowOutOfMemoryException((uint)newSize); + } } } diff --git a/src/libraries/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.Byte.cs b/src/libraries/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.Byte.cs index 7583b9a60c448f..6173106ab01e4c 100644 --- a/src/libraries/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.Byte.cs +++ b/src/libraries/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.Byte.cs @@ -100,8 +100,14 @@ public void GetMemory_ExceedMaximumBufferSize() { var output = new ArrayBufferWriter(int.MaxValue / 2 + 1); output.Advance(int.MaxValue / 2 + 1); - Memory memory = output.GetMemory(1); // Validate we can't double the buffer size, but can grow by sizeHint - Assert.Equal(1, memory.Length); + + // Validate we can't double the buffer size, but can grow + Memory memory = output.GetMemory(1); + + // The buffer should grow more than the 1 byte requested otherwise performance will not + // be usable between 1GB and 2GB + Assert.True(memory.Length > 1); + Assert.Throws(() => output.GetMemory(int.MaxValue)); } } From d56d39a98677617622c35a69ba9d7a8372b0b8b6 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Fri, 2 Oct 2020 08:57:00 -0500 Subject: [PATCH 2/3] Once half full, max out with MaxArrayLength --- .../src/System/Buffers/ArrayBufferWriter.cs | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs b/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs index fc9699a4ddf294..928e779e138198 100644 --- a/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs +++ b/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs @@ -15,10 +15,14 @@ namespace System.Buffers #endif sealed class ArrayBufferWriter : IBufferWriter { + // Copy of Array.MaxArrayLength. For byte arrays the limit is slightly larger + private const int MaxArrayLength = 0X7FEFFFFF; + + private const int DefaultInitialBufferSize = 256; + private T[] _buffer; private int _index; - private const int DefaultInitialBufferSize = 256; /// /// Creates an instance of an , in which data can be written to, @@ -168,7 +172,7 @@ private void CheckAndResizeBuffer(int sizeHint) { int currentLength = _buffer.Length; - // Attempt to grow by the larger of the minimum size and double the current size. + // Attempt to grow by the larger of the sizeHint and double the current size. int growBy = Math.Max(sizeHint, currentLength); if (currentLength == 0) @@ -180,20 +184,15 @@ private void CheckAndResizeBuffer(int sizeHint) if ((uint)newSize > int.MaxValue) { - // Attempt to grow by the larger of the minimum size and half of the available size. - growBy = Math.Max(sizeHint, (int.MaxValue - currentLength) / 2 + 1); - newSize = currentLength + growBy; + // Attempt to grow to MaxArrayLength. + uint needed = (uint)(currentLength - FreeCapacity + sizeHint); - if ((uint)newSize > int.MaxValue) + if (needed > MaxArrayLength) { - // Attempt to grow by the minimum size. - newSize = currentLength + sizeHint; - - if ((uint)newSize > int.MaxValue) - { - ThrowOutOfMemoryException((uint)newSize); - } + ThrowOutOfMemoryException(needed); } + + newSize = MaxArrayLength; } Array.Resize(ref _buffer, newSize); From 8262b3026cf4b4e4ef98badb5ee7fb038bf16324 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Tue, 6 Oct 2020 11:56:24 -0500 Subject: [PATCH 3/3] Fix STJ behavior\test and address feedback --- .../Common/src/System/Buffers/ArrayBufferWriter.cs | 1 + .../ArrayBufferWriterTests.Byte.cs | 14 +++++++++----- .../src/System/Text/Json/JsonHelpers.cs | 12 ++++++++++++ .../src/System/Text/Json/ThrowHelper.cs | 6 ++++++ .../src/System/Text/Json/Writer/Utf8JsonWriter.cs | 5 ++++- .../System.Text.Json/tests/Utf8JsonWriterTests.cs | 11 ++++++----- 6 files changed, 38 insertions(+), 11 deletions(-) diff --git a/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs b/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs index 928e779e138198..905e9f804abf6a 100644 --- a/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs +++ b/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs @@ -186,6 +186,7 @@ private void CheckAndResizeBuffer(int sizeHint) { // Attempt to grow to MaxArrayLength. uint needed = (uint)(currentLength - FreeCapacity + sizeHint); + Debug.Assert(needed > currentLength); if (needed > MaxArrayLength) { diff --git a/src/libraries/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.Byte.cs b/src/libraries/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.Byte.cs index 6173106ab01e4c..8b079c8e71f895 100644 --- a/src/libraries/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.Byte.cs +++ b/src/libraries/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.Byte.cs @@ -98,15 +98,19 @@ public async Task WriteAndCopyToStreamAsync() [OuterLoop] public void GetMemory_ExceedMaximumBufferSize() { - var output = new ArrayBufferWriter(int.MaxValue / 2 + 1); - output.Advance(int.MaxValue / 2 + 1); + const int MaxArrayLength = 0X7FEFFFFF; + + int initialCapacity = int.MaxValue / 2 + 1; + + var output = new ArrayBufferWriter(initialCapacity); + output.Advance(initialCapacity); // Validate we can't double the buffer size, but can grow Memory memory = output.GetMemory(1); - // The buffer should grow more than the 1 byte requested otherwise performance will not - // be usable between 1GB and 2GB - Assert.True(memory.Length > 1); + // The buffer should grow more than the 1 byte requested otherwise performance will not be usable + // between 1GB and 2GB. The current implementation maxes out the buffer size to MaxArrayLength. + Assert.Equal(MaxArrayLength - initialCapacity, memory.Length); Assert.Throws(() => output.GetMemory(int.MaxValue)); } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.cs b/src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.cs index 813627a7d6877b..bcdbdf04010298 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.cs @@ -11,6 +11,9 @@ namespace System.Text.Json { internal static partial class JsonHelpers { + // Copy of Array.MaxArrayLength. For byte arrays the limit is slightly larger + private const int MaxArrayLength = 0X7FEFFFFF; + /// /// Returns the span for the given reader. /// @@ -143,5 +146,14 @@ public static bool IsValidNumberHandlingValue(JsonNumberHandling handling) => JsonNumberHandling.AllowReadingFromString | JsonNumberHandling.WriteAsString | JsonNumberHandling.AllowNamedFloatingPointLiterals)); + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static void ValidateInt32MaxArrayLength(uint length) + { + if (length > MaxArrayLength) + { + ThrowHelper.ThrowOutOfMemoryException(length); + } + } } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.cs b/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.cs index 1e4c5280eb7b44..f51b9e7a1bc6d2 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.cs @@ -506,6 +506,12 @@ public static InvalidOperationException GetInvalidOperationException(ExceptionRe return ex; } + [DoesNotReturn] + public static void ThrowOutOfMemoryException(uint capacity) + { + throw new OutOfMemoryException(SR.Format(SR.BufferMaximumSizeExceeded, capacity)); + } + // This function will convert an ExceptionResource enum value to the resource string. [MethodImpl(MethodImplOptions.NoInlining)] private static string GetResourceString(ExceptionResource resource, int currentDepth, byte token, JsonTokenType tokenType) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.cs index 196e8d2d29aea2..2c5cc11afbd7a1 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.cs @@ -1004,7 +1004,10 @@ private void Grow(int requiredSize) { Debug.Assert(_arrayBufferWriter != null); - _memory = _arrayBufferWriter.GetMemory(checked(BytesPending + sizeHint)); + int needed = BytesPending + sizeHint; + JsonHelpers.ValidateInt32MaxArrayLength((uint)needed); + + _memory = _arrayBufferWriter.GetMemory(needed); Debug.Assert(_memory.Length >= sizeHint); } diff --git a/src/libraries/System.Text.Json/tests/Utf8JsonWriterTests.cs b/src/libraries/System.Text.Json/tests/Utf8JsonWriterTests.cs index 9e966bf531c4a0..f4c46445333921 100644 --- a/src/libraries/System.Text.Json/tests/Utf8JsonWriterTests.cs +++ b/src/libraries/System.Text.Json/tests/Utf8JsonWriterTests.cs @@ -744,16 +744,17 @@ public void WriteLargeJsonToStreamWithoutFlushing() } Assert.Equal(150_097_503, writer.BytesPending); - for (int i = 0; i < 6; i++) + for (int i = 0; i < 13; i++) { writer.WriteStringValue(text3); } - Assert.Equal(1_050_097_521, writer.BytesPending); + Assert.Equal(2_100_097_542, writer.BytesPending); + + // Next write forces a grow beyond max array length - // Next write forces a grow beyond 2 GB Assert.Throws(() => writer.WriteStringValue(text3)); - Assert.Equal(1_050_097_521, writer.BytesPending); + Assert.Equal(2_100_097_542, writer.BytesPending); var text4 = JsonEncodedText.Encode(largeArray.AsSpan(0, 1)); for (int i = 0; i < 10_000_000; i++) @@ -761,7 +762,7 @@ public void WriteLargeJsonToStreamWithoutFlushing() writer.WriteStringValue(text4); } - Assert.Equal(1_050_097_521 + (4 * 10_000_000), writer.BytesPending); + Assert.Equal(2_100_097_542 + (4 * 10_000_000), writer.BytesPending); } }