Inlined GC poll for methods marked with SuppressGCTransitionAttribute

[AaronRobinsonMSFT]

The code responsible for creating and inserting GC_Poll instances appears to have degraded since porting over from .NET Framework.

runtime/src/coreclr/src/jit/flowgraph.cpp

Line 3517 in 14a2f78

void Compiler::fgCreateGCPolls()

Calling the above function when either GCPOLL_CALL or GCPOLL_INLINE is set results in multiple asserts being fired. This was discovered during the suppress GC work - dotnet/coreclr#26458. The result of this was to manually insert GCPOLL_CALL instances instead of GCPOLL_INLINE.

Additional issues:

• Revert removal of SuppressGCTransition from SystemNative_GetTimestamp() coreclr#27473

/cc @briansull @dotnet/jit-contrib

category:cq
theme:optimization
skill-level:beginner
cost:medium

[jkotas]

The SuppressGCTransitionAttribute optimization leaves performance on the table because of this issue.

[briansull]

This will analyze loops to prove that a managed call always occurs in the execution of one iteration.
If it proves this it will omit the GC_POLL call for the loop.
When we discussed this behavior during dotnet/coreclr#26458, you advocated for always calling the GC_POLL rather than optimizing it away.

[jkotas]

How you recommend that we implement the inlined GC poll for SuppressGCTransitionAttribute? Ignore what is there today and just do it from scratch?

[AaronRobinsonMSFT]

@briansull The issue here was that during some scenarios no assert was hit with GCPOLL_INLINE, but there were cases when it would assert. Asserts also occurred when setting GCPOLL_CALL. There are obviously multiple scenarios here.

Inline PInvoke in an inlineable function.

   public static int GetTime()
   {
       return InlineablePInvokeWithGCSuppression();
   }

PInvoke in non-inlined function

   public static int GetAccTime()
   {
       int acc = 0;
       foreach (...)
       {
          acc += InlineablePInvokeWithGCSuppression();
       }

       return acc;
   }

Then there are scenarios involving CrossGen which can't use GCPOLL_INLINE at the moment, but requires GCPOLL_CALL - see https://github.com/dotnet/coreclr/issues/27206.

[briansull]

How you recommend that we implement the inlined GC poll for SuppressGCTransitionAttribute?

Yes, We should fix the codepath that we use for SuppressGCTransitionAttribute to support GCPOLL_INLINE in all cases.

The existing code fgCreateGCPolls() was last used to support the MacOS during the pre-RyuJIT time frame. That code should be deleted as it tries to eliminate the GCPOLL call when it can prove that a managed call will be made in every loop iteration, which is not what we want with this new feature.

[BruceForstall]

@briansull You fixed some of the GCPOLL code with #34837.

Did you try changing the code in morph that generates the GCPOLL_CALL for the SuppressGCTransitionAttribute case to GCPOLL_INLINE? That is, here:

    if (fgGlobalMorph && call->IsUnmanaged() && call->IsSuppressGCTransition())
    {
        // Insert a GC poll.
        bool insertedBB = fgCreateGCPoll(GCPOLL_CALL, compCurBB, compCurStmt);
        assert(!insertedBB); // No new block should be inserted
    }

Presumably you could do this and see asm diffs where the polls are inlined in the tests Interop\PInvoke\Attributes\SuppressGCTransition\SuppressGCTransitionTest.cs and JIT\Methodical\gc_poll\InsertGCPoll.cs.

[briansull]

I will take a look at making this change and seeing what asm diffs there are with it.

[BruceForstall]

@AaronRobinsonMSFT @jkotas Is there a benchmark where we could measure a performance improvement by implementing inlined GC polls for this scenario?

[AaronRobinsonMSFT]

I do not know of one at present. @VSadov was investigating using this attribute and it wasn't as fast as needed for him. He may have a scenario to validate.

[VSadov]

I did not notice that this has merged.

One important scenario that is blocked right now is the Math functions. - #13820

That could use SuppressGCTransition, but would require that:

• GC poll is inlined
• We do something smart with GC polls in a block
  No need to poll for every function when user does:

Math.Acos(Math.Cos(Math.Acos(Math.Cos(Math.Acos(Math.Cos(Math.Cos(x)))))))

I assume the first part is now fixed?