-
Notifications
You must be signed in to change notification settings - Fork 39
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
Address perf+correctness of StrLen and StrChr #480
base: main
Are you sure you want to change the base?
Changes from all commits
9df5497
1dcd96d
7149d92
2ab0c15
2f2352e
23e2b61
20dd125
cf488a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
using System.Text; | ||
|
||
namespace Cesium.Runtime.Tests; | ||
|
||
public unsafe class StringFunctionTests | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add some Unicode tests, shall we? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, will do |
||
{ | ||
[Theory] | ||
[InlineData("Hello\0", 5)] | ||
[InlineData("Goodbye\0", 7)] | ||
[InlineData("Hello\0Goodbye\0", 5)] | ||
[InlineData(" \0", 18)] | ||
public void StrLen(string input, int expected) | ||
{ | ||
// TODO: If you are rich enough to procure a 2-4+ GB RAM runner, | ||
// please update this test to exercise the path where the string | ||
// length exceeds int.MaxLength of bytes. | ||
var bytes = Encoding.UTF8.GetBytes(input); | ||
fixed (byte* str = bytes) | ||
{ | ||
var actual = StringFunctions.StrLen(str); | ||
|
||
Assert.Equal((nuint)expected, actual); | ||
} | ||
} | ||
|
||
[Fact] | ||
public void StrLen_Null() | ||
{ | ||
var actual = StringFunctions.StrLen(null); | ||
|
||
Assert.Equal((nuint)0, actual); | ||
} | ||
|
||
[Theory] | ||
[InlineData("Hello\n", 5)] | ||
[InlineData("Goodbye\n", 7)] | ||
[InlineData("Hello\nGoodbye\n", 5)] | ||
[InlineData(" \n", 18)] | ||
public void StrChr(string input, int expectedOffset) | ||
{ | ||
var needle = '\n'; | ||
var bytes = Encoding.UTF8.GetBytes(input); | ||
fixed (byte* str = bytes) | ||
{ | ||
var ptr = StringFunctions.StrChr(str, '\n'); | ||
|
||
Assert.Equal((byte)needle, *ptr); | ||
Assert.Equal(expectedOffset, (int)(ptr - str)); | ||
} | ||
} | ||
|
||
[Theory] | ||
[InlineData("Hello\0")] | ||
[InlineData("Goodbye\0")] | ||
[InlineData("Hello Goodbye\0")] | ||
[InlineData(" \0")] | ||
public void StrChr_NotFound(string input) | ||
{ | ||
var bytes = Encoding.UTF8.GetBytes(input); | ||
fixed (byte* str = bytes) | ||
{ | ||
var actual = StringFunctions.StrChr(str, '\n'); | ||
|
||
Assert.True(actual is null); | ||
} | ||
} | ||
|
||
[Fact] | ||
public void StrChr_Null() | ||
{ | ||
var actual = StringFunctions.StrChr(null, '\0'); | ||
|
||
Assert.True(actual is null); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
<Project Sdk="Microsoft.NET.Sdk"> | ||
|
||
<PropertyGroup> | ||
<TargetFrameworks>netstandard2.0;net6.0</TargetFrameworks> | ||
<TargetFramework>net7.0</TargetFramework> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would still insist on having netstandard2.0 support via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, this is runtime. Is it planned to be referenced as a package by .NET Framework/pre-.NET 7 targets or is it packaged as a standalone, well, runtime? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it is planned to be referenced by projects targeting older .NET versions. |
||
<ImplicitUsings>enable</ImplicitUsings> | ||
<Nullable>enable</Nullable> | ||
<AllowUnsafeBlocks>true</AllowUnsafeBlocks> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,7 @@ | ||
#if NETSTANDARD | ||
using System.Text; | ||
#else | ||
using System.Collections.Specialized; | ||
using System.Runtime.InteropServices; | ||
#endif | ||
using System.Numerics; | ||
using System.Runtime.CompilerServices; | ||
using System.Runtime.Intrinsics; | ||
using System.Runtime.Intrinsics.Arm; | ||
|
||
namespace Cesium.Runtime; | ||
|
||
|
@@ -14,50 +12,57 @@ public static unsafe class StringFunctions | |
{ | ||
public static nuint StrLen(byte* str) | ||
{ | ||
#if NETSTANDARD | ||
if (str == null) | ||
if (str != null) | ||
{ | ||
return 0; | ||
} | ||
var start = str; | ||
while ((nuint)str % 16 != 0) | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest introducing a constant for this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, it's just |
||
if (*str is 0) | ||
{ | ||
goto Done; | ||
} | ||
str++; | ||
} | ||
|
||
Encoding encoding = Encoding.UTF8; | ||
int byteLength = 0; | ||
byte* search = str; | ||
while (*search != '\0') | ||
{ | ||
byteLength++; | ||
search++; | ||
while (true) | ||
{ | ||
var eqmask = Vector128.Equals( | ||
Vector128.LoadAligned(str), | ||
Vector128<byte>.Zero); | ||
if (eqmask == Vector128<byte>.Zero) | ||
{ | ||
str += Vector128<byte>.Count; | ||
continue; | ||
} | ||
|
||
str += IndexOfMatch(eqmask); | ||
break; | ||
} | ||
Done: | ||
return (nuint)str - (nuint)start; | ||
} | ||
|
||
int stringLength = encoding.GetCharCount(str, byteLength); | ||
return (uint)stringLength; | ||
ForNeVeR marked this conversation as resolved.
Show resolved
Hide resolved
|
||
#else | ||
return (uint)(Marshal.PtrToStringUTF8((nint)str)?.Length ?? 0); | ||
#endif | ||
ForNeVeR marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return 0; | ||
} | ||
public static byte* StrCpy(byte* dest, byte* src) | ||
{ | ||
if (dest == null) | ||
if (dest != null) | ||
{ | ||
return null; | ||
} | ||
var result = dest; | ||
if (src != null) | ||
{ | ||
// SIMD scan into SIMD copy (traversing the data twice) | ||
// is much faster than a single scalar check+copy loop. | ||
var length = StrLen(src); | ||
Buffer.MemoryCopy(src, dest, length, length); | ||
dest += length; | ||
} | ||
|
||
var result = dest; | ||
if (src == null) | ||
{ | ||
*dest = 0; | ||
return dest; | ||
} | ||
|
||
byte* search = src; | ||
while (*search != '\0') | ||
{ | ||
*dest = *search; | ||
search++; | ||
dest++; | ||
} | ||
|
||
*dest = 0; | ||
return result; | ||
return null; | ||
} | ||
public static byte* StrNCpy(byte* dest, byte* src, nuint count) | ||
{ | ||
|
@@ -186,21 +191,50 @@ public static int StrNCmp(byte* lhs, byte* rhs, nuint count) | |
} | ||
public static byte* StrChr(byte* str, int ch) | ||
{ | ||
if (str == null) | ||
if (str != null) | ||
{ | ||
return null; | ||
} | ||
byte c = (byte)ch; | ||
|
||
while (*str != 0) | ||
{ | ||
if (*str == ch) | ||
while ((nuint)str % 16 != 0) | ||
{ | ||
return str; | ||
var curr = *str; | ||
if (curr == c) | ||
{ | ||
goto Done; | ||
} | ||
else if (curr == 0) | ||
{ | ||
goto NotFound; | ||
} | ||
str++; | ||
} | ||
|
||
str++; | ||
var element = Vector128.Create(c); | ||
var nullByte = Vector128<byte>.Zero; | ||
while (true) | ||
{ | ||
var chars = Vector128.LoadAligned(str); | ||
var eqmask = Vector128.Equals(chars, element) | | ||
Vector128.Equals(chars, nullByte); | ||
if (eqmask == Vector128<byte>.Zero) | ||
{ | ||
str += Vector128<byte>.Count; | ||
continue; | ||
} | ||
|
||
str += IndexOfMatch(eqmask); | ||
if (*str == 0) | ||
{ | ||
goto NotFound; | ||
} | ||
break; | ||
} | ||
|
||
Done: | ||
return str; | ||
} | ||
|
||
NotFound: | ||
return null; | ||
} | ||
|
||
|
@@ -215,7 +249,6 @@ public static int StrCmp(byte* lhs, byte* rhs) | |
if (*lhs > *rhs) return 1; | ||
} | ||
|
||
|
||
if (*lhs < *rhs) return -1; | ||
if (*lhs > *rhs) return 1; | ||
return 0; | ||
|
@@ -253,4 +286,20 @@ public static int MemCmp(void* lhs, void* rhs, nuint count) | |
|
||
return 0; | ||
} | ||
|
||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
static uint IndexOfMatch(Vector128<byte> eqmask) | ||
{ | ||
if (AdvSimd.Arm64.IsSupported) | ||
{ | ||
var res = AdvSimd | ||
.ShiftRightLogicalNarrowingLower(eqmask.AsUInt16(), 4) | ||
.AsUInt64() | ||
.ToScalar(); | ||
return (uint)BitOperations.TrailingZeroCount(res) >> 2; | ||
} | ||
|
||
return (uint)BitOperations.TrailingZeroCount( | ||
eqmask.ExtractMostSignificantBits()); | ||
} | ||
} |
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.
After some thought, I still want us to support .NET Framework on Windows. Let's re-enable that here, and add a fallback implementation for .NET Standard.