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

[NativeAOT] Missed devirtualization opportunities due to Unsafe.As #85906

Closed
MichalStrehovsky opened this issue May 8, 2023 · 3 comments · Fixed by #85954
Closed

[NativeAOT] Missed devirtualization opportunities due to Unsafe.As #85906

MichalStrehovsky opened this issue May 8, 2023 · 3 comments · Fixed by #85954
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Comments

@MichalStrehovsky
Copy link
Member

In the below program we miss a devirtualization opportunity in Method due to the Unsafe.As.

IFace has a single implementor but RyuJIT doesn't even ask for it.

using System;
using System.Runtime.CompilerServices;

interface IFace
{
    void Frob();
}

class Program : IFace
{
    public static IFace s_inst = new Program();

    public object inst;

    public Program() => inst = s_inst;

    public void Frob() { }

    [MethodImpl(MethodImplOptions.NoInlining)]
    public void Method() => Unsafe.As<IFace>(inst).Frob();

    static void Main()
    {
        new Program().Method();
    }
}

I noticed this in the pattern from here as I was looking why interface dispatch is so hot (2.7% of samples spent in RhpInterfaceDispatch1 alone) in BasicMinimalApis:

return Unsafe.As<IValueTaskSource<TResult>>(obj).GetStatus(_token) != ValueTaskSourceStatus.Pending;

Cc @EgorBo

@MichalStrehovsky MichalStrehovsky added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 8, 2023
@ghost
Copy link

ghost commented May 8, 2023

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

Issue Details

In the below program we miss a devirtualization opportunity in Method due to the Unsafe.As.

IFace has a single implementor but RyuJIT doesn't even ask for it.

using System;
using System.Runtime.CompilerServices;

interface IFace
{
    void Frob();
}

class Program : IFace
{
    public static IFace s_inst = new Program();

    public object inst;

    public Program() => inst = s_inst;

    public void Frob() { }

    [MethodImpl(MethodImplOptions.NoInlining)]
    public void Method() => Unsafe.As<IFace>(inst).Frob();

    static void Main()
    {
        new Program().Method();
    }
}

I noticed this in the pattern from here as I was looking why interface dispatch is so hot (2.7% of samples spent in RhpInterfaceDispatch1 alone) in BasicMinimalApis:

return Unsafe.As<IValueTaskSource<TResult>>(obj).GetStatus(_token) != ValueTaskSourceStatus.Pending;

Cc @EgorBo

Author: MichalStrehovsky
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 8, 2023
@EgorBo
Copy link
Member

EgorBo commented May 8, 2023

Dup of #84377

Currently Unsafe.As is just a no-op for JIT, I'll take a look today/tomorrow

@EgorBo EgorBo self-assigned this May 8, 2023
@EgorBo EgorBo removed the untriaged New issue has not been triaged by the area owner label May 8, 2023
@EgorBo
Copy link
Member

EgorBo commented May 8, 2023

Will keep it open since it also involves single-impl devirtualization

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 8, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 9, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 9, 2023
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants