Avoid scanning typeof checks when building whole program view #103883

MichalStrehovsky · 2024-06-24T07:49:06Z

Before this PR, we were somewhat able to eliminate dead typeof checks such as:

if (someType == typeof(Foo)
{
    ExpensiveMethod();
}

This work was done in #102248.

However, the optimization only happened during codegen. This meant that when building the whole program view, we'd still look at ExpensiveMethod and whatever damage this caused to the whole program view was permanent.

With this PR, the scanner now becomes aware of the optimization we do during codegen and tries to defer injecting dependencies until we will need them.

With this change, we detect the conditional branch, and generate whatever dependencies from the basic block as conditional. That way scanning can fully skip scanning ExpensiveMethod and the subsequent optimization will ensure the missed scanning will not cause issues at codegen time.

dotnet-policy-service · 2024-06-24T07:49:33Z

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

MichalStrehovsky · 2024-06-24T11:07:34Z

/azp run runtime-nativeaot-outerloop

azure-pipelines · 2024-06-24T11:07:51Z

Azure Pipelines successfully started running 1 pipeline(s).

MichalStrehovsky · 2024-06-24T12:43:22Z

I wish all test failures were like this:

      ****************************************************
      * Size test                                        *
      * Size of the executable is   1,262 kB             *
      ****************************************************
      BUG: File size is not in the expected range (1331200 to 1945600 bytes). Did a libraries change regress size of Hello World?

Before this PR, we were somewhat able to eliminate dead typeof checks such as: ```csharp if (someType == typeof(Foo) { ExpensiveMethod(); } ``` This work was done in dotnet#102248. However, the optimization only happened during codegen. This meant that when building the whole program view, we'd still look at `ExpensiveMethod` and whatever damage this caused to the whole program view was permanent. With this PR, the scanner now becomes aware of the optimization we do during codegen and tries to defer injecting dependencies until we will need them. With this change, we detect the conditional branch, and generate whatever dependencies from the basic block as conditional. That way scanning can fully skip scanning `ExpensiveMethod` and the subsequent optimization will ensure the missed scanning will not cause issues at codegen time.

MichalStrehovsky · 2024-07-01T07:43:24Z

/azp run runtime-nativeaot-outerloop

azure-pipelines · 2024-07-01T07:43:34Z

Azure Pipelines successfully started running 1 pipeline(s).

MichalStrehovsky · 2024-07-01T07:48:19Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/ShadowConcreteMethodNode.cs

This node represents a concrete instantiation of a method that has a shared method body. We only look at the method body once, but take note of all dependencies that are per-instantiation.

For example, when we compile static void Foo<T>() => typeof(T), the method body of Foo<__Canon> would say "I also depend on the MethodTable of T____Canon after substitution. If this compilation also contain a generic dictionary for Foo<Atom> (where Atom is a reference type), this is the node that would say "please generate a MethodTable for Atom - we do that in the existing GetStaticDependencies.

What this node was missing was reporting of conditional dependencies - if the canonical method body conditionally depends on something, we need to do the same thing as GetStaticDependencies, but also replicate the condition from the canonical method.

What this node was missing was reporting of conditional dependencies

Could you elaborate slightly? When you say "conditional dependency" here are you thinking of dependencies literally in conditions? That is, given the following code

void M<T>() { if (typeof(T) == typeof(int)) { ... } }

There is nominally a "conditional" dependency in M st., if T is instantiated as int, that block is necessary. Otherwise, that block is dead code.

Is that what this change handles?

Actually, you said that "This node represents a concrete instantiation of a method that has a shared method body.", meaning that this node is only present for reference types, right?

Actually, you said that "This node represents a concrete instantiation of a method that has a shared method body.", meaning that this node is only present for reference types, right?

Yes, this is only for instantiations over reference types.

Let's say we have:

class Foo<T> { public Type Blah() => typeof(T); public Type Blah2() => typeof(T[,,,]); } // And we do: new Foo<object>().Blah();

Canonical method Blah depends on e.g. T__Canon (for the typeof(T)). This by itself is rather useless for the dependency analysis system.

We don't know what Blah2 depends on since it was not called and got "trimmed".

Foo<object> MethodTable conditionally depends on ShadowConcreteMethod (the node we're discussing) of Foo<object>.Blah and Foo<object>.Blah2, the condition is the presence of the canonical method body (so for the former, the condition is satisfied, for the latter it isn't since the canonical body was not looked at).

When evaluating the dependencies of the ShadowConcreteMethod we look at the dependencies of the canonical method and specialize as necessary

The extension in this PR is to ensure we also look at the conditional dependencies of the canonical method, not just the unconditional ones.

Could you give an example of a conditional dependency of ShadowConcreteMethod?

MichalStrehovsky · 2024-07-01T07:52:26Z

src/coreclr/tools/Common/TypeSystem/IL/ILImporter.cs

@@ -301,12 +301,17 @@ private void ImportBasicBlocks()
        }

        private void MarkBasicBlock(BasicBlock basicBlock)
+        {
+            MarkBasicBlock(basicBlock, ref _pendingBasicBlocks);


I'm adding the notion of additional lists that we should look at. This helps in the conditional basic block scanning because it lets us defer looking at conditional blocks until after we scanned all the unconditional codepaths. The conditional scanning is rather simplistic and for:

static int Method() { if (Foo() == typeof(Bla)) Expensive(); return 0; }

We'd see the Expensive block is conditioned, then we'd make the return 0 conditional as well because the control falls through, and then we'd find out that return 0 is also reachable as a fallthrough without the condition and undo all the conditions (we don't keep track of edges and directions, just the condition).

Looking at conditional blocks last lets us avoid the rollback (see logic in ILImporter.Scanner.cs).

MichalStrehovsky · 2024-07-01T12:14:42Z

This is now ready for review. Cc @dotnet/ilc-contrib

Fundamentally this has two parts:

Infrastructure to support generating conditional dependencies when building whole program view (ability to say a part of method only depends on something if something else is in the whole program view graph).
Leverage Extract shared IL pattern analysis to a class #103701 and report conditional dependencies whenever a basic block is guarded by a typeof check.

Nice savings for WinRT components and simple app that uses reflection (probably representative of e.g. cdacreader):

Size statistics

Pull request #103883

Project	Size before	Size after	Difference
avalonia.app-linux	24597280	24556288	-40992
avalonia.app-windows	22124544	22089728	-34816
hello-linux	1348320	1348288	-32
hello-minimal-linux	1077880	1077848	-32
hello-minimal-windows	858624	854016	-4608
hello-windows	1104384	1099776	-4608
kestrel-minimal-linux	5578976	5570784	-8192
kestrel-minimal-windows	4924416	4923904	-512
reflection-linux	2228112	2071840	-156272
reflection-windows	1883648	1753088	-130560
webapiaot-linux	10225648	10225632	-16
webapiaot-windows	9160192	9160192	0
winrt-component-full-windows	5532160	5508608	-23552
winrt-component-minimal-windows	1835008	1747968	-87040

am11 · 2024-07-01T12:21:44Z

Great improvements! 😄

hello-linux 1348320 1348288 -32

should we be expecting more based on:

      ****************************************************
      * Size test                                        *
      * Size of the executable is   1,262 kB             *
      ****************************************************
      BUG: File size is not in the expected range (1331200 to 1945600 bytes). Did a libraries change regress size of Hello World?

agocke · 2024-07-02T17:09:28Z

src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs

@@ -28,7 +30,7 @@ internal partial class ILImporter

        private readonly MethodDesc _canonMethod;

-        private DependencyList _dependencies = new DependencyList();
+        private DependencyList _unconditionalDependencies = new DependencyList();


Suggested change

private DependencyList _unconditionalDependencies = new DependencyList();

private readonly DependencyList _unconditionalDependencies = new DependencyList();

agocke · 2024-07-02T17:11:07Z

src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs

@@ -172,9 +182,21 @@ public DependencyList Import()
            FindBasicBlocks();
            ImportBasicBlocks();

-            CodeBasedDependencyAlgorithm.AddDependenciesDueToMethodCodePresence(ref _dependencies, _factory, _canonMethod, _canonMethodIL);
+            CombinedDependencyList conditionalDependencies = null;
+            foreach (BasicBlock bb in _basicBlocks)


Why can there be null blocks in this list?

This array is indexed into by the IL offset. The entries are non-null at basic block start locations and null elsewhere.

MichalStrehovsky · 2024-07-10T08:32:24Z

@sbomer could you have a look at this please? We could also go over this on a teams call.

MichalStrehovsky · 2024-07-17T21:35:28Z

@sbomer could you have a look at this please? We could also go over this on a teams call.

sbomer

Sorry I missed the first ping. A couple questions but LGTM!

sbomer · 2024-07-18T00:01:01Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/ShadowConcreteMethodNode.cs

+                    Debug.Assert(canonDep.OtherReasonNode is not INodeWithRuntimeDeterminedDependencies);
+
+                    var node = canonDep.Node;
+                    if (node is INodeWithRuntimeDeterminedDependencies runtimeDeterminedNode)


If I didn't miss any cases, this can only be ReadyToRunGenericHelperNode, MakeGenericMethodSite, or MakeGenericTypeSite. How does this work for methods on generic types that are instantiated directly rather than through MakeGenericType?

I think #103883 (comment) explains it, but skips a small step. I wrote "Canonical method Blah depends on e.g. T__Canon (for the typeof(T)).", but the actual dependency is: "Canonical method Blah depends on ReadyToRunGenericHelperNode of T__Canon."

So this is how directly instantiated things are tracked. The remaining MakeGenericMethodSite and MakeGenericTypeSite are relatively recent additions from #99037 and are only used in relation to dataflow since the problem is similar (we compute something on a "lesser instantiated thing" and need to specialize it for whatever else is in the dependency graph).

sbomer · 2024-07-18T00:11:21Z

src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs

+            }
+        }
+
+        private void ImportFallthrough(BasicBlock next, object condition = null)


nit: to me "fallthrough" means "the case where the condition wasn't satisfied (or there was no condition)", so I'd expect condition to always be null. Maybe add a separate helper that accepts a condition? I see the existing code uses ImportFallthrough for branch targets too, so feel free to leave as-is if you prefer.

Yes, it's weird, it's not my naming, but I don't have a better name, so I'll not churn CI for this.

sbomer · 2024-07-18T00:15:19Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/ShadowConcreteMethodNode.cs

Could you give an example of a conditional dependency of ShadowConcreteMethod?

dotnet-issue-labeler bot added the area-NativeAOT-coreclr label Jun 24, 2024

dotnet-policy-service bot assigned MichalStrehovsky Jun 24, 2024

build-analysis bot mentioned this pull request Jun 24, 2024

GC/Regressions/v2.0-beta2/452950 failed in CI #103494

Closed

github-actions bot mentioned this pull request Jun 24, 2024

103883 MichalStrehovsky/rt-sz#39

Closed

MichalStrehovsky mentioned this pull request Jun 24, 2024

Extract shared IL pattern analysis to a class #103701

Merged

MichalStrehovsky force-pushed the scantypeof branch 3 times, most recently from 7b382cd to 0ea6c49 Compare June 28, 2024 09:05

This was referenced Jun 28, 2024

Build failure: Static graph-based restore failed with exit code .* but did not log an error. #103526

Open

Build failure: Static graph-based restore failed with exit code .* but did not log an error. dotnet/dnceng#3139

Closed

MichalStrehovsky force-pushed the scantypeof branch from 0ea6c49 to 59287d1 Compare June 28, 2024 11:10

MichalStrehovsky force-pushed the scantypeof branch from 59287d1 to 6ec1308 Compare July 1, 2024 07:43

MichalStrehovsky commented Jul 1, 2024

View reviewed changes

MichalStrehovsky marked this pull request as ready for review July 1, 2024 12:10

agocke reviewed Jul 2, 2024

View reviewed changes

Merge branch 'main' into scantypeof

29c3c45

sbomer approved these changes Jul 18, 2024

View reviewed changes

MichalStrehovsky merged commit 9c7ee97 into dotnet:main Jul 18, 2024
84 of 93 checks passed

MichalStrehovsky deleted the scantypeof branch July 18, 2024 14:51

matouskozak mentioned this pull request Jul 21, 2024

[Perf] Windows/x64: 8 Improvements on 7/18/2024 9:05:42 AM dotnet/perf-autofiling-issues#38609

Closed

github-actions bot locked and limited conversation to collaborators Aug 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid scanning typeof checks when building whole program view #103883

Avoid scanning typeof checks when building whole program view #103883

MichalStrehovsky commented Jun 24, 2024

dotnet-policy-service bot commented Jun 24, 2024

MichalStrehovsky commented Jun 24, 2024

azure-pipelines bot commented Jun 24, 2024

MichalStrehovsky commented Jun 24, 2024

MichalStrehovsky commented Jul 1, 2024

azure-pipelines bot commented Jul 1, 2024

MichalStrehovsky Jul 1, 2024

agocke Jul 2, 2024

agocke Jul 2, 2024

MichalStrehovsky Jul 9, 2024

sbomer Jul 18, 2024

MichalStrehovsky Jul 1, 2024

MichalStrehovsky commented Jul 1, 2024

am11 commented Jul 1, 2024

agocke Jul 2, 2024

agocke Jul 2, 2024

MichalStrehovsky Jul 9, 2024

MichalStrehovsky commented Jul 10, 2024

MichalStrehovsky commented Jul 17, 2024

sbomer left a comment

sbomer Jul 18, 2024

MichalStrehovsky Jul 18, 2024

sbomer Jul 18, 2024

sbomer Jul 18, 2024

MichalStrehovsky Jul 18, 2024

sbomer Jul 18, 2024

	private DependencyList _unconditionalDependencies = new DependencyList();
	private readonly DependencyList _unconditionalDependencies = new DependencyList();

Avoid scanning typeof checks when building whole program view #103883

Avoid scanning typeof checks when building whole program view #103883

Conversation

MichalStrehovsky commented Jun 24, 2024

dotnet-policy-service bot commented Jun 24, 2024

MichalStrehovsky commented Jun 24, 2024

azure-pipelines bot commented Jun 24, 2024

MichalStrehovsky commented Jun 24, 2024

MichalStrehovsky commented Jul 1, 2024

azure-pipelines bot commented Jul 1, 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

MichalStrehovsky commented Jul 1, 2024

Size statistics

am11 commented Jul 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalStrehovsky commented Jul 10, 2024

MichalStrehovsky commented Jul 17, 2024

sbomer left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment