Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Vectorize some Guid APIs (ctor, TryWriteBytes) #21336

Merged
merged 9 commits into from
Dec 3, 2018
Merged

Vectorize some Guid APIs (ctor, TryWriteBytes) #21336

Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a4aec27
Optimize some Guid APIs
EgorBo Dec 3, 2018
be3ac7f
compare as two ulong
EgorBo Dec 3, 2018
0feddf2
Optimize WriteByteHelper with SSE2
EgorBo Dec 3, 2018
90a7eb3
replace SSE2 with MemoryMarshal.Write
EgorBo Dec 3, 2018
e4d6a6a
get rid of WriteByteHelper
EgorBo Dec 3, 2018
fbd651d
(uint) hack didn't work
EgorBo Dec 3, 2018
e3a44a9
use TryWrite instead of Write
EgorBo Dec 3, 2018
0080ec8
Optimize ctor `Guid(ReadOnlySpan<byte> b)` and remove `Equals` optimi…
EgorBo Dec 3, 2018
448130d
fix compilation error and add comments
EgorBo Dec 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 42 additions & 29 deletions src/System.Private.CoreLib/shared/System/Guid.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ public Guid(ReadOnlySpan<byte> b)
if ((uint)b.Length != 16)
throw new ArgumentException(SR.Format(SR.Arg_GuidArrayCtor, "16"), nameof(b));

_k = b[15];
_a = b[3] << 24 | b[2] << 16 | b[1] << 8 | b[0];
_b = (short)(b[5] << 8 | b[4]);
_c = (short)(b[7] << 8 | b[6]);
Expand All @@ -61,7 +62,6 @@ public Guid(ReadOnlySpan<byte> b)
_h = b[12];
_i = b[13];
_j = b[14];
_k = b[15];
}

[CLSCompliant(false)]
Expand Down Expand Up @@ -93,14 +93,14 @@ public Guid(int a, short b, short c, byte[] d)
_a = a;
_b = b;
_c = c;
_k = d[7];
_d = d[0];
_e = d[1];
_f = d[2];
_g = d[3];
_h = d[4];
_i = d[5];
_j = d[6];
_k = d[7];
}

// Creates a new GUID initialized to the value represented by the
Expand Down Expand Up @@ -768,43 +768,51 @@ private static bool IsHexPrefix(ReadOnlySpan<char> str, int i) =>
str[i] == '0' &&
(str[i + 1] | 0x20) == 'x';

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void WriteByteHelper(Span<byte> destination)
{
destination[0] = (byte)(_a);
destination[1] = (byte)(_a >> 8);
destination[2] = (byte)(_a >> 16);
destination[3] = (byte)(_a >> 24);
destination[4] = (byte)(_b);
destination[5] = (byte)(_b >> 8);
destination[6] = (byte)(_c);
destination[7] = (byte)(_c >> 8);
destination[8] = _d;
destination[9] = _e;
destination[10] = _f;
destination[11] = _g;
destination[12] = _h;
destination[13] = _i;
destination[14] = _j;
destination[15] = _k;
}

// Returns an unsigned byte array containing the GUID.
public byte[] ToByteArray()
{
var g = new byte[16];
WriteByteHelper(g);
if (BitConverter.IsLittleEndian)
{
MemoryMarshal.TryWrite<Guid>(g, ref this);
}
else
{
TryWriteBytes(g);
}
return g;
}

// Returns whether bytes are sucessfully written to given span.
public bool TryWriteBytes(Span<byte> destination)
{
if (destination.Length < 16)
return false;

WriteByteHelper(destination);
return true;
if (BitConverter.IsLittleEndian)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constructor that takes Span can be optimized the same way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but will it compile? C# requires stucts' constructors to init all fields by hands (however Decimal compiles fine hm...).

Copy link
Member Author

@EgorBo EgorBo Dec 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok it works

this = MemoryMarshal.Read<MyGuid>(b);

return MemoryMarshal.TryWrite(destination, ref this);
}

// slower path for BigEndian
if ((uint)destination.Length >= 16)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Does writing it this way help anything? It is generally more readable to use early returns, like:

 if (destination.Length < 16)
    return false;

destination[0] = ...;
....

{
destination[0] = (byte)(_a);
destination[1] = (byte)(_a >> 8);
destination[2] = (byte)(_a >> 16);
destination[3] = (byte)(_a >> 24);
destination[4] = (byte)(_b);
destination[5] = (byte)(_b >> 8);
destination[6] = (byte)(_c);
destination[7] = (byte)(_c >> 8);
destination[8] = _d;
destination[9] = _e;
destination[10] = _f;
destination[11] = _g;
destination[12] = _h;
destination[13] = _i;
destination[14] = _j;
destination[15] = _k;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not to write this one first like you are doing in other places?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought since it's surrounded with if ((uint)destination.Length >= 16) JIT would not insert bounds checks but turns out it inserts them anyway (I think there is a feature-request for it somewhere...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this optimization is very sensitive to exact pattern today.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it still write bounds checks if you specify 16u?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grant-d unfortunately yes it ignores (uint) > 16 (and 16u)

return true;
}
return false;
}

// Returns the guid in "registry" format.
Expand Down Expand Up @@ -839,10 +847,15 @@ public override bool Equals(object o)
public bool Equals(Guid g)
{
// Now compare each of the elements
#if BIT64
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not to do this for the first check as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkotas in case if two guids are different (both random) - it becomes slower (see Equals_2 in the table - it does two ulong-checks).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • changed to be two ulong *

return Unsafe.As<int, ulong>(ref g._a) == Unsafe.As<int, ulong>(ref _a) &&
Unsafe.Add(ref Unsafe.As<int, ulong>(ref g._a), 1) == Unsafe.Add(ref Unsafe.As<int, ulong>(ref _a), 1);
#else
return g._a == _a &&
Unsafe.Add(ref g._a, 1) == Unsafe.Add(ref _a, 1) &&
Unsafe.Add(ref g._a, 2) == Unsafe.Add(ref _a, 2) &&
Unsafe.Add(ref g._a, 3) == Unsafe.Add(ref _a, 3);
#endif
}

private int GetResult(uint me, uint them)
Expand Down