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

Convert some instances of Unsafe.SizeOf and Unsafe.As to regular unsafe code #78741

Merged
merged 1 commit into from
Nov 23, 2022

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Nov 23, 2022

C# 11 allows use of managed types with pointers and sizeof. Replacing Unsafe.* with regular unsafe code makes the binaries a bit smaller and saves time and memory at runtime.

…fe code

C# 11 allows use of managed types with pointers and sizeof. Replacing Unsafe.* with regular unsafe code makes the binaries a bit smaller and saves time and memory at runtime.
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@jkotas jkotas changed the title Convert some instances of Unsafe.SizeOf and Unsafe.As to regular unsafe Convert some instances of Unsafe.SizeOf and Unsafe.As to regular unsafe code Nov 23, 2022
@MichalPetryka
Copy link
Contributor

Does it make sense to globally supress the warning in runtime code with this?

@jkotas
Copy link
Member Author

jkotas commented Nov 23, 2022

Does it make sense to globally suppress the warning in runtime code with this?

The repo coding style seems to prefer targeted #pragma warning disable. There are 1000+ #pragma warning disable under libraries.

I do not have a strong opinion on this.

@Sergio0694
Copy link
Contributor

Sergio0694 commented Nov 23, 2022

I have a somewhat related question, this seemed like a good place to ask 😄

Is this type of transformation also valid?

// C# 10
static unsafe void M1(object obj)
{
    Unsafe.As<Action>(obj)();
}

// C# 11
static unsafe void M2(object obj)
{
    (*(Action*)&obj)();
}

The second version is 2 bytes shorter in IL, and doesn't require JIT work to inline the call.

AFAIK this should be safe given that taking the address will also address-expose the target, so it'll remain alive. If this is indeed the case and this type of change is ok, I'm wondering whether it wouldn't also make sense to leverage this? I remember there's a bunch of similar cases in the runtime, eg. several Task-related APIs are using this to schedule continuation callbacks 🤔

@jkotas
Copy link
Member Author

jkotas commented Nov 23, 2022

Yes, it should work fine. It is a bit less readable in my opinion.

several Task-related APIs are using this to schedule continuation callbacks

Task infrastructure is not small. It would save a drop in a bucket there. I think it would more about style preference rather than perf optimizations there.

@teo-tsirpanis
Copy link
Contributor

I'm wondering why it emits a warning. What could go wrong with it? Or is it just that previous assumptions about pointers are no longer true?

@jkotas
Copy link
Member Author

jkotas commented Nov 23, 2022

I'm wondering why it emits a warning

It is trying to tell you "hey, this is probably not doing what you are looking for".

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Let's stick with the pragmas for this PR but reconsider suppressing globally after we get more experience with mistakes folks might make.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants