-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
HighPerformance package improvements #3351
HighPerformance package improvements #3351
Conversation
Thanks Sergio0694 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@azchohfi this looks straight-forward enough to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool @Sergio0694 👍. I love work to minimize on unnecessary allocations. I am curious about switching from Unsafe.Unbox<T>(box)
from what I read, it seemed pretty efficient.
@@ -125,7 +125,7 @@ public static bool TryGetFrom(object obj, [NotNullWhen(true)] out Box<T>? box) | |||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | |||
public static implicit operator T(Box<T> box) | |||
{ | |||
return Unsafe.Unbox<T>(box); | |||
return (T)(object)box; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious what the difference is here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory the codegen should be identical, but this version has a better IL encoding which should help with scenarios such as R2R when using crossgen (which has some inlining limitations with method calls), as well as cases where the indirect load might cause stack spills or just not be optimized best by the JIT (especially on older runtimes).
The difference is that the previous version basically invoked that generic Unsafe.Unbox<T>
and then issued a ldobj !!T
instruction to dereference that T&
value on the stack, whereas the second is simply resolved into a single unbox.any !!T
opcode, which is the same being used by normal unbox operations. This change is mostly just a precaution to help crossgen and the JIT to always produce the most optimal codegen possible, it has no actual functional changes.
If you're interested, I made a small repro you can check out (here) - you can see that the asm x64 codegen is exactly the same on .NET Core 3.1, but if you switch to the IL tab you'll be able to see that different encoding I mentioned 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, that's pretty cool (the change and sharplab.io). Thanks @Sergio0694 😃
Hello @Sergio0694! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Improvements for
Microsoft.Toolkit.HighPerformance
PR Type
What kind of change does this PR introduce?
Changes and fixes
ArrayPool<T>
overloads toMemoryOwner<T>
PR Checklist
Please check if your PR fulfills the following requirements:
Pull Request has been submitted to the documentation repository instructions. Link:Sample in sample app has been added / updated (for bug fixes / features)Icon has been created (if new sample) following the Thumbnail Style Guide and templates