Don't re-instrument methods which don't need that#84772

EgorBo · 2023-04-13T13:28:25Z

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

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

static int Test2 { get; set; }

Was:

JIT compiled Program:Test1():int [Instrumented Tier0, IL size=24, code size=130, hash=0x5d807ce4]
JIT compiled Program:Test1():int [Tier1-OSR @0xe with Dynamic PGO, IL size=24, code size=34, hash=0x5d807ce4]
JIT compiled Program:Test1():int [Instrumented Tier0, IL size=24, code size=130, hash=0x5d807ce4]
JIT compiled Program:Test1():int [Tier1-OSR @0xe with Dynamic PGO, IL size=24, code size=34, hash=0x5d807ce4]
JIT compiled Program:Test1():int [Tier1 with Dynamic PGO, IL size=24, code size=17, hash=0x5d807ce4]
JIT compiled Program:get_Test2():int [Instrumented Tier0, IL size=6, code size=12, hash=0xc7bf650e]
JIT compiled Program:get_Test2():int [Tier0, IL size=6, code size=12, hash=0xc7bf650e]
JIT compiled Program:get_Test2():int [Tier1, IL size=6, code size=7, hash=0xc7bf650e]

Now:

JIT compiled Program:Test1():int [Instrumented Tier0, IL size=24, code size=130, hash=0x5d807ce4]
JIT compiled Program:Test1():int [Tier1-OSR @0xe with Dynamic PGO, IL size=24, code size=34, hash=0x5d807ce4]
JIT compiled Program:Test1():int [Tier1 with Dynamic PGO, IL size=24, code size=17, hash=0x5d807ce4]
JIT compiled Program:get_Test2():int [Tier0, IL size=6, code size=12, hash=0xc7bf650e]
JIT compiled Program:get_Test2():int [Tier1, IL size=6, code size=7, hash=0xc7bf650e]

What happened:

get_Test2() is not promoted to Tier0-Instrumented since Tier0 realized it's too simple for that so it's promoted to Tier1 (it's also not needed here but that is a different issue)
Test1() promoted itself to Tier0-Instrumented due to loops so it also now tells vm it doesn't need to be promoted to Tier0-Instrumented again (and discard OSR versions)

ghost · 2023-04-13T13:28:37Z

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

Issue Details

Closes #84517

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

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

static int Test2 { get; set; }

Was:

JIT compiled Program:Test1():int [Instrumented Tier0, IL size=24, code size=130, hash=0x5d807ce4]
JIT compiled Program:get_Test2():int [Tier0, IL size=6, code size=12, hash=0xc7bf650e]
JIT compiled Program:Test1():int [Tier1-OSR @0xe with Dynamic PGO, IL size=24, code size=34, hash=0x5d807ce4]
JIT compiled Program:Test1():int [Instrumented Tier0, IL size=24, code size=130, hash=0x5d807ce4]
JIT compiled Program:get_Test2():int [Instrumented Tier0, IL size=6, code size=12, hash=0xc7bf650e]
JIT compiled Program:Test1():int [Tier1-OSR @0xe with Dynamic PGO, IL size=24, code size=34, hash=0x5d807ce4]
JIT compiled Program:Test1():int [Tier1 with Dynamic PGO, IL size=24, code size=17, hash=0x5d807ce4]
JIT compiled Program:get_Test2():int [Tier1, IL size=6, code size=7, hash=0xc7bf650e]

Now:

JIT compiled Program:Test1():int [Instrumented Tier0, IL size=24, code size=130, hash=0x5d807ce4]
JIT compiled Program:get_Test2():int [Tier0, IL size=6, code size=12, hash=0xc7bf650e]
JIT compiled Program:Test1():int [Tier1-OSR @0xe with Dynamic PGO, IL size=24, code size=34, hash=0x5d807ce4]
JIT compiled Program:Test1():int [Tier1 with Dynamic PGO, IL size=24, code size=17, hash=0x5d807ce4]
JIT compiled Program:get_Test2():int [Tier1, IL size=6, code size=7, hash=0xc7bf650e]

Author:	EgorBo
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

EgorBo · 2023-04-13T16:46:38Z

/azp run runtime-coreclr pgo, runtime-coreclr libraries-pgo

azure-pipelines · 2023-04-13T16:47:00Z

Azure Pipelines successfully started running 2 pipeline(s).

EgorBo · 2023-04-13T19:11:56Z

PTAL @AndyAyersMS thoughts on the approach?
@jkotas can we borrow a flag in MethodDesc for that?

AndyAyersMS · 2023-04-13T19:30:00Z

Seems like you perhaps should keep this "don't need to instrument" bit on the NativeCodeVersion instead of the MethodDesc?

jkotas · 2023-04-13T19:34:37Z

I agree with Andy.

EgorBo · 2023-04-13T19:37:45Z

Thanks! Looks like it has plenty of space

1>class NativeCodeVersion	size(16):
1>	+---
1> 0	| StorageKind m_storageKind
1>  	| <alignment member> (size=4)
1> 8	| m_pVersionNode
1> 8	| <unnamed-type-m_synthetic> m_synthetic
1>	+---

considering that m_storageKind needs only 2 bits

PS: changed it to:

1>class NativeCodeVersion	size(16):
1>	+---
1> 0	| StorageKind m_storageKind
1> 2	| NativeCodeVersionFlag m_flags
1>  	| <alignment member> (size=4)
1> 8	| m_pVersionNode
1> 8	| <unnamed-type-m_synthetic> m_synthetic
1>	+---

EgorBo · 2023-04-13T22:34:13Z

/azp run runtime-coreclr pgo, runtime-coreclr libraries-pgo

azure-pipelines · 2023-04-13T22:34:37Z

Azure Pipelines successfully started running 2 pipeline(s).

EgorBo · 2023-04-13T23:52:34Z

Moved to NativeCodeVersion, failures are #84798

AndyAyersMS

Changes LGTM, but I wonder...

Can we see the same effect with simple R2R methods? That is, would we do R2R -> Tier1 + instr -> Tier1 even if Tier1+instr did not add any probes because the method was trivial?

I wonder if a simpler way of detecting these cases is for the jit interface to track if a schema was created during jitting (that is, if allocPgoInstrumentationBySchema ever got called during this jit attempt).

If at Tier0 with allow eager instrumentation flag, and the JIT created a schema, bypass future instrumentation requests (JIT decided to do eager instrumentation)
If at Tier1 BBINSTR and the JIT did not create a schema when the VM requested one, then the current code is Tier1 quality, and so will not benefit from more rejitting.

This would consolidate the logic on the VM side.

EgorBo · 2023-04-14T14:47:23Z

Ah, interesting idea, I'll try

AndyAyersMS

This version LGTM

AndyAyersMS · 2023-04-18T02:06:09Z

libraries-pgo was green last I looked, so maybe fire off that or some other pgo stress leg?

EgorBo · 2023-04-18T07:47:39Z

/azp run runtime-coreclr pgo, runtime-coreclr libraries-pgo, runtime-coreclr pgostress

azure-pipelines · 2023-04-18T07:48:06Z

Azure Pipelines successfully started running 3 pipeline(s).

EgorBo · 2023-04-18T11:48:06Z

PGO pipelines look good, failures are #84752 and #81360

EgorBo · 2023-04-18T18:02:57Z

@jkotas do vm changes look good to you to merge?

jkotas · 2023-04-18T18:11:49Z

@kouvel Could you please take a look?

…nstrumentations

kouvel · 2023-04-19T10:43:38Z

src/coreclr/vm/jitinterface.cpp

+        }
+
+        // Leave a note that this method is properly instrumented
+        currentVersion.SetInstrumented();


NativeCodeVersion is a simple wrapper around either a MethodDesc or a NativeCodeVersionNode, and is typically passed by value. The default native code version doesn't have a node and just wraps a MethodDesc. So, it's not a good candidate for persistent storage of this type of info. This change to the code version would just modify the local copy and would not be persisted anywhere.

AFAIK there isn't a clear place currently where such info could be stored. CallCountingInfo may be a candidate, for instance see CallCountingManager::DisableCallCounting() and CallCountingInfo::CreateWithCallCountingDisabled(), which are used to indicate that a code version should not be tiered. However, currently it's not set up to store extra info aside from call counting, for example CallCountingManager::SetCodeEntryPoint() does something different if the CallCountingInfo already exists as opposed to when it's being created. It may be possible to enable precreating a CallCountingInfo for a code version by introducing a new CallCountingInfo::Stage something like NotStarted to indicate that call counting has not started, and to modify SetCodeEntryPoint() to do something similar to before in that case. If the CallCountingInfo already exists, it could be used as persistent storage, as the CallCountingManager has a hash table mapping a code version to a CallCountingInfo.

Maybe it's better to rollback to occupying a spare bit in MethodDesc m_flag2 then? This change is a sort of an optimization, not correctness. The problem with CallCountingStubs that they're created assyncronlsy like you pointed out

The info appears to be specific to a code version rather than a method desc, so I'm not sure the method desc would be an appropriate place for it

kouvel · 2023-04-19T10:44:42Z

src/coreclr/vm/codeversion.h

+
 private:

+    enum NativeCodeVersionFlag : UINT16


I suggest using an enum class to avoid polluting the parent namespace, though this may need to be moved based on my other comment.

kouvel · 2023-04-19T10:59:33Z

src/coreclr/vm/jitinterface.cpp

+        CodeVersionManager* pCodeVersionManager = m_pMethodBeingCompiled->GetCodeVersionManager();
+        CodeVersionManager::LockHolder codeVersioningLockHolder;
+        ILCodeVersion ilVersion = pCodeVersionManager->GetActiveILCodeVersion(m_pMethodBeingCompiled);
+        NativeCodeVersion currentVersion = ilVersion.GetActiveNativeCodeVersion(m_pMethodBeingCompiled);


The active code version may have changed meanwhile, so it may not represent the current code version being jitted. Could use something like this instead:

runtime/src/coreclr/vm/jitinterface.cpp

Line 6681 in 7b662a7

PrepareCodeConfig *config = GetThread()->GetCurrentPrepareCodeConfig();

Along with config->GetCodeVersion().

ghost · 2023-07-11T23:01:57Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 13, 2023

ghost assigned EgorBo Apr 13, 2023

build-analysis bot mentioned this pull request Apr 13, 2023

jitformat failing on linux x64 #84780

Closed

EgorBo marked this pull request as ready for review April 13, 2023 17:25

EgorBo requested a review from MichalStrehovsky as a code owner April 13, 2023 17:26

This was referenced Apr 13, 2023

Tracking issue for CI build timeouts #76454

Closed

System.Net.Quic.Tests.QuicStreamTests.WriteCanceled_NextWriteThrows test failure #76831

Closed

WasmTestOnBrowser-System.Text.Json.Tests.WorkItemExecution timing out #84434

Closed

AndyAyersMS reviewed Apr 14, 2023

View reviewed changes

address feedback

78efc23

EgorBo force-pushed the fix-redundant-instrumentations branch from 957fe5a to 78efc23 Compare April 17, 2023 23:31

AndyAyersMS approved these changes Apr 18, 2023

View reviewed changes

jkotas requested a review from kouvel April 18, 2023 18:11

EgorBo added 2 commits April 19, 2023 11:46

Merge branch 'main' of github.com:dotnet/runtime into fix-redundant-i…

53aa380

…nstrumentations

Cache GetOptimizationTier call

46a343e

kouvel suggested changes Apr 19, 2023

View reviewed changes

ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Apr 19, 2023

kouvel reviewed Apr 19, 2023

View reviewed changes

EgorBo marked this pull request as draft June 11, 2023 21:36

ghost closed this Jul 11, 2023

ghost locked as resolved and limited conversation to collaborators Aug 14, 2023

This pull request was closed.

Comments

Conversation

EgorBo commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Apr 13, 2023

Uh oh!

EgorBo commented Apr 13, 2023

Uh oh!

azure-pipelines bot commented Apr 13, 2023

Uh oh!

EgorBo commented Apr 13, 2023

Uh oh!

AndyAyersMS commented Apr 13, 2023

Uh oh!

jkotas commented Apr 13, 2023

Uh oh!

EgorBo commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EgorBo commented Apr 13, 2023

Uh oh!

azure-pipelines bot commented Apr 13, 2023

Uh oh!

EgorBo commented Apr 13, 2023

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

EgorBo commented Apr 14, 2023

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Apr 18, 2023

Uh oh!

EgorBo commented Apr 18, 2023

Uh oh!

azure-pipelines bot commented Apr 18, 2023

Uh oh!

EgorBo commented Apr 18, 2023

Uh oh!

EgorBo commented Apr 18, 2023

Uh oh!

jkotas commented Apr 18, 2023

Uh oh!

kouvel Apr 19, 2023 • 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!

EgorBo Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

kouvel Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

kouvel Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

kouvel Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

EgorBo commented Apr 13, 2023 •

edited

Loading

EgorBo commented Apr 13, 2023 •

edited

Loading

kouvel Apr 19, 2023 •

edited

Loading