Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce the amount of memory allocated by System.IO.Tests #66387

Merged
merged 4 commits into from
Mar 9, 2022

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Mar 9, 2022

Recently System.IO.Tests have started failing with OOM on Unix-like OSes. Since the CI has not produced any dump file and according to my knowledge we don't offer any memory profiler for Linux, I've used VS Memory Profiler to profile System.IO.Tests on Windows.

image

The number of live objects goes back to previous value (or a very similar number) after each GC collection, so I don't think that we have introduced a memory leak to the product itself.

However, one of the tests, namely WriteChars_VeryLargeArray_DoesNotOverflow allocated 6.5 GB and it has already been causing OOMs in the past as it's marked to skip on Android:

[SkipOnPlatform(TestPlatforms.Android, "OOM on Android could be uncatchable & kill the test runner")]
public unsafe void WriteChars_VeryLargeArray_DoesNotOverflow()

I guess (I have no memory dump from the CI) that the problem is caused by WriteChars_VeryLargeArray_DoesNotOverflow which allocates 6.5GB and causes other tests which run in parallel to fail with OOM.

My proposal:

  • allocate less memory to validate Int32Overflow, just slightly more than int.MaxValue
  • since we don't validate the buffers content (just the stream position) use the same memory for input and output
  • don't run other tests in parallel
  • throw SkipTestException on OOM so we can differentiate successful and skipped test

This is nicely reducing the amount of memory used by the System.IO.Tests and hopefully is still testing what author wanted to test (cc @GrabYourPitchforks)

image

fixes #65791

@adamsitnik adamsitnik added this to the 7.0.0 milestone Mar 9, 2022
@ghost ghost assigned adamsitnik Mar 9, 2022
@ghost
Copy link

ghost commented Mar 9, 2022

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Recently System.IO.Tests have started failing with OOM on Unix-like OSes. Since the CI has not produced any dump file and according to my knowledge we don't offer any memory profiler for Linux, I've used VS Memory Profiler to profile System.IO.Tests on Windows.

image

The number of live objects goes back to previous value (or a very similar number) after each GC collection, so I don't think that we have introduced a memory leak to the product itself.

However, one of the tests, namely WriteChars_VeryLargeArray_DoesNotOverflow allocated 6.5 GB and it has already been causing OOMs in the past as it's marked to skip on Android:

[SkipOnPlatform(TestPlatforms.Android, "OOM on Android could be uncatchable & kill the test runner")]
public unsafe void WriteChars_VeryLargeArray_DoesNotOverflow()

I guess (I have no memory dump from the CI) that the problem is caused by WriteChars_VeryLargeArray_DoesNotOverflow which allocates 6.5GB and causes other tests which run in parallel to fail with OOM.

My proposal:

  • allocate less memory to validate Int32Overflow, just slightly more than int.MaxValue
  • since we don't validate the buffers content, only stream position, use the same memory for input and output
  • don't run other tests in parallel
  • throw SkipTestException on OOM so we can differentiate successful and skipped test

This is nicely reducing the amount of memory used by the System.IO.Tests and hopefully is still testing what author wanted to test (cc @GrabYourPitchforks)

image

fixes #65791

Author: adamsitnik
Assignees: -
Labels:

area-System.IO

Milestone: 7.0.0

@adamsitnik
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop-linux

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
Copy link
Member Author

I've created new issues for the CI failures: #66395 #66396

@danmoseley
Copy link
Member

Thanks for taking care of this @adamsitnik

@ghost ghost locked as resolved and limited conversation to collaborators Apr 8, 2022
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.

System.IO.Tests work item failing with SIGKILL
3 participants