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

Try skipping generation of empty method dictionaries #82591

Merged
merged 2 commits into from Mar 2, 2023

Conversation

MichalStrehovsky
Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Feb 24, 2023

Say we're compiling Foo<__Canon>.Method for:

class Foo<T>
{
    static void Method() => GenericMethod<List<T>>();
    static void GenericMethod<T>() { }
}

In the method body, we're generating a call to GenericMethod<__Canon> with a generic dictionary that we looked up from Foos dictionary. But as you can see, the dictionary is empty because GenericMethod doesn't do anything with it's T. RyuJIT might even inline it.

The problem is that we computed how the dictionary will look like during scanning and we're forever stuck with instruction to generate a generic dictionary for every Foo<T> instantiation.

This is adding an optimization - if during scanning we find out that the dictionary layout of the target method is empty, we skip the slot. If RyuJIT ends up asking for something, we just give it garbage. The garbage will not be dereferenced because the dictionary layout was empty and there's nothing to look up from it. we instruct RyuJIT to generate a null generic context pointer.

Saves 1.5% on BasicMinimalApi.

Cc @dotnet/ilc-contrib

Say we're compiling `Foo<__Canon>.Method` for:

```csharp
class Foo<T>
{
    static void Method() => GenericMethod<List<T>>();
    static void GenericMethod<T>() { }
}
```

In the method body, we're generating a call to `GenericMethod<__Canon>` with a generic dictionary that we looked up from `Foo`s dictionary. But as you can see, the dictionary is empty because `GenericMethod` doesn't do anything with it's T. RyuJIT might even inline it.

The problem is that we computed how the dictionary will look like during scanning and we're forever stuck with instruction to generate a generic dictionary for every `Foo<T>` instantiation.

This is adding an optimization - if during scanning we find out that the dictionary layout of the target method is empty, we skip the slot. If RyuJIT ends up asking for something, we just give it garbage. The garbage will not be dereferenced because the dictionary layout was empty and there's nothing to look up from it.

Saves 1.5% on BasicMinimalApi.
@jkotas
Copy link
Member

jkotas commented Feb 24, 2023

If RyuJIT ends up asking for something, we just give it garbage.

Can we tell RyuJIT to use null as the dictionary value in this case?

It is confusing to pass garbage as the dictionary pointer. Also, managed debuggers give you better experience when the dictionary pointer is available, that won't be possible to do reliably if we start passing in garbage.

@MichalStrehovsky
Copy link
Member Author

If RyuJIT ends up asking for something, we just give it garbage.

Can we tell RyuJIT to use null as the dictionary value in this case?

It is confusing to pass garbage as the dictionary pointer. Also, managed debuggers give you better experience when the dictionary pointer is available, that won't be possible to do reliably if we start passing in garbage.

What direction would you prefer? Make a null slot and tell RyuJIT to look up that (worse size, but easier to implement), or extend jitinterface to express this?

@jkotas
Copy link
Member

jkotas commented Feb 25, 2023

It would be nice to tell the JIT to pass NULL as the dictionary pointer.

@ghost
Copy link

ghost commented Feb 25, 2023

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

Issue Details

Say we're compiling Foo<__Canon>.Method for:

class Foo<T>
{
    static void Method() => GenericMethod<List<T>>();
    static void GenericMethod<T>() { }
}

In the method body, we're generating a call to GenericMethod<__Canon> with a generic dictionary that we looked up from Foos dictionary. But as you can see, the dictionary is empty because GenericMethod doesn't do anything with it's T. RyuJIT might even inline it.

The problem is that we computed how the dictionary will look like during scanning and we're forever stuck with instruction to generate a generic dictionary for every Foo<T> instantiation.

This is adding an optimization - if during scanning we find out that the dictionary layout of the target method is empty, we skip the slot. If RyuJIT ends up asking for something, we just give it garbage. The garbage will not be dereferenced because the dictionary layout was empty and there's nothing to look up from it.

Saves 1.5% on BasicMinimalApi.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: MichalStrehovsky
Labels:

area-NativeAOT-coreclr

Milestone: -

@MichalStrehovsky
Copy link
Member Author

It would be nice to tell the JIT to pass NULL as the dictionary pointer.

Updated to do this. It shaves off another 2 kB, but I like it less. It was nicer when RyuJIT didn't need to be aware.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@jkotas
Copy link
Member

jkotas commented Feb 27, 2023

Would it be worth it to extend this to callsites in non-generic code?

@MichalStrehovsky
Copy link
Member Author

Would it be worth it to extend this to callsites in non-generic code?

I did a quick hack and it indicates about 0.08% savings for BasicMinimalApi so maybe not worth it given this would be another crosscutting thing that requires touching JitInterface.

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-extra-platforms

@MichalStrehovsky
Copy link
Member Author

@EgorBo could you please have a look at the JIT change?

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, jit side LGTM!

@MichalStrehovsky MichalStrehovsky merged commit 0b82c57 into dotnet:main Mar 2, 2023
@MichalStrehovsky MichalStrehovsky deleted the emptydict branch March 2, 2023 01:15
@dotnet dotnet locked as resolved and limited conversation to collaborators Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants