-
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
New Microsoft.Toolkit.HighPerformance package #3128
New Microsoft.Toolkit.HighPerformance package #3128
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 🙌 |
High performance API tests
High perf List<T> extensions
I'm really glad you thoroughly added enough tests for this! |
Ahahah yeah, I felt like a whole package like this would definitely require an extensive tests suite to make sure it behaved properly, especially with all the weird stuff going on. Besides, I figured if you guys were to really greenlight this whole project and trust me with building so niche and potentially dangerous APIs, I'd better make sure no to let you down and make things as thoroughly as I could! 😊 |
There seems to be some problem with Github. Should be back soon. |
Also pushing this commit just to re-trigger the CI build, since GitHub went offline and caused the previous one to fail/half midway through
Failed: 0 (0.00%) Nice! |
YES! 😄🎉🎉🎉🚀🚀🚀 |
@michael-hawker leaving some notes here since you mentioned you'd focus on naming/API surface in your review. I keep thinking about @aalmada's suggestion and I have to say on second though he did have a point there. To recap, these are the "ref-like" types included in this PR:
The original inspiration for the name was the Now, I've explained my rationale for using these names in my previous comment (here), in short I followed the naming scheme for collection types (eg.
This approach works too, but I've started wondering whether both these two naming schemes are a bit too much verbose, or just harder to read in general as they have pretty long names in general. So, while thinking about this I had another thought. The
This latter option has the advantage of being both much more compact (especially int[] array = new int[10];
ref int myRef1 = ref array[0];
Ref<int> myRef2 = new Ref<int>(ref array[0]); // same thing Here you can see the advantage of using this new naming scheme: Wanted to hear your thoughts on this before actually going ahead and renaming the types in quesiton, on my end I have to say I'm really liking this idea as I think it simplifies things quite a bit 😄 |
@Sergio0694 like you new idea on simplifying the names down to match the base C# We should also at least have a landing page in the Sample App, but let's add that to the follow-up PR or later. |
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.
This is amazing! Thanks @Sergio0694. I know there's still some follow-up to be done in a smaller follow-on PR, so we can address any API comment/unit test comments there. 🎉🎉🎉
/// </summary> | ||
/// <typeparam name="T">The type of value being boxed.</typeparam> | ||
[DebuggerDisplay("{ToString(),raw}")] | ||
public sealed class Box<T> |
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.
We should have some examples/docs at least inline here if not called out about why/where this could be used.
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.
Done in 018f4a7.
/// </summary> | ||
private T[]? array; | ||
|
||
#pragma warning disable IDE0032 |
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.
leave comments on warnings / reasoning
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.
Done in c309019.
} | ||
|
||
/// <summary> | ||
/// Gets a content hash from the input <see cref="ReadOnlySpan{T}"/> instance using the Djb2 algorithm. |
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.
Add a see link to a proper source of explanation for this Djb2 algorithm?
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.
Let's link to the list of algorithms here and the performance matrix and the reference implementation used 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.
And maybe this?
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.
Thanks for the links! 😄
Done in c07112c.
// Main loop with 8 unrolled iterations | ||
for (; i <= end8; i += 8) | ||
{ | ||
hash = unchecked((hash << 5) + hash + Unsafe.Add(ref r0, i + 0).GetHashCode()); |
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.
Maybe add comment about this optimization 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.
Ah, good idea! 😊
Done in 09c8e88.
} | ||
|
||
/// <summary> | ||
/// Gets a content hash from the input <see cref="Span{T}"/> instance using the Djb2 algorithm. |
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.
Reference other comment with links or same links?
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.
Linked the docs for the base ReadOnlySpan<T>
method, done in c07112c.
namespace Microsoft.Toolkit.HighPerformance.Helpers | ||
{ | ||
/// <summary> | ||
/// Combines the hash code of sequences of <typeparamref name="T"/> values into a single hash code. |
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.
Be sure to call out here as a remark or something that these codes are only guaranteed for the current execution session. Call out reference to other hashcode implementations for repeatable hashes. Probably best to just link out to the same remarks that the system hashcode has from here: https://docs.microsoft.com/en-us/dotnet/api/system.object.gethashcode#remarks
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.
Done in c9a5c4c.
Microsoft.Toolkit.HighPerformance/Microsoft.Toolkit.HighPerformance.csproj
Outdated
Show resolved
Hide resolved
var buffer = MemoryOwner<int>.Allocate(127); | ||
|
||
buffer.Dispose(); | ||
buffer.Dispose(); |
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.
Should there be some checks here to make sure something has happened?
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.
As discussed during the stream (https://www.twitch.tv/xamlllama), I've added a comment to this in 54536e4 to indicate that the test is just to make sure this code doesn't crash.
|
||
Assert.IsNotNull(stream); | ||
Assert.AreEqual(stream.Length, buffer.Length); | ||
Assert.IsTrue(stream.CanWrite); |
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.
We should probably validate that the stream is indeed empty without any data, right?
Likewise, we should also have tests where the original buffer contains some data, and we validate after conversion to a stream that the same data can be read back?
(We can do this in follow-on PR)
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.
Absolutely! Done both in e8a9534.
|
||
[TestCategory("ReadOnlySpanExtensions")] | ||
[TestMethod] | ||
public void Test_ReadOnlySpanExtensions_Tokenize_Csv() |
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.
Might want to change these names later to Comma
as CSV has other parsing implications 😋
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.
Ahahahah right 😄
Done in 6f488cd.
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.
nice information Admin...
i suggest if anyone want full version of latest software he should visit this site
WE DID IT! 🎉🎉🎉 Thank you so much @michael-hawker and @azchohfi for the amazing opportunity and for your time reviewing this, I'm so happy to have been able to contribute with this new toolkit package! 😊 Now on to work on the follow up PR! |
PR Type
What kind of change does this PR introduce?
What is the new behavior?
This PR introduces a new
Microsoft.Toolkit.HighPerformance
package, with a collection of types and extensions that can be used in high-performance scenarios.Below is a breakthrough of all the new types and APIs added in this PR:
Microsoft.Toolkit.HighPerformance.Buffers
Microsoft.Toolkit.HighPerformance.Enumerables
Microsoft.Toolkit.HighPerformance.Extensions
Microsoft.Toolkit.HighPerformance.Helpers
Microsoft.Toolkit.HighPerformance
PR Checklist
Please check if your PR fulfills the following requirements:
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 templatesOther information
cc. @michael-hawker from our previous conversation in the UWP Community Discord server.
The project targets both
.NET Standard 2.1
and.NET Standard 2.0
, so that it can both be available for UWP applications, as well as offering more advanced features for developers working on.NET Core 3.x
. It uses the following packages:System.Runtime.CompilerServices.Unsafe
System.Memory
(only on.NET Standard 2.0
)Microsoft.Bcl.HashCode
(only on.NET Standard 2.0
)Additionally, some APIs are just removed entirely when on
.NET Standard 2.0
, since they require runtime support that's not available on that target (eg theByReference<T>
types).