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

JIT: fix recursive inline check #88749

Merged
merged 1 commit into from
Jul 12, 2023
Merged

Conversation

AndyAyersMS
Copy link
Member

The existing check was too conservative, and blocked inlines of one instantation of a generic method into a different instantiation of the same method, or of two different methods that share the exact same IL stream.

Generalize the check to also compare the method handle and runtime context.

Fixes #88667
Fixes #58824

The existing check was too conservative, and blocked inlines of one instantation
of a generic method into a different instantiation of the same method, or of two
different methods that share the exact same IL stream.

Generalize the check to also compare the method handle and runtime context.

Fixes dotnet#88667
Fixes dotnet#58824
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 12, 2023
@ghost ghost assigned AndyAyersMS Jul 12, 2023
@ghost
Copy link

ghost commented Jul 12, 2023

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

Issue Details

The existing check was too conservative, and blocked inlines of one instantation of a generic method into a different instantiation of the same method, or of two different methods that share the exact same IL stream.

Generalize the check to also compare the method handle and runtime context.

Fixes #88667
Fixes #58824

Author: AndyAyersMS
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Jul 12, 2023

@EgorBo PTAL
cc @dotnet/jit-contrib

Per jit-diff, fairly minimal diffs overall. The WebCIL diffs are cases like we see in #88667.

PMI CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies for x64 default jit

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 66985998
Total bytes of diff: 66989680
Total bytes of delta: 3682 (0.01 % of base)
Total relative delta: NaN
    diff is a regression.
    relative diff is a regression.


Top file regressions (bytes):
         778 : Microsoft.CodeAnalysis.CSharp.dasm (0.01% of base)
         736 : System.Collections.Immutable.dasm (0.04% of base)
         657 : System.Private.CoreLib.dasm (0.01% of base)
         562 : System.Data.Common.dasm (0.04% of base)
         534 : System.ComponentModel.Composition.dasm (0.15% of base)
         276 : Microsoft.CodeAnalysis.VisualBasic.dasm (0.00% of base)
         150 : Microsoft.CodeAnalysis.dasm (0.00% of base)
          92 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (0.00% of base)
          52 : System.IO.Packaging.dasm (0.06% of base)
          13 : System.Private.DataContractSerialization.dasm (0.00% of base)

Top file improvements (bytes):
        -166 : Microsoft.NET.WebAssembly.Webcil.dasm (-0.74% of base)
          -2 : System.Private.Xml.dasm (-0.00% of base)

12 total files with Code Size differences (2 improved, 10 regressed), 265 unchanged.

Top method regressions (bytes):
         562 (156.11% of base) : System.Data.Common.dasm - System.Data.RBTree`1[System.Numerics.Vector`1[float]]:IndexOf(int,System.Numerics.Vector`1[float]):int:this (FullOpts)
         267 (88.12% of base) : System.ComponentModel.Composition.dasm - System.ComponentModel.Composition.Hosting.ImportEngine+EngineContext:GetAddedPartManagers():System.Collections.Generic.IEnumerable`1[System.ComponentModel.Composition.Hosting.ImportEngine+PartManager]:this (FullOpts)
         267 (88.12% of base) : System.ComponentModel.Composition.dasm - System.ComponentModel.Composition.Hosting.ImportEngine+EngineContext:GetRemovedPartManagers():System.Collections.Generic.IEnumerable`1[System.ComponentModel.Composition.Hosting.ImportEngine+PartManager]:this (FullOpts)
         232 (79.73% of base) : System.Collections.Immutable.dasm - System.Collections.Immutable.ImmutableList`1+Node[System.Numerics.Vector`1[float]]:Add(System.Numerics.Vector`1[float]):System.Collections.Immutable.ImmutableList`1+Node[System.Numerics.Vector`1[float]]:this (FullOpts)
         212 (25.51% of base) : System.Private.CoreLib.dasm - System.DateTime:.ctor(int,int,int,int,int,int,int):this (FullOpts) (2 methods)
         206 (87.66% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.NullableWalker+LocalState:PopulateAll(Microsoft.CodeAnalysis.CSharp.NullableWalker):this (FullOpts)
         202 (57.22% of base) : System.Private.CoreLib.dasm - System.DateTime:.ctor(int,int,int,int,int,int):this (FullOpts)
         176 (56.96% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Microsoft.CodeAnalysis.VisualBasic.SyntaxTreeDiagnosticEnumerator:Push(Microsoft.CodeAnalysis.GreenNode,bool):this (FullOpts)
         174 (37.10% of base) : System.Private.CoreLib.dasm - System.DateTime:Init(int,int,int,int,int,int,int,int):ulong (FullOpts)
         150 (72.46% of base) : Microsoft.CodeAnalysis.dasm - Microsoft.Cci.ITypeReferenceExtensions:GetConsolidatedTypeArguments(Microsoft.Cci.ITypeReference,Microsoft.CodeAnalysis.PooledObjects.ArrayBuilder`1[Microsoft.Cci.ITypeReference],Microsoft.CodeAnalysis.Emit.EmitContext) (FullOpts)
         148 (89.70% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.NullableWalker+LocalState:Normalize(Microsoft.CodeAnalysis.CSharp.NullableWalker,Microsoft.CodeAnalysis.CSharp.NullableWalker+Variables):this (FullOpts)
         137 (75.69% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.NullableWalker+LocalState:<Dump>g__getName|38_0(int,byref):System.String (FullOpts)
         105 (71.92% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.NullableWalker+LocalState:ForEach[System.Numerics.Vector`1[float]](System.Action`2[int,System.Numerics.Vector`1[float]],System.Numerics.Vector`1[float]):this (FullOpts)
         102 (60.00% of base) : System.Collections.Immutable.dasm - System.Collections.Immutable.ImmutableList`1+Node[System.Numerics.Vector`1[float]]:Contains(System.Collections.Immutable.ImmutableList`1+Node[System.Numerics.Vector`1[float]],System.Numerics.Vector`1[float],System.Collections.Generic.IEqualityComparer`1[System.Numerics.Vector`1[float]]):bool (FullOpts)
          92 (30.67% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - Microsoft.Diagnostics.Tracing.StackSources.LinuxPerfScriptStackSource:InternFrames(System.Collections.Generic.IEnumerator`1[Microsoft.Diagnostics.Tracing.StackSources.Frame],int,int,System.Nullable`1[int],Microsoft.Diagnostics.Tracing.StackSources.BlockedTimeAnalyzer):int:this (FullOpts)
          69 (35.20% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Microsoft.CodeAnalysis.VisualBasic.SyntaxTreeDiagnosticEnumerator:PushToken(Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax.SyntaxToken,bool):this (FullOpts)
          68 (75.56% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.NullableWalker+LocalState:Join(byref):bool:this (FullOpts)
          68 (75.56% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.NullableWalker+LocalState:Meet(byref):bool:this (FullOpts)
          66 (53.66% of base) : System.Collections.Immutable.dasm - System.Collections.Immutable.ImmutableList`1+Node[System.Nullable`1[int]]:Contains(System.Collections.Immutable.ImmutableList`1+Node[System.Nullable`1[int]],System.Nullable`1[int],System.Collections.Generic.IEqualityComparer`1[System.Nullable`1[int]]):bool (FullOpts)
          52 (66.67% of base) : System.IO.Packaging.dasm - System.IO.Packaging.XmlCompatibilityReader+CompatibilityScope:CanIgnore(System.String):bool:this (FullOpts)

Top method improvements (bytes):
         -48 (-15.79% of base) : Microsoft.NET.WebAssembly.Webcil.dasm - Microsoft.NET.WebAssembly.Webcil.WebcilConverter+PEFileInfo:Equals(Microsoft.NET.WebAssembly.Webcil.WebcilConverter+PEFileInfo):bool:this (FullOpts)
         -41 (-13.67% of base) : Microsoft.NET.WebAssembly.Webcil.dasm - Microsoft.NET.WebAssembly.Webcil.WebcilConverter+PEFileInfo:GetHashCode():int:this (FullOpts)
         -41 (-12.06% of base) : Microsoft.NET.WebAssembly.Webcil.dasm - Microsoft.NET.WebAssembly.Webcil.WebcilConverter+WCFileInfo:GetHashCode():int:this (FullOpts)
         -36 (-9.05% of base) : Microsoft.NET.WebAssembly.Webcil.dasm - Microsoft.NET.WebAssembly.Webcil.WebcilConverter+WCFileInfo:Equals(Microsoft.NET.WebAssembly.Webcil.WebcilConverter+WCFileInfo):bool:this (FullOpts)
         -22 (-15.28% of base) : System.Private.Xml.dasm - System.Xml.Serialization.SourceInfo:Equals(System.Object):bool:this (FullOpts)
          -1 (-0.10% of base) : System.Private.CoreLib.dasm - System.DateTime:.ctor(int,int,int,int,int,int,int,int):this (FullOpts) (2 methods)

Top method regressions (percentages):
         562 (156.11% of base) : System.Data.Common.dasm - System.Data.RBTree`1[System.Numerics.Vector`1[float]]:IndexOf(int,System.Numerics.Vector`1[float]):int:this (FullOpts)
         148 (89.70% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.NullableWalker+LocalState:Normalize(Microsoft.CodeAnalysis.CSharp.NullableWalker,Microsoft.CodeAnalysis.CSharp.NullableWalker+Variables):this (FullOpts)
         267 (88.12% of base) : System.ComponentModel.Composition.dasm - System.ComponentModel.Composition.Hosting.ImportEngine+EngineContext:GetAddedPartManagers():System.Collections.Generic.IEnumerable`1[System.ComponentModel.Composition.Hosting.ImportEngine+PartManager]:this (FullOpts)
         267 (88.12% of base) : System.ComponentModel.Composition.dasm - System.ComponentModel.Composition.Hosting.ImportEngine+EngineContext:GetRemovedPartManagers():System.Collections.Generic.IEnumerable`1[System.ComponentModel.Composition.Hosting.ImportEngine+PartManager]:this (FullOpts)
         206 (87.66% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.NullableWalker+LocalState:PopulateAll(Microsoft.CodeAnalysis.CSharp.NullableWalker):this (FullOpts)
         232 (79.73% of base) : System.Collections.Immutable.dasm - System.Collections.Immutable.ImmutableList`1+Node[System.Numerics.Vector`1[float]]:Add(System.Numerics.Vector`1[float]):System.Collections.Immutable.ImmutableList`1+Node[System.Numerics.Vector`1[float]]:this (FullOpts)
         137 (75.69% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.NullableWalker+LocalState:<Dump>g__getName|38_0(int,byref):System.String (FullOpts)
          68 (75.56% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.NullableWalker+LocalState:Join(byref):bool:this (FullOpts)
          68 (75.56% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.NullableWalker+LocalState:Meet(byref):bool:this (FullOpts)
         150 (72.46% of base) : Microsoft.CodeAnalysis.dasm - Microsoft.Cci.ITypeReferenceExtensions:GetConsolidatedTypeArguments(Microsoft.Cci.ITypeReference,Microsoft.CodeAnalysis.PooledObjects.ArrayBuilder`1[Microsoft.Cci.ITypeReference],Microsoft.CodeAnalysis.Emit.EmitContext) (FullOpts)
         105 (71.92% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.NullableWalker+LocalState:ForEach[System.Numerics.Vector`1[float]](System.Action`2[int,System.Numerics.Vector`1[float]],System.Numerics.Vector`1[float]):this (FullOpts)
          48 (69.57% of base) : System.Collections.Immutable.dasm - System.Collections.Immutable.ImmutableList`1+Node[double]:AddRange(System.ReadOnlySpan`1[double]):System.Collections.Immutable.ImmutableList`1+Node[double]:this (FullOpts)
          48 (69.57% of base) : System.Collections.Immutable.dasm - System.Collections.Immutable.ImmutableList`1+Node[int]:AddRange(System.ReadOnlySpan`1[int]):System.Collections.Immutable.ImmutableList`1+Node[int]:this (FullOpts)
          48 (69.57% of base) : System.Collections.Immutable.dasm - System.Collections.Immutable.ImmutableList`1+Node[long]:AddRange(System.ReadOnlySpan`1[long]):System.Collections.Immutable.ImmutableList`1+Node[long]:this (FullOpts)
          48 (69.57% of base) : System.Collections.Immutable.dasm - System.Collections.Immutable.ImmutableList`1+Node[short]:AddRange(System.ReadOnlySpan`1[short]):System.Collections.Immutable.ImmutableList`1+Node[short]:this (FullOpts)
          48 (69.57% of base) : System.Collections.Immutable.dasm - System.Collections.Immutable.ImmutableList`1+Node[System.Nullable`1[int]]:AddRange(System.ReadOnlySpan`1[System.Nullable`1[int]]):System.Collections.Immutable.ImmutableList`1+Node[System.Nullable`1[int]]:this (FullOpts)
          48 (69.57% of base) : System.Collections.Immutable.dasm - System.Collections.Immutable.ImmutableList`1+Node[System.Numerics.Vector`1[float]]:AddRange(System.ReadOnlySpan`1[System.Numerics.Vector`1[float]]):System.Collections.Immutable.ImmutableList`1+Node[System.Numerics.Vector`1[float]]:this (FullOpts)
          48 (69.57% of base) : System.Collections.Immutable.dasm - System.Collections.Immutable.ImmutableList`1+Node[ubyte]:AddRange(System.ReadOnlySpan`1[ubyte]):System.Collections.Immutable.ImmutableList`1+Node[ubyte]:this (FullOpts)
          52 (66.67% of base) : System.IO.Packaging.dasm - System.IO.Packaging.XmlCompatibilityReader+CompatibilityScope:CanIgnore(System.String):bool:this (FullOpts)
         102 (60.00% of base) : System.Collections.Immutable.dasm - System.Collections.Immutable.ImmutableList`1+Node[System.Numerics.Vector`1[float]]:Contains(System.Collections.Immutable.ImmutableList`1+Node[System.Numerics.Vector`1[float]],System.Numerics.Vector`1[float],System.Collections.Generic.IEqualityComparer`1[System.Numerics.Vector`1[float]]):bool (FullOpts)

Top method improvements (percentages):
         -48 (-15.79% of base) : Microsoft.NET.WebAssembly.Webcil.dasm - Microsoft.NET.WebAssembly.Webcil.WebcilConverter+PEFileInfo:Equals(Microsoft.NET.WebAssembly.Webcil.WebcilConverter+PEFileInfo):bool:this (FullOpts)
         -22 (-15.28% of base) : System.Private.Xml.dasm - System.Xml.Serialization.SourceInfo:Equals(System.Object):bool:this (FullOpts)
         -41 (-13.67% of base) : Microsoft.NET.WebAssembly.Webcil.dasm - Microsoft.NET.WebAssembly.Webcil.WebcilConverter+PEFileInfo:GetHashCode():int:this (FullOpts)
         -41 (-12.06% of base) : Microsoft.NET.WebAssembly.Webcil.dasm - Microsoft.NET.WebAssembly.Webcil.WebcilConverter+WCFileInfo:GetHashCode():int:this (FullOpts)
         -36 (-9.05% of base) : Microsoft.NET.WebAssembly.Webcil.dasm - Microsoft.NET.WebAssembly.Webcil.WebcilConverter+WCFileInfo:Equals(Microsoft.NET.WebAssembly.Webcil.WebcilConverter+WCFileInfo):bool:this (FullOpts)
          -1 (-0.10% of base) : System.Private.CoreLib.dasm - System.DateTime:.ctor(int,int,int,int,int,int,int,int):this (FullOpts) (2 methods)

40 total methods with Code Size differences (6 improved, 34 regressed), 416702 unchanged.

Per BenchmarkDotNet, this fixes the perf regressions from #88667.

Method Job Toolchain Mean Error StdDev Median Min Max Ratio Allocated Alloc Ratio
ValueTupleCompareNoOpt Job-PSRQUH DIFF 3.7414 ns 0.0201 ns 0.0157 ns 3.7377 ns 3.7206 ns 3.7643 ns 0.73 - NA
ValueTupleCompareNoOpt Job-KLRQLL BASE 5.1556 ns 0.0547 ns 0.0512 ns 5.1451 ns 5.0433 ns 5.2335 ns 1.00 - NA
ValueTupleCompare Job-PSRQUH DIFF 0.6834 ns 0.0147 ns 0.0130 ns 0.6862 ns 0.6633 ns 0.7081 ns 0.25 - NA
ValueTupleCompare Job-KLRQLL BASE 2.6975 ns 0.0315 ns 0.0295 ns 2.6993 ns 2.6589 ns 2.7505 ns 1.00 - NA
ValueTupleCompareCached Job-PSRQUH DIFF 1.3210 ns 0.0209 ns 0.0195 ns 1.3252 ns 1.2910 ns 1.3630 ns 0.41 - NA
ValueTupleCompareCached Job-KLRQLL BASE 3.2162 ns 0.0441 ns 0.0413 ns 3.2205 ns 3.1239 ns 3.2672 ns 1.00 - NA
ValueTupleCompareWrapped Job-PSRQUH DIFF 0.5306 ns 0.0196 ns 0.0183 ns 0.5265 ns 0.4972 ns 0.5694 ns 0.21 - NA
ValueTupleCompareWrapped Job-KLRQLL BASE 2.5522 ns 0.0260 ns 0.0244 ns 2.5587 ns 2.5150 ns 2.5940 ns 1.00 - NA

@AndyAyersMS AndyAyersMS requested a review from EgorBo July 12, 2023 17:00
@EgorBo
Copy link
Member

EgorBo commented Jul 12, 2023

@MihuBot

@AndyAyersMS
Copy link
Member Author

I don't yet understand if / how the two x64 failures (file-check) are related -- those tests seemingly won't do any inlining.

The x86 failure is similar to #88579. It also showed up in #88527

@AndyAyersMS
Copy link
Member Author

I don't yet understand if / how the two x64 failures (file-check) are related -- those tests seemingly won't do any inlining.

Suspect is is fallout from #88692

@AndyAyersMS
Copy link
Member Author

Opened issues for the failures, not sure if/when build analysis will update to show this.

@AndyAyersMS
Copy link
Member Author

Errors are known.

AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this pull request Jul 21, 2023
With PGO and (via dotnet#88749) one level of recursive inlining enabled, the jit sees
the recursive call made by `IntroSort` as an attractive inline candidate,
but it isn't.

Fixes dotnet#89106.
AndyAyersMS added a commit that referenced this pull request Jul 21, 2023
With PGO and (via #88749) one level of recursive inlining enabled, the jit sees
the recursive call made by `IntroSort` as an attractive inline candidate,
but it isn't.

Fixes #89106.
jakobbotsch added a commit to jakobbotsch/runtime that referenced this pull request Aug 10, 2023
…ecursive methods

The inliner heuristics do not take into account that inlining methods
causes type/method loading of the generic context. After dotnet#88749 this can
quickly cause significant resources to be consumed as part of inlining
when polymorphic recursion is involved (the blow-up can be exponential,
as we see in the failing test under jitstress).

This PR adds another safe-guard to the recursive inlining check in terms
of a complexity limit on the generic context of the inline candidate.

Fix dotnet#90144
jakobbotsch added a commit that referenced this pull request Aug 12, 2023
…ecursive methods (#90306)

The inliner heuristics do not take into account that inlining methods
causes type/method loading of the generic context. After #88749 this can
quickly cause significant resources to be consumed as part of inlining
when polymorphic recursion is involved (the blow-up can be exponential,
as we see in the failing test under jitstress).

This PR adds another safe-guard to the recursive inlining check in terms
of a complexity limit on the generic context of the inline candidate.

Fix #90144
@ghost ghost locked as resolved and limited conversation to collaborators Aug 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
2 participants