Optimise Guard.IsBitwiseEqual #3325
Conversation
|
Thanks john-h-k 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 🙌 |
ghost
assigned 
|
cc @Sergio0694 |
|
@john-h-k would you mind just resolving the conflicts before we review this? |
|
Done! @azchohfi |
Co-authored-by: Sergio Pedri <sergio0694@live.com>
|
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
Co-authored-by: Sergio Pedri <sergio0694@live.com>
Apologies for multiple commits I'm.on a phone
|
@azchohfi thoughts here? I think my main question is around the Otherwise, if you and @Sergio0694 approve, I'm good, as long as it'd be in not too late Friday. |
|
@john-h-k A couple notes related to this PR, then it looks great!
@michael-hawker The |
If anything, I strongly dislike the current avoidal of
|
|
@john-h-k On that last point, I have no objections if we want to use If nothing else, that'd also make the code more consistent, since you did use |
…oolkit into improve-guard
|
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
|
@john-h-k Code looks fine now but the CI is failing. |
|
This is the error from the build log, but I'm not sure what that means: |
Co-authored-by: Sergio Pedri <sergio0694@live.com>
|
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
|
@john-h-k Found a workaround to make .NET Native happy. /// <summary>
/// Asserts that the input value must be a bitwise match with a specified value.
/// </summary>
/// <typeparam name="T">The type of input values to compare.</typeparam>
/// <param name="value">The input <typeparamref name="T"/> value to test.</param>
/// <param name="target">The target <typeparamref name="T"/> value to test for.</param>
/// <param name="name">The name of the input parameter being tested.</param>
/// <exception cref="ArgumentException">Thrown if <paramref name="value"/> is not a bitwise match for <paramref name="target"/>.</exception>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static unsafe void IsBitwiseEqualTo<T>(T value, T target, string name)
where T : unmanaged
{
// Include some fast paths if the input type is of size 1, 2, 4, 8, or 16.
// In those cases, just reinterpret the bytes as values of an integer type,
// and compare them directly, which is much faster than having a loop over each byte.
// The conditional branches below are known at compile time by the JIT compiler,
// so that only the right one will actually be translated into native code.
if (sizeof(T) == 1)
{
byte valueByte = Unsafe.As<T, byte>(ref value);
byte targetByte = Unsafe.As<T, byte>(ref target);
if (valueByte == targetByte)
{
return;
}
ThrowHelper.ThrowArgumentExceptionForsBitwiseEqualTo(value, target, name);
}
else if (sizeof(T) == 2)
{
ushort valueUShort = Unsafe.As<T, ushort>(ref value);
ushort targetUShort = Unsafe.As<T, ushort>(ref target);
if (valueUShort == targetUShort)
{
return;
}
ThrowHelper.ThrowArgumentExceptionForsBitwiseEqualTo(value, target, name);
}
else if (sizeof(T) == 4)
{
uint valueUInt = Unsafe.As<T, uint>(ref value);
uint targetUInt = Unsafe.As<T, uint>(ref target);
if (valueUInt == targetUInt)
{
return;
}
ThrowHelper.ThrowArgumentExceptionForsBitwiseEqualTo(value, target, name);
}
else if (sizeof(T) == 8)
{
ulong valueULong = Unsafe.As<T, ulong>(ref value);
ulong targetULong = Unsafe.As<T, ulong>(ref target);
if (Bit64Compare(ref valueULong, ref targetULong))
{
return;
}
ThrowHelper.ThrowArgumentExceptionForsBitwiseEqualTo(value, target, name);
}
else if (sizeof(T) == 16)
{
ulong valueULong0 = Unsafe.As<T, ulong>(ref value);
ulong targetULong0 = Unsafe.As<T, ulong>(ref target);
if (Bit64Compare(ref valueULong0, ref targetULong0))
{
ulong valueULong1 = Unsafe.Add(ref Unsafe.As<T, ulong>(ref value), 1);
ulong targetULong1 = Unsafe.Add(ref Unsafe.As<T, ulong>(ref target), 1);
if (Bit64Compare(ref valueULong1, ref targetULong1))
{
return;
}
}
ThrowHelper.ThrowArgumentExceptionForsBitwiseEqualTo(value, target, name);
}
else
{
Span<byte> valueBytes = new Span<byte>(Unsafe.AsPointer(ref value), sizeof(T));
Span<byte> targetBytes = new Span<byte>(Unsafe.AsPointer(ref target), sizeof(T));
if (valueBytes.SequenceEqual(targetBytes))
{
return;
}
ThrowHelper.ThrowArgumentExceptionForsBitwiseEqualTo(value, target, name);
}
}
// Compares 64 bits of data from two given memory locations for bitwise equality
[Pure]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static unsafe bool Bit64Compare(ref ulong left, ref ulong right)
{
// Handles 32 bit case, because using ulong is inefficient
if (sizeof(IntPtr) == 4)
{
ref int r0 = ref Unsafe.As<ulong, int>(ref left);
ref int r1 = ref Unsafe.As<ulong, int>(ref right);
return r0 == r1 &&
Unsafe.Add(ref r0, 1) == Unsafe.Add(ref r1, 1);
}
return left == right;
} |
|
It worked! 🎉🎉🎉 FYI @MichalStrehovsky, @tommcdon looks like .NET Native (x64) fails to build when using unsafe code and abundant pointers in this specific case. Commit 5754bcd fixed the issue, so you should be able to see from there the before/after to possibly find out what was causing this. The specific error was:
Hope this helps! 😄 |
Optimisation (identical behavior)
What is the current behavior?
Slow path is taken for any type which isn't 1, 2, 34, or 8 byte size
What is the new behavior?
Fast path is also taken for 16 byte types, and other sized types use
Span<T>.SequenceEqual, which is well optimised, rather than the naive for-loop approachExtensively codegen checked. All is inlined as expected. The only possible improvement is making the parameters passed by readonly reference (
in), but it also has downsides in common scenariosPR Checklist