Factors out few GC specific tests #29647

marek-safar · 2018-05-10T22:35:29Z

No description provided.

jkotas · 2018-05-11T01:49:03Z

src/CoreFx.Private.TestUtilities/src/System/PlatformDetection.cs

@@ -41,6 +41,8 @@ public static partial class PlatformDetection

        public static bool IsDomainJoinedMachine => !Environment.MachineName.Equals(Environment.UserDomainName, StringComparison.OrdinalIgnoreCase);

+        public static bool IsNotGCForVeryLargeObjects => true;


What is the definition of the capability that this is controlling?

It should be something like https://docs.microsoft.com/en-us/dotnet/framework/configure-apps/file-schema/runtime/gcallowverylargeobjects-element on netcore

On .NET Framework, these tests will pass both with gcAllowVeryLargeObjects=false and gcAllowVeryLargeObjects=true. So it must be something else...

I wrote like I don't know what blocks coreclr to allocate collections with more than int.MaxValue items, happy to tweak the name to fit that

.NET Framework/Core have documented limits on maximum array sizes. From https://docs.microsoft.com/en-us/dotnet/framework/configure-apps/file-schema/runtime/gcallowverylargeobjects-element :

The maximum index in any single dimension is 2,147,483,591 (0x7FFFFFC7) for byte arrays and arrays of single-byte structures, and 2,146,435,071 (0X7FEFFFFF) for other types.

These limits are less than Int32.MaxValue for defense-in-depth security (to reduce changes of integer overflows causing security bugs), and to allow certain types of bounds-check elimination optimizations and loop unrolling (the JIT can assume that array.Length + small_constant won't overflow).

Thanks, renamed the property. I didn't know coreclr throws OutOfMemory when indexing into an already allocated array.

I didn't know coreclr throws OutOfMemory when indexing into an already allocated array.

OutOfMemory exception is thrown by new when the array is allocated , not when you index into it.

@jkotas I have to say this is getting more confusing as the documentation seems to be .NET Framework specific.

var a = new byte[(uint)int.MaxValue + 1]; // This throws System.OverflowException Array dimensions exceeded supported range var b = new byte[int.MaxValue - 100]; Array.Resize (ref b, int.MaxValue); // This throws OutOfMemoryException

Shouldn't the first one just work, coreclr has LargeObjects enabled by default since dotnet/coreclr#8853 and you said

OutOfMemory exception is thrown by new when the array is allocated , not when you index into it.

The second one is why the tests have OOM expectation but should that throw the exception with Array dimensions exceeded supported range instead?

The behavior of new T[len] is:

0 <= len && len <= MAX_LENGTH - Succeeds, assuming there is enough memory

MAX_LENGTH < len && len <= Int32.MaxValue - throw OutOfMemoryException.

len < 0 || len > Int32.MaxValue - throw OverflowException

On .NET Core, and on .NET Framework with gcAllowVeryLargeObjects=true, MAX_LENGTH is sizeof(T) > 1 ? 0X7FEFFFFF : 0x7FFFFFC7
On .NET Framework with gcAllowVeryLargeObjects=false, MAX_LENGTH is approximately 0x7FFFFFC7 / sizeof(T) .

jkotas · 2018-05-11T15:10:07Z

src/CoreFx.Private.TestUtilities/src/System/PlatformDetection.cs

@@ -73,6 +73,8 @@ private static bool GetIsWindowsSubsystemForLinux()
            return false;
        }

+        public static bool IsIntMaxValueArrayIndexNotSupported => true;


It is better to use positive names (XXXSupported instead of XXXNotSupported) for readability.

I agree but xunit conditionals don't support negating syntax (you have Not everywhere)

Nit, but I believe when we do this we consistently begin with IsNot...

jkotas · 2018-05-11T15:10:42Z

src/CoreFx.Private.TestUtilities/src/System/PlatformDetection.cs

@@ -73,6 +73,8 @@ private static bool GetIsWindowsSubsystemForLinux()
            return false;
        }

+        public static bool IsIntMaxValueArrayIndexNotSupported => true;


It would be nice to auto-detect the right value for this, similar to how IsNonZeroLowerBoundArraySupported is auto-detected.

I could add that but what's the coreclr way of checking that for this behaviour? Based on my comment above I could catch System.OverflowException but that seems quite odd to me.

jkotas · 2018-05-12T20:56:18Z

@dotnet-bot test NETFX x86 Release Build please

jkotas · 2018-05-12T21:40:56Z

src/CoreFx.Private.TestUtilities/src/System/PlatformDetection.cs

@@ -73,6 +73,20 @@ private static bool GetIsWindowsSubsystemForLinux()
            return false;
        }

+        static Lazy<bool> s_largeArrayIsNotSupported = new Lazy<bool>(() => {


Nit: I think the corefx convention would be to put { on a new line.

jkotas · 2018-05-12T21:42:02Z

src/CoreFx.Private.TestUtilities/src/System/PlatformDetection.cs

+        static Lazy<bool> s_largeArrayIsNotSupported = new Lazy<bool>(() => {
+            try
+            {
+                var tmp = new byte[int.MaxValue];


Some codegens may optimize this allocation out. I may be useful to consume the value in a way that is unlikely to be optimized out. E.g. call ToString() on it.

jkotas · 2018-05-12T21:43:06Z

LGTM otherwise

jkotas · 2018-05-14T14:14:01Z

src/CoreFx.Private.TestUtilities/src/System/PlatformDetection.cs

+            try
+            {
+                var tmp = new byte[int.MaxValue];
+                return tmp == null;


This should to be a black box operation that the optimizations are not able to eliminate. tmp == null can be optimized out easily.

FWIW, RyuJIT does some of these optimizations today already. For example, if you have:

class Test { static public bool Method() { Test t = new Test(); return (t != null); } }

It will get optimized to just return true.

Would [MethodImpl(MethodImplOptions.NoOptimization)] help?

Yes, NoOptimization is probably the most appropriate solution.

stephentoub · 2018-05-14T21:54:03Z

src/CoreFx.Private.TestUtilities/src/System/PlatformDetection.cs

+            }
+        }
+
+        public static bool IsNotIntMaxValueArrayIndexSupported => s_largeArrayIsNotSupported.Value;


What you have is fine, but does it need to use lazy? I'm wondering if it can just be:

public static bool IsNotIntMaxValueArrayIndexSupported { get; } = IsLargeArrayIsNotSupported();

or if there's a reason doing that in the static cctor is a bad idea?

It could be but allocating 2GB of memory when any of other values are read from the type does not seem to me like a good idea

Ah, this is in the PlatformDetection class; in that case, I agree. I was thinking it was on the test class itself, in which case such a benefit wouldn't exist.

stephentoub · 2018-05-15T12:53:38Z

src/CoreFx.Private.TestUtilities/src/System/PlatformDetection.cs

@@ -73,6 +74,24 @@ private static bool GetIsWindowsSubsystemForLinux()
            return false;
        }

+        static Lazy<bool> s_largeArrayIsNotSupported = new Lazy<bool>(IsLargeArrayIsNotSupported);


Nit: private

stephentoub · 2018-05-15T12:53:57Z

src/CoreFx.Private.TestUtilities/src/System/PlatformDetection.cs

+        static Lazy<bool> s_largeArrayIsNotSupported = new Lazy<bool>(IsLargeArrayIsNotSupported);
+
+        [MethodImpl(MethodImplOptions.NoOptimization)]
+        static bool IsLargeArrayIsNotSupported()


Nit: private

stephentoub

Other than my few remaining nits, LGTM.

* Factors out few GC specific tests * Tweaks to address the review Commit migrated from dotnet/corefx@ae9c360

jkotas reviewed May 11, 2018

View reviewed changes

marek-safar force-pushed the oom branch from 658f2a7 to fdeeead Compare May 11, 2018 15:01

jkotas reviewed May 11, 2018

View reviewed changes

marek-safar force-pushed the oom branch 2 times, most recently from 01a1491 to 73dba83 Compare May 11, 2018 23:26

Factors out few GC specific tests

ebdc077

marek-safar force-pushed the oom branch from 73dba83 to ebdc077 Compare May 11, 2018 23:58

karelz added the area-System.Runtime label May 12, 2018

karelz assigned marek-safar and jkotas May 12, 2018

jkotas reviewed May 12, 2018

View reviewed changes

jkotas reviewed May 14, 2018

View reviewed changes

marek-safar force-pushed the oom branch 2 times, most recently from ea55cf0 to b630e9f Compare May 14, 2018 15:16

stephentoub reviewed May 14, 2018

View reviewed changes

stephentoub reviewed May 15, 2018

View reviewed changes

stephentoub approved these changes May 15, 2018

View reviewed changes

Tweaks to address the review

4b7cde9

marek-safar force-pushed the oom branch from b630e9f to 4b7cde9 Compare May 15, 2018 13:40

stephentoub merged commit ae9c360 into dotnet:master May 15, 2018

karelz added this to the 3.0 milestone Jun 2, 2018

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Factors out few GC specific tests (dotnet/corefx#29647)

766a364

* Factors out few GC specific tests * Tweaks to address the review Commit migrated from dotnet/corefx@ae9c360

		@@ -41,6 +41,8 @@ public static partial class PlatformDetection

		public static bool IsDomainJoinedMachine => !Environment.MachineName.Equals(Environment.UserDomainName, StringComparison.OrdinalIgnoreCase);

		public static bool IsNotGCForVeryLargeObjects => true;

Factors out few GC specific tests #29647

Factors out few GC specific tests #29647

Uh oh!

Conversation

marek-safar commented May 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented May 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented May 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkotas May 11, 2018 •

edited

Loading

jkotas May 11, 2018 •

edited

Loading