Add MethodImplOptions.AggressiveOptimization and use it for tiering #20009

kouvel · 2018-09-17T19:04:25Z

Part of fix for https://github.com/dotnet/corefx/issues/32235
Workaround for https://github.com/dotnet/coreclr/issues/19751

Added and set CORJIT_FLAG_AGGRESSIVE_OPT to indicate that a method is flagged with AggressiveOptimization
For a method flagged with AggressiveOptimization, tiering uses a foreground tier 1 JIT on first call to the method, skipping the tier 0 JIT and call counting
When tiering is disabled, a method flagged with AggressiveOptimization does not use r2r-pregenerated code
R2r crossgen does not generate code for a method flagged with AggressiveOptimization

kouvel · 2018-09-17T19:05:18Z

API review: https://github.com/dotnet/corefx/issues/32235

Part of fix for https://github.com/dotnet/corefx/issues/32235 Depends on dotnet/coreclr#20009 API review: https://github.com/dotnet/corefx/issues/32235

jkotas · 2018-09-18T16:59:18Z

src/vm/codeversion.cpp

-    m_optTier(NativeCodeVersion::OptimizationTier0),
+#ifdef FEATURE_TIERED_COMPILATION
+    m_optTier(
+        pMethodDesc->RequestedAggressiveOptimization()


Should this rather be initialized based on what was actually compiled?

The tier is only applicable to methods eligible for tiering, that currently includes methods with r2r-pregenerated code and excludes methods with fragile ngen-pregenerated code. For the eligible methods, the only reason currently they would use tier 1 initially is if they are flagged with AggressiveOptimization. Whether the method has pregenerated code available is checked later before compiling the method.

I meant whether this should be set based on what was JITed or going to be JITed.

If I am reading the code correctly, we will create a new NativeCodeVersionNode when we promote thing from Tier0 to Tier1; or when we actually JIT new code. We know the tier in both places.

The change is calling RequestedAggressiveOptimization on more places than what I would expect it to be called. So I am trying to understand why all the calls are really required.

Oh ok, yes that should be avoidable, I'll take a look

I would also suggest hoisting this call out of the constructor, but only because I view the CodeVersionNode as being a simple data container that shouldn't embed policy. I threw together a quick suggestion here:
noahfalk@697bc9e

jkotas · 2018-09-18T17:00:35Z

src/vm/prestub.cpp

@@ -334,7 +334,7 @@ PCODE MethodDesc::GetPrecompiledCode(PrepareCodeConfig* pConfig)
 #endif

 #ifdef FEATURE_READYTORUN
-    if (pCode == NULL)
+    if (pCode == NULL && !RequestedAggressiveOptimization())


This check should not be required. I think we should assume that if there is code in R2R image, it was aggressively optimized if necessary.

AndyAyersMS · 2018-09-18T17:26:08Z

src/zap/zapinfo.cpp

+            // Skip methods marked with MethodImplOptions.AggressiveOptimization, they will be jitted instead
+            return;
+        }
+
        // READYTORUN: FUTURE: Producedure spliting


Should we do always do this (regardless of R2R or fragile ngen?) We still use fragile for corelib in 2.2 and (for now) in 3.0.

Perhaps academic for now as there aren't any AggressiveOptimization attributes in corelib, but somebody might add one.

I thought that fragile ngen code would be (at the moment) as good as tier 1 JIT. Is that not the case?

I assume fragile NGEN still wouldn't use more recent hardware instructions, so that is one place it might differ from JIT tier1.

I'll change it for now, though thinking about it more, it may need to be revisited in the future for both r2r and fragile ngen. For methods that don't need any special new instructions and can be aggressively optimized, it may be worth pregenerating the code, and that would have to be determined by the JIT instead of this check.

jkotas · 2018-09-18T18:02:54Z

src/vm/prestub.cpp

@@ -1752,9 +1752,15 @@ PCODE MethodDesc::DoPrestub(MethodTable *pDispatchingMT)
    BOOL fWasPromotedToTier1 = FALSE;
    if (fEligibleForTieredCompilation)
    {
-        fEligibleForCallCounting = g_pConfig->TieredCompilation_CallCounting();
-        if (fEligibleForCallCounting)
+        if (RequestedAggressiveOptimization())


How much slower can the call counting get with this call on the call counting path?

Note that RequestedAggressiveOptimization will access metadata that may include taking locks when the metadata are in R/W mode.

I measured the call counting overhead in MusicStore, AllReady, and CscSource benchmarks, I don't see any noticeable difference with the changes. At the moment I think it's relatively easy to reduce the number of calls to RequestedAggressiveOptimization() per prestub invocation to a 2 - one where you highlighted above, and another in TieredCompilationManager::GetJitFlags. It would be possible to reduce that to 1 by plumbing the value through. Another simpler option may be to do what is done for NoInlining, which uses a bit in MethodDescClassification (mdcNotInline) to store that info in the MethodDesc once. It looks like some bits are available.

Another simpler option may be to do what is done for NoInlining, which uses a bit in MethodDescClassification (mdcNotInline)

I do not think it is worth burning a MethodDesc bit for this.

noahfalk

oo shoot, I just realized I wrote the comments earlier today but never hit 'submit review'. Sorry for the late response Kount.

noahfalk · 2018-09-19T01:55:40Z

src/vm/codeversion.cpp

-    m_optTier(NativeCodeVersion::OptimizationTier0),
+#ifdef FEATURE_TIERED_COMPILATION
+    m_optTier(
+        pMethodDesc->RequestedAggressiveOptimization()


I would also suggest hoisting this call out of the constructor, but only because I view the CodeVersionNode as being a simple data container that shouldn't embed policy. I threw together a quick suggestion here:
noahfalk@697bc9e

noahfalk · 2018-09-19T01:56:57Z

src/vm/codeversion.cpp

@@ -336,7 +341,10 @@ NativeCodeVersion::OptimizationTier NativeCodeVersion::GetOptimizationTier() con
    }
    else
    {
-        return NativeCodeVersion::OptimizationTier0;
+        return


I think we should extract the policy into a distinct method, I suggest TieredCompilation::GetDefaultOptimizationTier

noahfalk · 2018-09-19T02:25:13Z

src/zap/zapinfo.cpp

+            // Skip methods marked with MethodImplOptions.AggressiveOptimization, they will be jitted instead
+            return;
+        }
+
        // READYTORUN: FUTURE: Producedure spliting


I assume fragile NGEN still wouldn't use more recent hardware instructions, so that is one place it might differ from JIT tier1.

noahfalk · 2018-09-19T02:27:45Z

src/vm/prestub.cpp

@@ -1752,9 +1752,15 @@ PCODE MethodDesc::DoPrestub(MethodTable *pDispatchingMT)
    BOOL fWasPromotedToTier1 = FALSE;
    if (fEligibleForTieredCompilation)
    {
-        fEligibleForCallCounting = g_pConfig->TieredCompilation_CallCounting();
-        if (fEligibleForCallCounting)
+        if (RequestedAggressiveOptimization())


I'd suggest trying to keep the policy about which methods need call counting distinct from the code that causes the call counting to occur.

kouvel · 2018-09-27T05:36:41Z

@dotnet-bot test this please

kouvel · 2018-09-27T05:42:14Z

@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0)

kouvel · 2018-09-27T18:31:43Z

The failed "Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0)" appears to be a duplicate broken job, there is another job of the same configuration that was rerun and passed.

@noahfalk I think this PR is ready to go, it would be great if you could take a look at the last commit when you're back.

AndyAyersMS · 2018-09-27T21:30:19Z

If we inform the jit about AggressiveOptimization via jit flags, the jit can't see if potential inlinees have this attribute.

An alternative is to pass this information via the internal method attribs, via a new CORINFO_FLG_ enumerant, then the jit can look for this attribute on inlinees.

Part of fix for https://github.com/dotnet/corefx/issues/32235 Workaround for https://github.com/dotnet/coreclr/issues/19751 - Added and set CORJIT_FLAG_AGGRESSIVE_OPT to indicate that a method is flagged with AggressiveOptimization - For a method flagged with AggressiveOptimization, tiering uses a foreground tier 1 JIT on first call to the method, skipping the tier 0 JIT and call counting - When tiering is disabled, a method flagged with AggressiveOptimization does not use r2r-pregenerated code - R2r crossgen does not generate code for a method flagged with AggressiveOptimization

…ested

kouvel · 2018-10-01T18:29:33Z

Thanks @AndyAyersMS, fixed in the latest commit (also rebased on latest)

AndyAyersMS

LGTM.

noahfalk

LGTM modulo testing I mentioned in the comment

noahfalk · 2018-10-02T01:36:41Z

src/vm/codeversion.cpp

    }
 }
-
-#ifndef DACCESS_COMPILE
-void NativeCodeVersion::SetOptimizationTier(NativeCodeVersion::OptimizationTier tier)


Being able to make this immutable is nice 👍

noahfalk · 2018-10-02T01:53:53Z

src/vm/method.hpp

+        DWORD dwImplFlags = 0;
+        return
+            IsIL() && // only makes sense for IL methods, and this implies !IsNoMetadata()
+            SUCCEEDED(GetMDImport()->GetMethodImplProps(GetMemberDef(), NULL, &dwImplFlags)) &&


I think this could be IsMiAggressiveOptimization(GetImplAttrs()), but I wouldn't worry about it unless you've got other changes to make in addition.

noahfalk · 2018-10-02T02:01:55Z

tests/src/ilasm/System/Runtime/CompilerServices/MethodImplOptionsTests.cs

@@ -0,0 +1,152 @@
+// Licensed to the .NET Foundation under one or more agreements.


Whether in this PR or a later one, it would be good to add a test that confirms a method marked with the AggressiveOptimization attribute doesn't JIT twice. Now that EventPipe appears to work uniformly across platforms and RuntimeEventSource is implemented I think you could build a test that solely uses an in-proc EventListener to observe the jit events. I should have a little sample laying around if its helpful though I haven't explored jit events specifically.

I'll do this in a separate PR, I'll need to add a perf test as well. Filed https://github.com/dotnet/coreclr/issues/20229 to track both.

kouvel · 2018-10-04T15:40:39Z

It looks like the change to MethodImplOptions.cs in CoreLib/shared was not mirrored to corert, is there something I need to do to initiate the mirroring?

jkotas · 2018-10-04T15:43:44Z

@Anipik The mirror seems to be down. Could you please take a look?

Anipik · 2018-10-04T16:27:18Z

@jkotas its done

kouvel · 2018-10-04T17:39:19Z

Thanks!

…otnet#20009) Add MethodImplOptions.AggressiveOptimization and use it for tiering Part of fix for https://github.com/dotnet/corefx/issues/32235 Workaround for https://github.com/dotnet/coreclr/issues/19751 - Added and set CORJIT_FLAG_AGGRESSIVE_OPT to indicate that a method is flagged with AggressiveOptimization - For a method flagged with AggressiveOptimization, tiering uses a foreground tier 1 JIT on first call to the method, skipping the tier 0 JIT and call counting - When tiering is disabled, a method flagged with AggressiveOptimization does not use r2r-pregenerated code - R2r crossgen does not generate code for a method flagged with AggressiveOptimization

Part of fix for https://github.com/dotnet/corefx/issues/32235 Depends on dotnet/coreclr#20009 API review: https://github.com/dotnet/corefx/issues/32235

Picks up the new tiering-related `MethodImpl` option added in dotnet/coreclr#20009. I went ahead and also implemented the `NoOptimization` `MethodImpl` option because it's related and we need to have it to ship anyway.

Part of fix for https://github.com/dotnet/corefx/issues/32235 Depends on dotnet/coreclr#20009 API review: https://github.com/dotnet/corefx/issues/32235

…otnet#20009) Add MethodImplOptions.AggressiveOptimization and use it for tiering Part of fix for https://github.com/dotnet/corefx/issues/32235 Workaround for https://github.com/dotnet/coreclr/issues/19751 - Added and set CORJIT_FLAG_AGGRESSIVE_OPT to indicate that a method is flagged with AggressiveOptimization - For a method flagged with AggressiveOptimization, tiering uses a foreground tier 1 JIT on first call to the method, skipping the tier 0 JIT and call counting - When tiering is disabled, a method flagged with AggressiveOptimization does not use r2r-pregenerated code - R2r crossgen does not generate code for a method flagged with AggressiveOptimization

…tes.AggressiveOptimization (#32322) * Expose MethodImplOptions.AggressiveOptimization Part of fix for https://github.com/dotnet/corefx/issues/32235 Depends on dotnet/coreclr#20009 API review: https://github.com/dotnet/corefx/issues/32235 * Include new test * Expose MethodImplAttributes.AggressiveOptimization and fix test API review: https://github.com/dotnet/corefx/issues/32628

…tes.AggressiveOptimization (dotnet/corefx#32322) * Expose MethodImplOptions.AggressiveOptimization Part of fix for https://github.com/dotnet/corefx/issues/32235 Depends on dotnet/coreclr#20009 API review: https://github.com/dotnet/corefx/issues/32235 * Include new test * Expose MethodImplAttributes.AggressiveOptimization and fix test API review: https://github.com/dotnet/corefx/issues/32628 Commit migrated from dotnet/corefx@3f0a1ee

kouvel added area-ReadyToRun area-System.Runtime area-CodeVersioning labels Sep 17, 2018

kouvel added this to the 3.0 milestone Sep 17, 2018

kouvel self-assigned this Sep 17, 2018

kouvel added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 17, 2018

kouvel force-pushed the NoTierMethod branch from d310b5f to edc2e6a Compare September 17, 2018 19:17

kouvel mentioned this pull request Sep 17, 2018

Expose MethodImplOptions.AggressiveOptimization and MethodImplAttributes.AggressiveOptimization dotnet/corefx#32322

Merged

kouvel requested review from noahfalk, AndyAyersMS and jkotas September 18, 2018 16:48

jkotas reviewed Sep 18, 2018

View reviewed changes

AndyAyersMS reviewed Sep 18, 2018

View reviewed changes

jkotas reviewed Sep 18, 2018

View reviewed changes

noahfalk reviewed Sep 19, 2018

View reviewed changes

kouvel added 6 commits October 1, 2018 10:34

Fix switches

e33595b

If pregenerated r2r code is available, assume it is optimized as requ…

5ef7cd1

…ested

Specify tier when adding a native code version

62dd597

Refactor some things

eca77d2

Use CORINFO_FLG instead of CORJIT_FLAG

380321a

kouvel force-pushed the NoTierMethod branch from bdbf2bc to 380321a Compare October 1, 2018 18:28

AndyAyersMS approved these changes Oct 1, 2018

View reviewed changes

noahfalk approved these changes Oct 2, 2018

View reviewed changes

kouvel added 2 commits October 2, 2018 13:48

Use GetImplAttrs instead

de595c4

Fix build

f5e46d4

kouvel merged commit b68296c into dotnet:master Oct 3, 2018

kouvel deleted the NoTierMethod branch October 3, 2018 15:52

kouvel removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 3, 2018

kouvel mentioned this pull request Oct 4, 2018

[2.2] Port some changes relevant to tiered compilation to 2.2 #20265

Closed

MichalStrehovsky mentioned this pull request Oct 5, 2018

Update RyuJIT dotnet/corert#6429

Merged

kouvel removed area-CodeVersioning labels Dec 21, 2018

benaadams mentioned this pull request Jan 24, 2019

Intrinsicify .SequenceCompareTo(byte, ...) #22127

Merged

zpodlovics mentioned this pull request Mar 19, 2019

Tail opcode being emitted for normal methods, destroys JIT optimizations dotnet/fsharp#6329

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MethodImplOptions.AggressiveOptimization and use it for tiering #20009

Add MethodImplOptions.AggressiveOptimization and use it for tiering #20009

kouvel commented Sep 17, 2018 •

edited

Loading

kouvel commented Sep 17, 2018 •

edited

Loading

jkotas Sep 18, 2018

kouvel Sep 18, 2018

jkotas Sep 18, 2018

kouvel Sep 18, 2018

noahfalk Sep 19, 2018

jkotas Sep 18, 2018 •

edited

Loading

AndyAyersMS Sep 18, 2018

kouvel Sep 18, 2018

noahfalk Sep 19, 2018

kouvel Sep 19, 2018

jkotas Sep 18, 2018

kouvel Sep 18, 2018

jkotas Sep 18, 2018

noahfalk left a comment

noahfalk Sep 19, 2018

noahfalk Sep 19, 2018

noahfalk Sep 19, 2018

noahfalk Sep 19, 2018

kouvel commented Sep 27, 2018

kouvel commented Sep 27, 2018

kouvel commented Sep 27, 2018

AndyAyersMS commented Sep 27, 2018

kouvel commented Oct 1, 2018

AndyAyersMS left a comment

noahfalk left a comment

noahfalk Oct 2, 2018

noahfalk Oct 2, 2018

noahfalk Oct 2, 2018

kouvel Oct 2, 2018

kouvel commented Oct 4, 2018

jkotas commented Oct 4, 2018

Anipik commented Oct 4, 2018

kouvel commented Oct 4, 2018

		@@ -0,0 +1,152 @@
		// Licensed to the .NET Foundation under one or more agreements.

Add MethodImplOptions.AggressiveOptimization and use it for tiering #20009

Add MethodImplOptions.AggressiveOptimization and use it for tiering #20009

Conversation

kouvel commented Sep 17, 2018 • edited Loading

kouvel commented Sep 17, 2018 • edited Loading

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

jkotas Sep 18, 2018 • edited Loading

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

Choose a reason for hiding this comment

noahfalk 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

kouvel commented Sep 27, 2018

kouvel commented Sep 27, 2018

kouvel commented Sep 27, 2018

AndyAyersMS commented Sep 27, 2018

kouvel commented Oct 1, 2018

AndyAyersMS left a comment

Choose a reason for hiding this comment

noahfalk 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

kouvel commented Oct 4, 2018

jkotas commented Oct 4, 2018

Anipik commented Oct 4, 2018

kouvel commented Oct 4, 2018

kouvel commented Sep 17, 2018 •

edited

Loading

kouvel commented Sep 17, 2018 •

edited

Loading

jkotas Sep 18, 2018 •

edited

Loading