Skip to content

Avoid OutOfMemoryException in some test environments - #45710

Merged
steveharter merged 5 commits into
dotnet:masterfrom
steveharter:ABWTest
Jan 7, 2021
Merged

Avoid OutOfMemoryException in some test environments#45710
steveharter merged 5 commits into
dotnet:masterfrom
steveharter:ABWTest

Conversation

@steveharter

Copy link
Copy Markdown
Contributor

Fixes #43932

@steveharter steveharter added this to the 6.0.0 milestone Dec 7, 2020
@steveharter steveharter self-assigned this Dec 7, 2020
Comment on lines +109 to +111
Memory<byte> memory;
try
{

@ahsonkhan ahsonkhan Dec 7, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If allocating ~2 GB is problematic, could ~1 GB also be an issue? In that case, it might be better to put the try/catch around the whole test, including the initialization of ABW and the first call to Advance.

Alternatively, maybe we should just opt to skip the test for the specific constrained devices where 1-2GB allocation is problematic (with some kind of conditional skip attribute similar to ConditionalFact(nameof(IsX64)), if possible). How much RAM is available on the Windows.Nano test machines?

@steveharter steveharter Dec 14, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Perhaps 1GB could cause issues as well; I see other JSON inner loop tests that allocate ~300K contiguous but not up to 1GB.

}

// Validate > MaxArrayLength.
output = new ArrayBufferWriter<byte>(1);

@ahsonkhan ahsonkhan Dec 15, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is changing the semantics of the test. We should consider leaving it as is by removing this and moving the call to GetMemory below into the try-catch, or if we find this test case useful, keeping a call to GetMemory in the try-catch in addition to this.


// 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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

After successfully setting ABW to init capacity, verify that output.GetMemory(int.max) fails.

Suggested change
Assert.Equal(MaxArrayLength - initialCapacity, memory.Length);
Assert.Equal(MaxArrayLength - initialCapacity, memory.Length);
Assert.Throws<OutOfMemoryException>(() => output.GetMemory(int.MaxValue));

@@ -1,4 +1,8 @@
Microsoft Visual Studio Solution File, Format Version 12.00


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Revert unrelated changes to this .sln file?

@ahsonkhan ahsonkhan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Otherwise, looks good.

@steveharter
steveharter merged commit 5175f6d into dotnet:master Jan 7, 2021
@steveharter
steveharter deleted the ABWTest branch January 7, 2021 23:54
@ghost ghost locked as resolved and limited conversation to collaborators Feb 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test failure: System.Buffers.Tests.ArrayBufferWriterTests_Byte.GetMemory_ExceedMaximumBufferSize

2 participants