GC polling in unboxing JIT helpers #32353

VSadov · 2020-02-15T01:49:33Z

moved fast part of Unbox to a managed helper
added GC polling to the native helper that handles rare cases of Unbox.
added GC polling to JIT_Unbox_Nullable native helpers
(it does not look like it is worth the trouble to move any parts of this one to managed)

VSadov · 2020-02-15T02:13:47Z

Initially I assumed that JIT inlines Unbox in most cases and the change would be mostly for the purpose of completeness and predictability.

Turns out Unbox helper calls are common because JIT uses helpers when compiling with MinOpts.

jkotas · 2020-02-15T07:04:22Z

Is there a targeted microbenchmark that shows the improved GC pause times in pathological cases?

jkotas · 2020-02-15T07:06:19Z

src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs

+        [DebuggerStepThrough]
+        private static ref byte Unbox(void* toTypeHnd, object obj)
+        {
+            // this will NRE if obj is null, as expected.


Suggested change

// this will NRE if obj is null, as expected.

// this will throw NullReferenceException if obj is null, as expected.

The number of people with a decoder ring for NRE is pretty small... #Resolved

jkotas · 2020-02-15T07:09:00Z

src/coreclr/src/vm/jithelpers.cpp

+        (pMT1->IsEnum() || pMT1->IsTruePrimitive()) &&
+        (pMT2->IsEnum() || pMT2->IsTruePrimitive()))
+    {
+        // we allow enums and their primtive type to be interchangable


Suggested change

// we allow enums and their primtive type to be interchangable

// we allow enums and their primitive type to be interchangeable

``` #Resolved

jkotas · 2020-02-15T07:09:46Z

src/coreclr/src/vm/jithelpers.cpp

    OBJECTREF objRef = ObjectToOBJECTREF(obj);
    HELPER_METHOD_FRAME_BEGIN_RET_1(objRef);
-    if (TypeHandle(type).IsEquivalentTo(objRef->GetTypeHandle()))
+
+    if (pMT1->GetInternalCorElementType() == pMT2->GetInternalCorElementType() &&


Moving this inside HELPER_METHOD_FRAME will regress performance for enums...

We return a ref to the boxed value. If I want to poll, I need to protect the box. Is there a way to protect without a frame?

In reply to: 379777249 [](ancestors = 379777249)

One way to do this is to use fast path without HMF when g_TrapReturningThreads.LoadWithoutBarrier() is zero, and use polling show path with HMF otherwise. It would duplicate a bit of the logic, but it should not be a huge problem for the unbox helper.

Another way to do this would be to introduce FC_GC_POLL_EX variant for byrefs, but that would be quite a bit of plumbing. I do not think it is worth it.

Right. A flag check would not trash the refs and should be much cheaper.

With suggested changes (g_TrapReturningThreads and GetVerifierCorElementType) the enum case is slightly faster than originally.

That is - as long as g_TrapReturningThreads is checked after comparing GetVerifierCorElementType.
Not sure why the order of compares matters. It is nearly the same codegen. I am guessing the rarely taken branch is more predictable this way.

VSadov · 2020-02-15T16:42:50Z

There are ways to ensure that a helper is called (unbox enum from boxed underlying type, prevent inlining, prevent rejitting), so I assumed a similar repro as with casts can be constructed. I’ll try making one.

VSadov · 2020-02-15T21:21:48Z

The following reports pauses > 100 ms. Sometimes up to half second on my machine.
After the fix it is nearly always 0 ms and nothing is printed.

using System;
using System.Diagnostics;
using System.Threading;

class Program
{
    enum E1
    {
        e
    }

    static object o = E1.e;

    static void Work()
    {
        for(; ;)
        {
            int x = (int)o + (int)o + (int)o + (int)o + (int)o + (int)o + (int)o + (int)o;
        }
    }

    static void Main()
    {
        new Thread(() =>
        {
            Work();
        }).Start();

        for (; ; ) 
        { 
            var sw = Stopwatch.StartNew(); 
            GC.Collect(); 
            sw.Stop();

            if (sw.ElapsedMilliseconds > 100)
            {
                Console.WriteLine(sw.ElapsedMilliseconds);
            }

            Thread.Sleep(1); 
        }
    }
}

jkotas · 2020-02-16T02:21:23Z

src/coreclr/src/vm/jithelpers.cpp

    OBJECTREF objRef = ObjectToOBJECTREF(obj);
    HELPER_METHOD_FRAME_BEGIN_RET_1(objRef);
-    if (TypeHandle(type).IsEquivalentTo(objRef->GetTypeHandle()))
+
+    if (pMT1->GetInternalCorElementType() == pMT2->GetInternalCorElementType() &&


You should be also able to make the enum case faster by using GetVerifierCorElementType. It should make the IsEnum/IsTruePrimitiveType conditions unnecessary.

jkotas · 2020-02-16T05:25:10Z

src/coreclr/src/vm/jithelpers.cpp

-        }
+    // we allow enums and their primitive type to be interchangable.
+    // if suspension is requested, defer to the framed helper.
+    if (pMT1->GetVerifierCorElementType() == pMT2->GetVerifierCorElementType() &&


This also needs to check that the CorElementType is primitive (ie != ELEMENTTYPE_VALUETYPE?)

It looks like the fastest check is

if (pMT1->GetInternalCorElementType() == pMT2->GetInternalCorElementType() && (pMT1->IsEnum() || pMT1->IsTruePrimitive()) && (pMT2->IsEnum() || pMT2->IsTruePrimitive())) {

Why not this?

CorElementType corElementType = pMT1->GetVerifierCorElementType(); if (corElementType != ELEMENT_TYPE_VALUETYPE && corElementType == pMT2->GetVerifierCorElementType()) {

It is yet a bit faster. :-)

jkotas · 2020-02-16T05:26:14Z

src/coreclr/src/vm/interpreter.cpp

@@ -8698,7 +8698,7 @@ void Interpreter::UnboxAny()
                    CorElementType type1 = pMT1->GetInternalCorElementType();


Change it here to GetVerifierCorElementType as well?

Changed the interpreter too. Do we have any tests for that?

jkotas · 2020-02-16T11:51:46Z

src/coreclr/src/vm/jithelpers.cpp

-        }
+    // we allow enums and their primitive type to be interchangable.
+    // if suspension is requested, defer to the framed helper.
+    CorElementType corElementType = pMT1->GetInternalCorElementType();


Suggested change

CorElementType corElementType = pMT1->GetInternalCorElementType();

CorElementType corElementType = pMT1->GetVerifierCorElementType();

Multiple places. I had the same bug in my comment and fixed it, but I guess you have seen the original buggy version of the comment.

It seemed that GetInternalCorElementType could be actually correct here and cheaper than GetVerifierCorElementType

When we get here, since this is Unbox, the target type is a struct, enum or a true primitive

We are not interested in structs, since they either should have matched already or need an equivalency check.

For others we just need the GetInternalCorElementType to match.

From what I see the subtleness is about types that are primitives, but not true primitives. I am not sure what they are (byrefs?, void?), but I guess we should not see them here.

In reply to: 379897103 [](ancestors = 379897103)

types that are primitives, but not true primitives

These are structs that wrap a primitive type on x86.

In the early days of .NET Framework, we had a performance problem with structs that wrap a primitive type (there is a lot of struct like this). The JIT had close to none optimizations for structs at that time, and fixing it the right way was very hard. So we invented this quick hack: Treat them as the primitive type that they are wrapping. This is the type that GetInternalCorElementType returns.

GetInternalCorElementType should be only ever used in places that deal with calling convention.

If you seeing GetVerifierCorElementType to be more expensive than GetInternalCorElementType , you should be able to make GetVerifierCorElementType just return GetInternalCorElementType everywhere except x86. The two should return identical element type everywhere except x86.

I see. Yes, GetVerifierCorElementType is noticeably more expensive.

Hmmm, we are using the fake cor element types on !x86 as well in a few cases:

runtime/src/coreclr/src/vm/methodtablebuilder.cpp

Line 9759 in 4f9ae42

else if (strcmp(name, g_RuntimeArgumentHandleName) == 0)

. So using GetInternalCorElementType on !x86 may not be a good idea.

I have tried

if (pMT1->GetInternalCorElementType() == pMT2->GetInternalCorElementType() && (pMT1->IsEnum() || pMT1->IsTruePrimitive()) && (pMT2->IsEnum() || pMT2->IsTruePrimitive()) &&

vs

CorElementType corElementType = pMT1->GetVerifierCorElementType(); if (corElementType != ELEMENT_TYPE_VALUETYPE && corElementType == pMT2->GetVerifierCorElementType() &&

The first variant is faster. (20%-30% on an Unbox microbenchmark)
I think the checks are equivalent, unless I am missing something.

jkotas · 2020-02-16T11:53:19Z

src/coreclr/src/vm/interpreter.cpp

@@ -8534,7 +8534,7 @@ void Interpreter::Unbox()
                CorElementType type1 = pMT1->GetInternalCorElementType();
                CorElementType type2 = pMT2->GetInternalCorElementType();

-                // we allow enums and their primtive type to be interchangable
+                // we allow enums and their primitive type to be interchangable
                if (type1 == type2)
                {
                    if ((pMT1->IsEnum() || pMT1->IsTruePrimitive()) &&


This place too?

jkotas · 2020-02-16T11:54:22Z

src/coreclr/src/vm/jithelpers.cpp

-    }
-
-    // Even less common cases (type equivalence) go to a framed helper.
-    ENDFORBIDGC();


You need to keep ENDFORBIDGC(); before forwarding the helper call. It will fix some of the test failures hit by the CI.

jkotas · 2020-02-16T11:55:33Z

src/coreclr/src/vm/jithelpers.cpp

    }
+
+    HELPER_METHOD_POLL(); 


Result is not protected here. Move the POLL right after HELPER_METHOD_FRAME_BEGIN_RET_1 #Resolved

Or protect the result, but moving the poll earlier is simpler.

In reply to: 379897330 [](ancestors = 379897330)

VSadov · 2020-02-16T23:42:42Z

Thanks!!!

GC polling in unboxing JIT helpers

af57efd

VSadov requested a review from jkotas February 15, 2020 01:49

jkotas added the tenet-performance Performance related issue label Feb 15, 2020

jkotas reviewed Feb 15, 2020

View reviewed changes

PR feedback

367e70e

jkotas reviewed Feb 16, 2020

View reviewed changes

more PR feedback

0af207a

jkotas reviewed Feb 16, 2020

View reviewed changes

VSadov added 2 commits February 15, 2020 23:37

PR feedback

ec839d8

more PR feedback

55a812c

jkotas reviewed Feb 16, 2020

View reviewed changes

couple fixes (missed ENDFORBIDGC and moving GC poll earlier)

7c325e0

VSadov force-pushed the unbox branch from d3ad934 to 7c325e0 Compare February 16, 2020 16:29

further tuning the type check for enums

3f71c1d

jkotas approved these changes Feb 16, 2020

View reviewed changes

VSadov merged commit dfa5d03 into dotnet:master Feb 16, 2020

VSadov deleted the unbox branch February 16, 2020 23:42

richlander mentioned this pull request Jun 6, 2020

Improving P95+ latency #37534

Closed

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

	// this will NRE if obj is null, as expected.
	// this will throw NullReferenceException if obj is null, as expected.

	// we allow enums and their primtive type to be interchangable
	// we allow enums and their primitive type to be interchangeable
	``` #Resolved

		@@ -8698,7 +8698,7 @@ void Interpreter::UnboxAny()
		CorElementType type1 = pMT1->GetInternalCorElementType();

	CorElementType corElementType = pMT1->GetInternalCorElementType();
	CorElementType corElementType = pMT1->GetVerifierCorElementType();

GC polling in unboxing JIT helpers #32353

GC polling in unboxing JIT helpers #32353

Uh oh!

Conversation

VSadov commented Feb 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VSadov commented Feb 15, 2020

Uh oh!

jkotas commented Feb 15, 2020

Uh oh!

jkotas Feb 15, 2020 • edited by VSadov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Feb 15, 2020 • edited by VSadov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov commented Feb 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VSadov commented Feb 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Feb 16, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov Feb 16, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Feb 16, 2020 • edited by VSadov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov commented Feb 16, 2020

Uh oh!

VSadov commented Feb 15, 2020 •

edited

Loading

jkotas Feb 15, 2020 •

edited by VSadov

Loading

jkotas Feb 15, 2020 •

edited by VSadov

Loading

VSadov commented Feb 15, 2020 •

edited

Loading

jkotas Feb 16, 2020 •

edited

Loading

VSadov Feb 16, 2020 •

edited

Loading

jkotas Feb 16, 2020 •

edited by VSadov

Loading