-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Code clean up around TryWriteBigEndian/TryWriteLittleEndian #110897
Conversation
Tagging subscribers to this area: @dotnet/area-system-numerics |
I wonder whether we could refactor so that the primitive types have internal methods for the implementation, e.g.: namespace System.Buffers.Binary
{
public static partial class BinaryPrimitives
{
public static void WriteDoubleBigEndian(Span<byte> destination, double value) => value.WriteBigEndian(destination);
}
}
namespace System
{
public readonly struct Double
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal void WriteBigEndian(Span<byte> destination)
{
if (BitConverter.IsLittleEndian)
{
long tmp = BinaryPrimitives.ReverseEndianness(BitConverter.DoubleToInt64Bits(m_value));
MemoryMarshal.Write(destination, in tmp);
}
else
{
MemoryMarshal.Write(destination, in m_value);
}
}
}
} |
Not sure this improves anything to be honest. The main motivation behind this change is to remove unsafe code (in this case, duplicated unsafe code). |
Unsafe.WriteUnaligned( | ||
ref MemoryMarshal.GetReference(buffer), | ||
BitOperations.RotateLeft(s1 * 5, 7) * 9); | ||
MemoryMarshal.Write(buffer, BitOperations.RotateLeft(s1 * 5, 7) * 9); |
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.
Does this one get optimized correctly today?
The perf of Random is fairly sensitive and I believe at the time it wasn't well handled.
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.
The codegen seesm to be the same - I've checked. We might want to make various helpers like this as "zero budget" for inliner if we hit any issues because of that
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.
sounds good; just wanted to confirm that it was getting handled right since its a slightly different change than all the others in the PR
Contributes to #94941