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.

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

Struct interface dispatch still allocating for ref generics #9947

Closed
stephentoub opened this issue Mar 16, 2018 · 4 comments
Closed

Struct interface dispatch still allocating for ref generics #9947

stephentoub opened this issue Mar 16, 2018 · 4 comments
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI tenet-performance Performance related issue
Milestone

Comments

@stephentoub
Copy link
Member

dotnet/coreclr#14698 helped to avoid allocations when invoking methods on structs cast to interfaces, but it appears it didn't completely address it.

This code:
https://github.com/dotnet/coreclr/blob/8758f76b4e5ba6733e4f3184293414fc9182d06f/src/mscorlib/src/System/Runtime/CompilerServices/AsyncMethodBuilder.cs#L392-L398
takes advantage of this feature, and with a repro like:

using System;
using System.Threading;
using System.Threading.Tasks;

class Program
{
    static async Task Main()
    {
        for (int i = 0; i < 100; i++)
        {
            await new ValueTask<int>(Task.Delay(1).ContinueWith(_ => default(int))).ConfigureAwait(false);
        }
    }
}

it successfully avoids the allocation, but change the ints in the above to strings, and it starts allocating boxes for the interface:

using System;
using System.Threading;
using System.Threading.Tasks;

class Program
{
    static async Task Main()
    {
        for (int i = 0; i < 100; i++)
        {
            await new ValueTask<string>(Task.Delay(1).ContinueWith(_ => default(string))).ConfigureAwait(false);
        }
    }
}

@AndyAyersMS, could you take a look?

We either need to fix this for 2.1 (if you can that would be excellent), or in 2.1 I need to change the async method builder code to work a different way, as we can't have this allocation happening per await.

@AndyAyersMS
Copy link
Member

There is a chain of optimizations required here and the jit hits a limitation at the last step.

Importing BB09 (PC=157) of 'AwaitUnsafeOnCompleted(byref,byref):this'
    [ 0] 157 (0x09d) ldarg.1
    [ 1] 158 (0x09e) ldobj 1B00000B
    [ 1] 163 (0x0a3) box 1B00000B
Compiler::impImportAndPushBox -- handling BOX(value class) via inline allocate/copy sequence

    [ 1] 168 (0x0a8) castclass 02000755
Considering optimization of castclass ....
Cast will succeed, optimizing to simply return input

    [ 1] 173 (0x0ad) ldloc.0
    [ 2] 174 (0x0ae) callvirt 06005066

impDevirtualizeCall: Trying to devirtualize interface call:
    class for 'this' is ConfiguredValueTaskAwaiter[__Canon] [exact] (attrib 21830010)
    base method is System.Runtime.CompilerServices.IValueTaskAwaiter::AwaitUnsafeOnCompleted
    exact; can devirtualize

Now have direct call to boxed entry point, looking for unboxed entry point
Found unboxed entry point, but it needs method table arg, deferring

To get rid of the box we need the full chain to fire.

I don't recall offhand why the jit didn't or couldn't handle the case where the unboxed entry needs an extra argument. Let me dig in some more.

@stephentoub
Copy link
Member Author

Let me dig in some more.

Thanks for looking into it...

@jkotas
Copy link
Member

jkotas commented Mar 16, 2018

change the ints in the above to strings, and it starts allocating boxes for the interface

Looks like a problem with inlining of dictionary access. The current JIT/EE interface contract is not designed to track enough type information details to inline dictionary access.

It may be possible to fix this if the chain originates from non-generic code. But once you start calling it from generic code and change string to T or List<T>, it will fail to devirtualize again and that case is hard to fix.

@AndyAyersMS
Copy link
Member

We don't have to make it all the way to inlining here -- if we can rework the call to invoke the unboxed entry point we can remove the upstream allocation.

I have a prototype that seems to be working...let me test it a bit more.

Assumption here is that whatever value the jit was going to pass to the newobj helper is the right value to pass as the context arg to the unboxed entry point (in other words in the unboxing stub that we'd normally call, the context always comes from the this).

AndyAyersMS referenced this issue in AndyAyersMS/coreclr Mar 17, 2018
Improve the jit's ability to optimize a box-interface call sequence
to handle cases where the unboxed entry point requires a method table
argument.

Added two test cases, one from the inspiring issue, and another where
the jit is then able to inline the method.

Closes #16982.
AndyAyersMS referenced this issue in AndyAyersMS/coreclr Mar 17, 2018
Improve the jit's ability to optimize a box-interface call sequence
to handle cases where the unboxed entry point requires a method table
argument.

Added two test cases, one from the inspiring issue, and another where
the jit is then able to inline the method.

Closes #16982.
AndyAyersMS referenced this issue in dotnet/coreclr Mar 20, 2018
Improve the jit's ability to optimize a box-interface call sequence
to handle cases where the unboxed entry point requires a method table
argument.

Added two test cases, one from the inspiring issue, and another where
the jit is then able to inline the method.

Closes #16982.
@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI tenet-performance Performance related issue
Projects
None yet
Development

No branches or pull requests

4 participants