From 9df54975f9163901fe22d9bf26cd68ea3950d169 Mon Sep 17 00:00:00 2001 From: neon-sunset Date: Fri, 17 Nov 2023 13:03:46 +0200 Subject: [PATCH 1/7] Address perf+correctness of StrLen and StrChr --- Cesium.Runtime.Tests/StringFunctionTests.cs | 43 +++++++++++++++++++++ Cesium.Runtime/Cesium.Runtime.csproj | 4 ++ Cesium.Runtime/StringFunctions.cs | 42 +++++++------------- 3 files changed, 61 insertions(+), 28 deletions(-) create mode 100644 Cesium.Runtime.Tests/StringFunctionTests.cs diff --git a/Cesium.Runtime.Tests/StringFunctionTests.cs b/Cesium.Runtime.Tests/StringFunctionTests.cs new file mode 100644 index 00000000..d2419ed6 --- /dev/null +++ b/Cesium.Runtime.Tests/StringFunctionTests.cs @@ -0,0 +1,43 @@ +using System.Text; + +namespace Cesium.Runtime.Tests; + +public unsafe class StringFunctionTests +{ + [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); + } + } + + [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)); + } + } +} diff --git a/Cesium.Runtime/Cesium.Runtime.csproj b/Cesium.Runtime/Cesium.Runtime.csproj index d8fe30c4..87457314 100644 --- a/Cesium.Runtime/Cesium.Runtime.csproj +++ b/Cesium.Runtime/Cesium.Runtime.csproj @@ -12,4 +12,8 @@ + + + + diff --git a/Cesium.Runtime/StringFunctions.cs b/Cesium.Runtime/StringFunctions.cs index 93dc43a6..87c8437d 100644 --- a/Cesium.Runtime/StringFunctions.cs +++ b/Cesium.Runtime/StringFunctions.cs @@ -1,7 +1,9 @@ #if NETSTANDARD +using System; using System.Text; #else using System.Collections.Specialized; +using System.Runtime.CompilerServices; using System.Runtime.InteropServices; #endif @@ -14,26 +16,8 @@ public unsafe static class StringFunctions { public static nuint StrLen(byte* str) { -#if NETSTANDARD - if (str == null) - { - return 0; - } - - Encoding encoding = Encoding.UTF8; - int byteLength = 0; - byte* search = str; - while (*search != '\0') - { - byteLength++; - search++; - } - - int stringLength = encoding.GetCharCount(str, byteLength); - return (uint)stringLength; -#else - return (uint)(Marshal.PtrToStringUTF8((nint)str)?.Length ?? 0); -#endif + var offset = StrChr(str, 0); + return (nuint)(offset - str); } public static byte* StrCpy(byte* dest, byte* src) { @@ -186,19 +170,21 @@ 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; - } + int match; + nuint offset = 0; + byte c = (byte)ch; - while (*str != 0) - { - if (*str == ch) + // TODO: Copy IndexOfNullByte impl. from CoreLib in a distant future + var span = new Span(str, int.MaxValue); + while ((match = span.IndexOf(c)) < 0) { - return str; + offset += int.MaxValue; + span = new Span(str + offset, int.MaxValue); } - str++; + return str + ((uint)match + offset); } return null; From 1dcd96d82adb83e1d49d97a135c0744cd8c4310e Mon Sep 17 00:00:00 2001 From: neon-sunset Date: Fri, 17 Nov 2023 13:21:32 +0200 Subject: [PATCH 2/7] Add missed null coverage --- Cesium.Runtime.Tests/StringFunctionTests.cs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/Cesium.Runtime.Tests/StringFunctionTests.cs b/Cesium.Runtime.Tests/StringFunctionTests.cs index d2419ed6..4e7b979c 100644 --- a/Cesium.Runtime.Tests/StringFunctionTests.cs +++ b/Cesium.Runtime.Tests/StringFunctionTests.cs @@ -23,6 +23,14 @@ public void StrLen(string input, int expected) } } + [Fact] + public void StrLen_Null() + { + var actual = StringFunctions.StrLen(null); + + Assert.Equal((nuint)0, actual); + } + [Theory] [InlineData("Hello\n", 5)] [InlineData("Goodbye\n", 7)] @@ -40,4 +48,12 @@ public void StrChr(string input, int expectedOffset) Assert.Equal(expectedOffset, (int)(ptr - str)); } } + + [Fact] + public void StrChr_Null() + { + var actual = StringFunctions.StrChr(null, '\0'); + + Assert.True(actual is null); + } } From 7149d92fc4bba56283cb40147c89a328926f6e3e Mon Sep 17 00:00:00 2001 From: neon-sunset Date: Sat, 18 Nov 2023 08:35:29 +0200 Subject: [PATCH 3/7] Fix incorrect impl., check if CI passes, temporarily drop NS2.0 --- .../Cesium.Runtime.Tests.csproj | 2 +- Cesium.Runtime.Tests/StringFunctionTests.cs | 12 ++++++ Cesium.Runtime/Cesium.Runtime.csproj | 6 +-- Cesium.Runtime/StringFunctions.cs | 42 +++++++++++-------- 4 files changed, 38 insertions(+), 24 deletions(-) diff --git a/Cesium.Runtime.Tests/Cesium.Runtime.Tests.csproj b/Cesium.Runtime.Tests/Cesium.Runtime.Tests.csproj index ec7c60b9..df2f8dda 100644 --- a/Cesium.Runtime.Tests/Cesium.Runtime.Tests.csproj +++ b/Cesium.Runtime.Tests/Cesium.Runtime.Tests.csproj @@ -2,7 +2,7 @@ net7.0 - $(TargetFrameworks);net48 + enable false true diff --git a/Cesium.Runtime.Tests/StringFunctionTests.cs b/Cesium.Runtime.Tests/StringFunctionTests.cs index 4e7b979c..b32a1259 100644 --- a/Cesium.Runtime.Tests/StringFunctionTests.cs +++ b/Cesium.Runtime.Tests/StringFunctionTests.cs @@ -49,6 +49,18 @@ public void StrChr(string input, int expectedOffset) } } + [Fact] + public void StrChr_NotFound() + { + var bytes = Encoding.UTF8.GetBytes("Hello\0"); + fixed (byte* str = bytes) + { + var actual = StringFunctions.StrChr(str, '\n'); + + Assert.True(actual is null); + } + } + [Fact] public void StrChr_Null() { diff --git a/Cesium.Runtime/Cesium.Runtime.csproj b/Cesium.Runtime/Cesium.Runtime.csproj index 87457314..1b9ddc98 100644 --- a/Cesium.Runtime/Cesium.Runtime.csproj +++ b/Cesium.Runtime/Cesium.Runtime.csproj @@ -1,7 +1,7 @@ - netstandard2.0;net6.0 + net6.0 enable enable true @@ -12,8 +12,4 @@ - - - - diff --git a/Cesium.Runtime/StringFunctions.cs b/Cesium.Runtime/StringFunctions.cs index 87c8437d..bc7e2b24 100644 --- a/Cesium.Runtime/StringFunctions.cs +++ b/Cesium.Runtime/StringFunctions.cs @@ -1,12 +1,3 @@ -#if NETSTANDARD -using System; -using System.Text; -#else -using System.Collections.Specialized; -using System.Runtime.CompilerServices; -using System.Runtime.InteropServices; -#endif - namespace Cesium.Runtime; /// @@ -16,8 +7,22 @@ public unsafe static class StringFunctions { public static nuint StrLen(byte* str) { - var offset = StrChr(str, 0); - return (nuint)(offset - str); + if (str != null) + { + int match; + nuint offset = 0; + + // TODO: Copy IndexOfNullByte impl. from CoreLib in a distant future + while ((match = new Span(str + offset, int.MaxValue) + .IndexOf((byte)0)) < 0) + { + offset += int.MaxValue; + } + + return offset + (uint)match; + } + + return 0; } public static byte* StrCpy(byte* dest, byte* src) { @@ -173,18 +178,19 @@ public static int StrNCmp(byte* lhs, byte* rhs, nuint count) if (str != null) { int match; - nuint offset = 0; byte c = (byte)ch; - // TODO: Copy IndexOfNullByte impl. from CoreLib in a distant future - var span = new Span(str, int.MaxValue); - while ((match = span.IndexOf(c)) < 0) + while ((match = new Span(str, int.MaxValue) + .IndexOfAny(c, 0)) < 0) { - offset += int.MaxValue; - span = new Span(str + offset, int.MaxValue); + str += int.MaxValue; } + str += (nint)(uint)match; - return str + ((uint)match + offset); + if (*str == c) + { + return str; + } } return null; From 2ab0c15f87e79d38b23a05914b43277620e87271 Mon Sep 17 00:00:00 2001 From: neon-sunset Date: Sat, 30 Dec 2023 14:34:03 +0200 Subject: [PATCH 4/7] Address SEGFAULT by implementing search manually and aligning the loads at 16B boundary, and bump up to .NET 7 which is required for crossplat SIMD --- Cesium.Runtime/Cesium.Runtime.csproj | 2 +- Cesium.Runtime/StringFunctions.cs | 111 +++++++++++++++++++-------- 2 files changed, 81 insertions(+), 32 deletions(-) diff --git a/Cesium.Runtime/Cesium.Runtime.csproj b/Cesium.Runtime/Cesium.Runtime.csproj index 1b9ddc98..53844648 100644 --- a/Cesium.Runtime/Cesium.Runtime.csproj +++ b/Cesium.Runtime/Cesium.Runtime.csproj @@ -1,7 +1,7 @@ - net6.0 + net7.0 enable enable true diff --git a/Cesium.Runtime/StringFunctions.cs b/Cesium.Runtime/StringFunctions.cs index bc7e2b24..3eb0b4a1 100644 --- a/Cesium.Runtime/StringFunctions.cs +++ b/Cesium.Runtime/StringFunctions.cs @@ -1,3 +1,8 @@ +using System.Numerics; +using System.Runtime.CompilerServices; +using System.Runtime.Intrinsics; +using System.Runtime.Intrinsics.Arm; + namespace Cesium.Runtime; /// @@ -9,44 +14,55 @@ public static nuint StrLen(byte* str) { if (str != null) { - int match; - nuint offset = 0; - - // TODO: Copy IndexOfNullByte impl. from CoreLib in a distant future - while ((match = new Span(str + offset, int.MaxValue) - .IndexOf((byte)0)) < 0) + var start = str; + while ((nuint)str % 16 != 0) { - offset += int.MaxValue; + if (*str is 0) + { + goto Done; + } + str++; } - return offset + (uint)match; + while (true) + { + var eqmask = Vector128.Equals( + Vector128.LoadAligned(str), + Vector128.Zero); + if (eqmask == Vector128.Zero) + { + str += Vector128.Count; + continue; + } + + str += IndexOfMatch(eqmask); + break; + } + Done: + return (nuint)str - (nuint)start; } 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) { @@ -177,22 +193,55 @@ public static int StrNCmp(byte* lhs, byte* rhs, nuint count) { if (str != null) { - int match; byte c = (byte)ch; - while ((match = new Span(str, int.MaxValue) - .IndexOfAny(c, 0)) < 0) + while ((nuint)str % 16 != 0) { - str += int.MaxValue; + var curr = *str; + if (curr == c || curr == 0) + { + goto Done; + } + str++; } - str += (nint)(uint)match; - if (*str == c) + var element = Vector128.Create(c); + var nullByte = Vector128.Zero; + while (true) { - return str; + var chars = Vector128.LoadAligned(str); + var eqmask = Vector128.Equals(chars, element) | + Vector128.Equals(chars, nullByte); + if (eqmask == Vector128.Zero) + { + str += Vector128.Count; + continue; + } + + str += IndexOfMatch(eqmask); + break; } + + Done: + return str; } return null; } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + static uint IndexOfMatch(Vector128 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()); + } } From 23e2b6171395cfadc4ca605b74c6b48a82349f49 Mon Sep 17 00:00:00 2001 From: neon-sunset Date: Sat, 30 Dec 2023 14:41:50 +0200 Subject: [PATCH 5/7] Fix implementation --- Cesium.Runtime/StringFunctions.cs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/Cesium.Runtime/StringFunctions.cs b/Cesium.Runtime/StringFunctions.cs index 77d9afee..7510b882 100644 --- a/Cesium.Runtime/StringFunctions.cs +++ b/Cesium.Runtime/StringFunctions.cs @@ -198,10 +198,14 @@ public static int StrNCmp(byte* lhs, byte* rhs, nuint count) while ((nuint)str % 16 != 0) { var curr = *str; - if (curr == c || curr == 0) + if (curr == c) { goto Done; } + else if (curr == 0) + { + goto NotFound; + } str++; } @@ -219,6 +223,10 @@ public static int StrNCmp(byte* lhs, byte* rhs, nuint count) } str += IndexOfMatch(eqmask); + if (*str == 0) + { + goto NotFound; + } break; } @@ -226,6 +234,7 @@ public static int StrNCmp(byte* lhs, byte* rhs, nuint count) return str; } + NotFound: return null; } From 20dd12563e28268f546aceeb837d324890578db4 Mon Sep 17 00:00:00 2001 From: neon-sunset Date: Sat, 30 Dec 2023 14:47:17 +0200 Subject: [PATCH 6/7] Extend test cases --- Cesium.Runtime.Tests/StringFunctionTests.cs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/Cesium.Runtime.Tests/StringFunctionTests.cs b/Cesium.Runtime.Tests/StringFunctionTests.cs index b32a1259..c3d0714a 100644 --- a/Cesium.Runtime.Tests/StringFunctionTests.cs +++ b/Cesium.Runtime.Tests/StringFunctionTests.cs @@ -49,10 +49,14 @@ public void StrChr(string input, int expectedOffset) } } - [Fact] - public void StrChr_NotFound() + [Theory] + [InlineData("Hello\0")] + [InlineData("Goodbye\0")] + [InlineData("Hello Goodbye\0")] + [InlineData(" \0")] + public void StrChr_NotFound(string input) { - var bytes = Encoding.UTF8.GetBytes("Hello\0"); + var bytes = Encoding.UTF8.GetBytes(input); fixed (byte* str = bytes) { var actual = StringFunctions.StrChr(str, '\n'); From cf488a7bb3daf408c666ac2115f50e5512dc2fc1 Mon Sep 17 00:00:00 2001 From: neon-sunset Date: Sat, 30 Dec 2023 17:08:23 +0200 Subject: [PATCH 7/7] Fix CI --- .vscode/launch.json | 4 ++-- Cesium.Compiler/Compilation.cs | 2 +- Cesium.Compiler/RuntimeConfig.cs | 13 ------------- Cesium.Test.Framework/CSharpCompilationUtil.cs | 2 +- 4 files changed, 4 insertions(+), 17 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index 1bbf4d2a..339d2cdc 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -10,7 +10,7 @@ "request": "launch", "preLaunchTask": "build", // If you have changed target frameworks, make sure to update the program path. - "program": "${workspaceFolder}/Cesium.Compiler/bin/Debug/net6.0/Cesium.Compiler.dll", + "program": "${workspaceFolder}/Cesium.Compiler/bin/Debug/net7.0/Cesium.Compiler.dll", "args": [], "cwd": "${workspaceFolder}/Cesium.Compiler", // For more information about the 'console' field, see https://aka.ms/VSCode-CS-LaunchJson-Console @@ -23,4 +23,4 @@ "request": "attach" } ] -} \ No newline at end of file +} diff --git a/Cesium.Compiler/Compilation.cs b/Cesium.Compiler/Compilation.cs index 865db0a5..8c6d8696 100644 --- a/Cesium.Compiler/Compilation.cs +++ b/Cesium.Compiler/Compilation.cs @@ -141,7 +141,7 @@ private static void SaveAssembly( { var runtimeConfigFilePath = Path.ChangeExtension(outputFilePath, "runtimeconfig.json"); Console.WriteLine($"Generating a .NET 6 runtime config at {runtimeConfigFilePath}."); - File.WriteAllText(runtimeConfigFilePath, RuntimeConfig.EmitNet6()); + File.WriteAllText(runtimeConfigFilePath, RuntimeConfig.EmitNet7()); } } } diff --git a/Cesium.Compiler/RuntimeConfig.cs b/Cesium.Compiler/RuntimeConfig.cs index 2b75217d..f7c935ab 100644 --- a/Cesium.Compiler/RuntimeConfig.cs +++ b/Cesium.Compiler/RuntimeConfig.cs @@ -2,19 +2,6 @@ namespace Cesium.Compiler; internal static class RuntimeConfig { - public static string EmitNet6() => """ - { - "runtimeOptions": { - "tfm": "net6.0", - "rollForward": "Major", - "framework": { - "name": "Microsoft.NETCore.App", - "version": "6.0.0" - } - } - } - """.ReplaceLineEndings("\n"); - public static string EmitNet7() => """ { "runtimeOptions": { diff --git a/Cesium.Test.Framework/CSharpCompilationUtil.cs b/Cesium.Test.Framework/CSharpCompilationUtil.cs index 2c520bb5..60ec07e1 100644 --- a/Cesium.Test.Framework/CSharpCompilationUtil.cs +++ b/Cesium.Test.Framework/CSharpCompilationUtil.cs @@ -11,7 +11,7 @@ public static class CSharpCompilationUtil public static readonly TargetRuntimeDescriptor DefaultRuntime = TargetRuntimeDescriptor.Net60; private const string _configuration = "Debug"; private const string _targetRuntime = "net7.0"; - private const string _cesiumRuntimeLibTargetRuntime = "net6.0"; + private const string _cesiumRuntimeLibTargetRuntime = "net7.0"; private const string _projectName = "TestProject"; /// Semaphore that controls the amount of simultaneously running tests.