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.

Sign up for GitHub

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

Jump to bottom

PGO: redundant Tier0-instr promotions #84517

Open
EgorBo opened this issue Apr 8, 2023 · 5 comments
Open

PGO: redundant Tier0-instr promotions #84517

EgorBo opened this issue Apr 8, 2023 · 5 comments
Assignees
@EgorBo
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI Priority:2 Work that is important, but not critical for the release
Milestone
9.0.0

Comments

@EgorBo
Copy link
Member

EgorBo commented Apr 8, 2023 

static void Main()
{
    for (int i = 0; i < 100; i++)
    {
        Test();
        var _ = Test2;
        Thread.Sleep(16);
    }
}

[MethodImpl(MethodImplOptions.NoInlining)]
static int Test1()
{
    int sum = 0;
    for (int i = 0; i < 10; i++)
        sum += i;
    return sum;
}

static int Test2 { get; set; }

Actual result:

 147: JIT compiled Prog:Test1():int [Instrumented Tier0, IL size=21, code size=109, hash=0x792ae54b]
 311: JIT compiled Prog:Test1():int [Instrumented Tier0, IL size=21, code size=109, hash=0x792ae54b]
 313: JIT compiled Prog:Test1():int [Tier1 with Dynamic PGO, IL size=21, code size=14, hash=0x792ae54b]

 152: JIT compiled Prog:get_Test2():int [Tier0, IL size=6, code size=12, hash=0xf8310770]
 316: JIT compiled Prog:get_Test2():int [Instrumented Tier0, IL size=6, code size=12, hash=0xf8310770]
 319: JIT compiled Prog:get_Test2():int [Tier1, IL size=6, code size=7, hash=0xf8310770]

Since Test1 has a loop we always instrument it in Tier0, but then it's promoted to Instrumented Tier0 again - it can be omitted.

Also, all Tier0 methods should warn VM in advance that they don't need instrumentation at all (e.g. simple auto properties like get_Test2).

cc @AndyAyersMS
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Apr 8, 2023
@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 Apr 8, 2023
@ghost
Copy link

ghost commented Apr 8, 2023

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

Issue Details 
static void Main()
{
    for (int i = 0; i < 100; i++)
    {
        Test();
        var _ = Test2;
        Thread.Sleep(16);
    }
}

[MethodImpl(MethodImplOptions.NoInlining)]
static int Test1()
{
    int sum = 0;
    for (int i = 0; i < 10; i++)
        sum += i;
    return sum;
}

static int Test2 { get; set; }

Actual result:

 147: JIT compiled Prog:Test1():int [Instrumented Tier0, IL size=21, code size=109, hash=0x792ae54b]
 311: JIT compiled Prog:Test1():int [Instrumented Tier0, IL size=21, code size=109, hash=0x792ae54b]
 313: JIT compiled Prog:Test1():int [Tier1 with Dynamic PGO, IL size=21, code size=14, hash=0x792ae54b]

 152: JIT compiled Prog:get_Test2():int [Tier0, IL size=6, code size=12, hash=0xf8310770]
 316: JIT compiled Prog:get_Test2():int [Instrumented Tier0, IL size=6, code size=12, hash=0xf8310770]
 319: JIT compiled Prog:get_Test2():int [Tier1, IL size=6, code size=7, hash=0xf8310770]

Since Test1 has a loop we always instrument it in Tier0, but then it's promoted to Instrumented Tier0 again - it can be omitted.

Also, all Tier0 methods should warn VM in advance that they don't need instrumentation at all (e.g. simple auto properties like get_Test2).

cc @AndyAyersMS

Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr, untriaged
Milestone: -

@EgorBo EgorBo self-assigned this Apr 8, 2023
@EgorBo EgorBo removed the untriaged New issue has not been triaged by the area owner label Apr 8, 2023
@EgorBo EgorBo added this to the 8.0.0 milestone Apr 8, 2023
@AndyAyersMS AndyAyersMS mentioned this issue Apr 10, 2023
Closed
@AndyAyersMS
Copy link
Member

Even more interesting when OSR gets involved:

 832: JIT compiled Benchstone.BenchI.MulMatrix:Inner(int[][],int[][],int[][]) [Instrumented Tier0, IL size=435, code size=2943, hash=0x482bb336]
 833: JIT compiled Benchstone.BenchI.MulMatrix:Inner(int[][],int[][],int[][]) [Tier1-OSR @0x60 with Dynamic PGO, IL size=435, code size=1323, hash=0x482bb336]
 834: JIT compiled Benchstone.BenchI.MulMatrix:Inner(int[][],int[][],int[][]) [Tier1-OSR @0x2a with Dynamic PGO, IL size=435, code size=1529, hash=0x482bb336]
 835: JIT compiled Benchstone.BenchI.MulMatrix:Inner(int[][],int[][],int[][]) [Instrumented Tier0, IL size=435, code size=2943, hash=0x482bb336]
 836: JIT compiled Benchstone.BenchI.MulMatrix:Inner(int[][],int[][],int[][]) [Tier1-OSR @0x60 with Dynamic PGO, IL size=435, code size=1323, hash=0x482bb336]
 837: JIT compiled Benchstone.BenchI.MulMatrix:Inner(int[][],int[][],int[][]) [Tier1-OSR @0x2a with Dynamic PGO, IL size=435, code size=1547, hash=0x482bb336]
 838: JIT compiled Benchstone.BenchI.MulMatrix:Inner(int[][],int[][],int[][]) [Tier1 with Dynamic PGO, IL size=435, code size=1194, hash=0x482bb336]

@EgorBo
Copy link
Member Author

EgorBo commented Apr 12, 2023

Even more interesting when OSR gets involved:

 832: JIT compiled Benchstone.BenchI.MulMatrix:Inner(int[][],int[][],int[][]) [Instrumented Tier0, IL size=435, code size=2943, hash=0x482bb336]
 833: JIT compiled Benchstone.BenchI.MulMatrix:Inner(int[][],int[][],int[][]) [Tier1-OSR @0x60 with Dynamic PGO, IL size=435, code size=1323, hash=0x482bb336]
 834: JIT compiled Benchstone.BenchI.MulMatrix:Inner(int[][],int[][],int[][]) [Tier1-OSR @0x2a with Dynamic PGO, IL size=435, code size=1529, hash=0x482bb336]
 835: JIT compiled Benchstone.BenchI.MulMatrix:Inner(int[][],int[][],int[][]) [Instrumented Tier0, IL size=435, code size=2943, hash=0x482bb336]
 836: JIT compiled Benchstone.BenchI.MulMatrix:Inner(int[][],int[][],int[][]) [Tier1-OSR @0x60 with Dynamic PGO, IL size=435, code size=1323, hash=0x482bb336]
 837: JIT compiled Benchstone.BenchI.MulMatrix:Inner(int[][],int[][],int[][]) [Tier1-OSR @0x2a with Dynamic PGO, IL size=435, code size=1547, hash=0x482bb336]
 838: JIT compiled Benchstone.BenchI.MulMatrix:Inner(int[][],int[][],int[][]) [Tier1 with Dynamic PGO, IL size=435, code size=1194, hash=0x482bb336]

Heh indeed funny one but presumably the same issue, I'll fix it this week

@EgorBo EgorBo mentioned this issue Apr 13, 2023
Closed
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 13, 2023
@AndyAyersMS
Copy link
Member

We can also see this when we have a nontrivial intrinsic R2R method; we rejit at tier1-instr but it doesn't actually instrument; then we rejit at tier1 and don't have any dynamic PGO data, so we are jitting twice and probably producing the same code each time.

 841: JIT compiled System.Memory.Span`1[ubyte]:SequenceEqual():bool:this [Tier0, IL size=28, code size=151, hash=0xbffeed85]
**877: JIT compiled System.SpanHelpers:SequenceEqual(byref,byref,ulong):bool [Instrumented Tier1 with Static PGO, IL size=612, code size=302, hash=0xacbc5054]**
 878: JIT compiled System.Memory.Span`1[ubyte]:SequenceEqual():bool:this [Instrumented Tier0, IL size=28, code size=151, hash=0xbffeed85]
 881: JIT compiled System.MemoryExtensions:SequenceEqual[ubyte](System.Span`1[ubyte],System.ReadOnlySpan`1[ubyte]):bool [Instrumented Tier1, IL size=96, code size=42, hash=0x608a246a]
**882: JIT compiled System.SpanHelpers:SequenceEqual(byref,byref,ulong):bool [Tier1 with Static PGO, IL size=612, code size=302, hash=0xacbc5054]**
 883: JIT compiled System.Memory.Span`1[ubyte]:SequenceEqual():bool:this [Tier1, IL size=28, code size=81, hash=0xbffeed85]
 886: JIT compiled System.MemoryExtensions:SequenceEqual[ubyte](System.Span`1[ubyte],System.ReadOnlySpan`1[ubyte]):bool [Tier1, IL size=96, code size=42, hash=0x608a246a]

@EgorBo EgorBo mentioned this issue Apr 20, 2023
Merged
@AndyAyersMS AndyAyersMS mentioned this issue Jun 19, 2023
Open
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 11, 2023
@EgorBo
Copy link
Member Author

EgorBo commented Aug 5, 2023

Moving to 9.0 as the VM change was not trivial. We think that this doesn't really affect perf or startup but still nice to fix

@EgorBo EgorBo modified the milestones: 8.0.0, 9.0.0 Aug 5, 2023
@EgorBo EgorBo added the Priority:2 Work that is important, but not critical for the release label May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@EgorBo EgorBo

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI Priority:2 Work that is important, but not critical for the release
Projects
None yet
Milestone
9.0.0
Development

Successfully merging a pull request may close this issue.

Don't re-instrument methods which don't need that EgorBo/runtime-1
2 participants
@EgorBo @AndyAyersMS