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

Remove unnecessary Unsafe.As call from StringValues #112507

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

GrabYourPitchforks
Copy link
Member

(Caught by static analysis.)

This is essentially calling Unsafe.As<string>(string), which is a no-op. Remove the call to Unsafe.As.

@Copilot Copilot bot review requested due to automatic review settings February 12, 2025 23:28

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-extensions-primitives
See info in area-owners.md if you want to be subscribed.

@tarekgh tarekgh added this to the 10.0.0 milestone Feb 12, 2025
@EgorBo
Copy link
Member

EgorBo commented Feb 13, 2025

Other Unsafe.As look also a bit unnecessary there as well, cast from object to string or string[] is as cheap as

       mov      rcx, 0x7FF89056B608      ; System.String[]
       cmp      qword ptr [rax], rcx
       je       SHORT G_M23900_IG07

@lewing
Copy link
Member

lewing commented Feb 13, 2025

/ba-g the server2025 failures were due to an ES rollout

@tarekgh tarekgh merged commit 66b39df into main Feb 13, 2025
83 of 88 checks passed
@GrabYourPitchforks
Copy link
Member Author

Other Unsafe.As look also a bit unnecessary there as well, cast from object to string or string[] is as cheap as

       mov      rcx, 0x7FF89056B608      ; System.String[]
       cmp      qword ptr [rax], rcx
       je       SHORT G_M23900_IG07

@EgorBo I agree it would be worth changing that as part of a larger effort to remove use of unsafe code where it's not giving a meaningful benefit. But I figured this one-line change would be uncontroversial because the use of unsafe code in this instance can be shown to provide zero benefit. :)

@GrabYourPitchforks GrabYourPitchforks deleted the GrabYourPitchforks-patch-1 branch February 13, 2025 20:36
@stephentoub
Copy link
Member

Other Unsafe.As look also a bit unnecessary there as well, cast from object to string or string[] is as cheap as

I agree it would be worth changing that as part of a larger effort to remove use of unsafe code where it's not giving a meaningful benefit.

I would not be surprised if changing this to a real cast showed up on ASP.NET benchmarks, as I understand this to be on some critical paths.

If it doesn't affect any ASP.NET benchmarks, sure, kill it.

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

Successfully merging this pull request may close these issues.

5 participants