Utilize ImmutableCollectionsMarshal to get ROS for XxHash128.Hash call #73692
Conversation
There is apparently a *significant* advantage to being able to utilize the static Hash method on XxHash128
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
|
Frankly, these numbers look too good, so I'm a bit skeptical |
| public static Checksum CreateNew(ArrayBuilder<Checksum> checksums) | ||
| { | ||
| var checksumsCount = checksums.Count; | ||
| if (checksumsCount <= 100) |
There was a problem hiding this comment.
@stephentoub does runtime have a general rule of thumb about reasonable stackalloc sizes? for context, each item in this array is 16 bytes, so this would be up to 1.6k alloc (on a leaf node of the stack, no recursion). Is that in line with what you guys do?
There was a problem hiding this comment.
We generally avoid going above 1k
There was a problem hiding this comment.
OK, current changes should not exceeed 1 KB
| public static Checksum Create(ImmutableArray<Checksum> checksums) | ||
| => Create(checksums, static (checksums, writer) => | ||
| { | ||
| foreach (var checksum in checksums) | ||
| checksum.WriteTo(writer); | ||
| }); | ||
|
|
||
| public static Checksum CreateNew(ImmutableArray<byte> bytes) |
There was a problem hiding this comment.
one nagging issue i have with this is ensuring we don't end up in collision situations. So, like getting the checksum for an empty array of bytes, will produced the same checksum as an empty enumerable of strings. I don't know if that's a concern or not. And it's likely a problem external to this PR.
|
@ToddGrun can you write some test helpers that hash teh old way and the new way? to validate you're getting the same hashes? |
Seems reasonable to me. We're going from N chatty calls to keep hashing tiny bits of data, to one chunky call where the data can be hashed in its entirety :) Should be easy to verify with some tests. This is what i did as well to validate that our NetCore impls of xxhash matches the NetFx versions (where we don't have access to all the goodness netcore provides). We have tests that still demonstrate you get the exact same hashed values out. |
| var count = Math.Min(maxStackAllocCount, checksumsCount - checksumsIndex); | ||
|
|
||
| for (var checksumsSpanIndex = 0; checksumsSpanIndex < count; checksumsIndex++) | ||
| checksumsSpan[checksumsSpanIndex++] = checksums[checksumsIndex]; |
There is apparently a significant advantage to being able to utilize the static Hash method on XxHash128
Here are the numbers I see in commit 1 (has both old and new code still present so can be measured side by side)