Fix the OOM error in scripting tests #75163

jaredpar · 2024-09-19T04:45:29Z

Was able to reproduce the Scripting OutOfMemoryException issues on my local machine. After looking at the test run in PerfView it was clear that we were allocating a lot more memory than necessary and that was, likey, the cause of the OOM that we saw. Given that it was running under x86 this seems fairly likely.

The allocations were coming more from the ResultsProvider and IOperation test assemlbies. But given that scripting is small our scheduling algorithm often puts scripting into a slice that includes both these test assemblies. Their allocations are what is pushing us to OOM. That is also likely why we couldn't reproduce this by just running the scripting tests locally.

This PR attempts to address the cases that I saw. There are a few cases where I moved eager operations to lazy. The bigger change though is moving a to using an interpolated string handler for RoslynDebug.Assert. There are a number of cases where had roughly the following code:

Debug.Assert(condition, $"... {syntaxNode} ...");

On net472 that is going to eagerly allocate the message string which realizes the entire syntax node. One assert alone was contributing 9GB of allocations all by itself. Did a simple regex search for Debug.Assert that had an interpolation and moved them in mass to RoslynDebug.Assert. Now the strings are only realized if the assert fails. Will discuss with the team if we want to think about an analyzer here

PS: if the scripting tests OOM on this PR I may take tomorrow off.

Was able to reproduce the Scripting `OutOfMemoryException` issues on my local machine. After looking at the test run in PerfView it was clear that we were allocating a lot more memory than necessary and that was, likey, the cause of the OOM that we saw. Given that it was running under x86 this seems fairly likely. The allocations were coming more from the ResultsProvider and IOperation test assemlbies. But given that scripting is small our scheduling algorithm often puts scripting into a slice that includes both these test assemblies. Their allocations are what is pushing us to OOM. That is also likely why we couldn't reproduce this by just running the scripting tests locally. This PR attempts to address the cases that I saw. There are a few cases where I moved eager operations to lazy. The bigger change though is moving a to using an interpolated string handler for `RoslynDebug.Assert`. There are a number of cases where had roughly the following code: ```csharp Debug.Assert(condition, $"... {syntaxNode} ..."); ``` On net472 that is going to eagerly allocate the message string which realizes the entire syntax node. One assert alone was contributing 9GB of allocations all by itself. Did a simple regex search for `Debug.Assert` that had an interpolation and moved them in mass to `RoslynDebug.Assert`. Now the strings are only realized if the assert fails. Will discuss with the team if we want to think about an analyzer here PS: if the scripting tests OOM on this PR I may take tomorrow off.

jaredpar · 2024-09-19T04:46:19Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/Conversion.cs

@@ -252,7 +252,7 @@ private static void AssertTrivialConversion(ConversionKind kind)
                    break;
            }

-            Debug.Assert(isTrivial, "this conversion needs additional data: " + kind);
+            RoslynDebug.Assert(isTrivial, $"this conversion needs additional data: {kind}");


To emphasize how much these changes matter: this Debug.Assert alone was causing 50MB of allocations in the slice I was debugging.

jaredpar · 2024-09-19T04:47:11Z

src/ExpressionEvaluator/Core/Test/ResultProvider/Debugger/Engine/DkmClrType.cs

@@ -44,7 +44,9 @@ internal DkmClrType(DkmClrModuleInstance module, DkmClrAppDomain appDomain, Type
            ModuleInstance = module;
            AppDomain = appDomain;
            _lmrType = lmrType;
-            _evalAttributes = GetEvalAttributes(lmrType);
+            _lazyEvalAttributes = new System.Lazy<ReadOnlyCollection<DkmClrEvalAttribute>>(
+                () => GetEvalAttributes(lmrType),


GetEvalAttribute realizes a huge number of string and arrays inside the reflection libraries. About 150MB of savings from this change.

jaredpar · 2024-09-19T04:48:02Z

src/Compilers/Core/Portable/Operations/OperationMapBuilder.cs

+                    RoslynDebug.Assert(
+                        !argument.ContainsKey(operation.Syntax),
+                        $"Duplicate operation node for {operation.Syntax}. Existing node is {(argument.TryGetValue(operation.Syntax, out var original) ? (OperationKind?)original.Kind : null)}, new node is {operation.Kind}.");


About 9GB of allocations mostly due to the {operation.Syntax} portion.

tmat · 2024-09-19T04:49:18Z

Nice!

jaredpar · 2024-09-19T05:18:26Z

src/Compilers/Core/Portable/InternalUtilities/Debug.cs

+
+        /// <inheritdoc cref="Debug.Assert(bool, string)"/>
+        [Conditional("DEBUG")]
+        public static void Assert([DoesNotReturnIf(false)] bool condition, [InterpolatedStringHandlerArgument(nameof(condition))] ref AssertInterpolatedStringHandler message)


It's a static method so can't polyfill until we get extension everything 😦

333fred · 2024-09-19T06:07:35Z

src/Compilers/Core/Portable/InternalUtilities/Debug.cs

+            }
+
+            internal string ToStringAndClear() => _builder is null
+                ? ""


Nit: seems like this should never be hit. Consider just throwing.

jjonescz · 2024-09-19T08:33:31Z

src/Compilers/Core/Portable/InternalUtilities/Debug.cs


        /// <inheritdoc cref="Debug.Assert(bool, string)"/>
        [Conditional("DEBUG")]
-        public static void Assert([DoesNotReturnIf(false)] bool b, string message)
-            => Debug.Assert(b, message);
+        public static void Assert([DoesNotReturnIf(false)] bool condition, string message)


Can this overload be removed?

@333fred as I'm not sure about the implications here of doing this.

No, a regular string won't work with the handler.

cston · 2024-09-19T15:22:55Z

src/Compilers/Core/Portable/InternalUtilities/Debug.cs

+                }
+            }
+
+            internal string ToStringAndClear() => _builder!.ToString();


_builder!.ToString();

Should this call _builder.Clear() as well?

AlekseyTs

LGTM (commit 3)

jaredpar · 2024-09-19T17:06:05Z

Sigh .... Still some more work to do here.

@jaredpar

* Add an analyzer for Debug.Assert As @jaredpar found in dotnet/roslyn#75163, interpolated strings in `Debug.Assert` can consume a surprising amount of memory. On modern .NET, this is fine; `Debug.Assert` has an interpolated string handler that will avoid the allocations if the assert isn't triggered. However, on our framework tests, this can be very bad and OOM our tests. So, this analyzer looks for cases where interpolated strings are passed to `Debug.Assert`, and recommends moving over to `RoslynDebug.Assert` instead, which is an interpolated string handler on all platforms. Note that I only did C# support, as there's no equivalent handler API for VB. * Make sure RoslynDebug is available in one test. * Trailing whitespace * Run pack. * Remove VB * Pack again * Use batch fixall provider * Avoid passing empty string as command line argument See microsoft/dotnet#1062 * Don't warn for constant strings. * Apply suggestions from code review Co-authored-by: Jared Parsons <jared@paranoidcoding.org> --------- Co-authored-by: Sam Harwell <Sam.Harwell@microsoft.com> Co-authored-by: Jared Parsons <jared@paranoidcoding.org>

jaredpar requested review from a team as code owners September 19, 2024 04:45

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 19, 2024

dotnet-policy-service bot added the VSCode label Sep 19, 2024

jaredpar commented Sep 19, 2024

View reviewed changes

333fred approved these changes Sep 19, 2024

View reviewed changes

jjonescz approved these changes Sep 19, 2024

View reviewed changes

pr feedback

7553572

jaredpar enabled auto-merge (squash) September 19, 2024 15:16

cston reviewed Sep 19, 2024

View reviewed changes

oops

741a8bd

cston approved these changes Sep 19, 2024

View reviewed changes

AlekseyTs approved these changes Sep 19, 2024

View reviewed changes

333fred mentioned this pull request Sep 19, 2024

Add an analyzer for Debug.Assert dotnet/roslyn-analyzers#7416

Merged

jaredpar disabled auto-merge September 19, 2024 17:54

jaredpar merged commit 2b40221 into dotnet:main Sep 19, 2024
26 of 28 checks passed

jaredpar deleted the oom branch September 19, 2024 17:54

dotnet-policy-service bot added this to the Next milestone Sep 19, 2024

This was referenced Sep 23, 2024

[Automated] PRs inserted in VS build main-35322.09 #75206

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35325.131 #75248

Closed

akhera99 modified the milestones: Next, 17.12 P3 Sep 26, 2024

dotnet-bot mentioned this pull request Oct 1, 2024

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35330.159 #75309

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the OOM error in scripting tests #75163

Fix the OOM error in scripting tests #75163

jaredpar commented Sep 19, 2024

jaredpar Sep 19, 2024

jaredpar Sep 19, 2024

jaredpar Sep 19, 2024

tmat commented Sep 19, 2024

jaredpar Sep 19, 2024

333fred Sep 19, 2024

jjonescz Sep 19, 2024

jaredpar Sep 19, 2024

333fred Sep 19, 2024

cston Sep 19, 2024

AlekseyTs left a comment

jaredpar commented Sep 19, 2024

Fix the OOM error in scripting tests #75163

Fix the OOM error in scripting tests #75163

Conversation

jaredpar commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

jaredpar commented Sep 19, 2024