Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,14 @@ namespace System.Buffers
#endif
sealed class ArrayBufferWriter<T> : IBufferWriter<T>
{
// Copy of Array.MaxArrayLength. For byte arrays the limit is slightly larger
private const int MaxArrayLength = 0X7FEFFFFF;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use Array.MaxArrayLength, so we don't have this constant in multiple places?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is compiled in NS2.0 assemblies, so it needs to have a local copy. I agree it would be nice to have an API for this - reactivating #31366

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it matter on full framework where MaxArrayLength might be smaller (I believe not due to how the buffer grows, but I thought I'd double check)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it is not easy to find the maximum array length in .NET Framework. It depends on gcAllowVeryLargeObjects configuration setting. It just means that this code may throw OOM on .NET Framework even when it could have been avoided. I think that is fine.


private const int DefaultInitialBufferSize = 256;

private T[] _buffer;
private int _index;

private const int DefaultInitialBufferSize = 256;

/// <summary>
/// Creates an instance of an <see cref="ArrayBufferWriter{T}"/>, in which data can be written to,
Expand Down Expand Up @@ -167,6 +171,8 @@ private void CheckAndResizeBuffer(int sizeHint)
if (sizeHint > FreeCapacity)
{
int currentLength = _buffer.Length;

// Attempt to grow by the larger of the sizeHint and double the current size.
int growBy = Math.Max(sizeHint, currentLength);

if (currentLength == 0)
Expand All @@ -178,11 +184,16 @@ private void CheckAndResizeBuffer(int sizeHint)

if ((uint)newSize > int.MaxValue)
{
newSize = currentLength + sizeHint;
if ((uint)newSize > int.MaxValue)
// Attempt to grow to MaxArrayLength.
uint needed = (uint)(currentLength - FreeCapacity + sizeHint);
Debug.Assert(needed > currentLength);

if (needed > MaxArrayLength)
{
ThrowOutOfMemoryException((uint)newSize);
ThrowOutOfMemoryException(needed);
}

newSize = MaxArrayLength;
}

Array.Resize(ref _buffer, newSize);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,20 @@ public async Task WriteAndCopyToStreamAsync()
[OuterLoop]
public void GetMemory_ExceedMaximumBufferSize()
{
var output = new ArrayBufferWriter<byte>(int.MaxValue / 2 + 1);
output.Advance(int.MaxValue / 2 + 1);
Memory<byte> memory = output.GetMemory(1); // Validate we can't double the buffer size, but can grow by sizeHint
Assert.Equal(1, memory.Length);
const int MaxArrayLength = 0X7FEFFFFF;

int initialCapacity = int.MaxValue / 2 + 1;

var output = new ArrayBufferWriter<byte>(initialCapacity);
output.Advance(initialCapacity);

// Validate we can't double the buffer size, but can grow
Memory<byte> memory = output.GetMemory(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<OutOfMemoryException>(() => output.GetMemory(int.MaxValue));
}
}
Expand Down
12 changes: 12 additions & 0 deletions src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/// <summary>
/// Returns the span for the given reader.
/// </summary>
Expand Down Expand Up @@ -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);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1004,7 +1004,10 @@ private void Grow(int requiredSize)
{
Debug.Assert(_arrayBufferWriter != null);

_memory = _arrayBufferWriter.GetMemory(checked(BytesPending + sizeHint));
int needed = BytesPending + sizeHint;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was changed to keep the OOM exception semantics (instead of OverflowException). There are no effective perf differences since the overhead of the new if statement roughly equals the extra logic in using checked().

Note there are other cases in STJ that also use checked() that could be changed like this code, but it doesn't use ABW so not changed in this PR. I may create a new issue to track this if deemed useful.

Copy link
Contributor

@ahsonkhan ahsonkhan Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was changed to keep the OOM exception semantics (instead of OverflowException).

Add a test, if one doesn't already exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the existing test that was modified in this PR was failing due to this (not throwing OOM as expected).

JsonHelpers.ValidateInt32MaxArrayLength((uint)needed);

_memory = _arrayBufferWriter.GetMemory(needed);

Debug.Assert(_memory.Length >= sizeHint);
}
Expand Down
11 changes: 6 additions & 5 deletions src/libraries/System.Text.Json/tests/Utf8JsonWriterTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -744,24 +744,25 @@ 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<OutOfMemoryException>(() => 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++)
{
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);
}
}

Expand Down