Skip to content
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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -23,4 +23,4 @@
"request": "attach"
}
]
}
}
2 changes: 1 addition & 1 deletion Cesium.Compiler/Compilation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
}
13 changes: 0 additions & 13 deletions Cesium.Compiler/RuntimeConfig.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
2 changes: 1 addition & 1 deletion Cesium.Runtime.Tests/Cesium.Runtime.Tests.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

<PropertyGroup>
<TargetFrameworks>net7.0</TargetFrameworks>
<TargetFrameworks Condition=" $([MSBuild]::IsOsPlatform('Windows')) ">$(TargetFrameworks);net48</TargetFrameworks>
<!-- <TargetFrameworks Condition=" $([MSBuild]::IsOsPlatform('Windows')) ">$(TargetFrameworks);net48</TargetFrameworks> -->
Copy link
Owner

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.

<ImplicitUsings>enable</ImplicitUsings>
<IsPackable>false</IsPackable>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
Expand Down
75 changes: 75 additions & 0 deletions Cesium.Runtime.Tests/StringFunctionTests.cs
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
Copy link
Owner

Choose a reason for hiding this comment

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

Let's add some Unicode tests, shall we?

Copy link
Author

Choose a reason for hiding this comment

The 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);
}
}
2 changes: 1 addition & 1 deletion Cesium.Runtime/Cesium.Runtime.csproj
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>
Copy link
Owner

Choose a reason for hiding this comment

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

I would still insist on having netstandard2.0 support via #if.

Copy link
Author

Choose a reason for hiding this comment

The 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?

Copy link
Owner

Choose a reason for hiding this comment

The 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>
Expand Down
143 changes: 96 additions & 47 deletions Cesium.Runtime/StringFunctions.cs
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;

Expand All @@ -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)
{
Copy link
Owner

Choose a reason for hiding this comment

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

I suggest introducing a constant for this 16, like MinimalPageBoundary or something, to make the size choice more clear.

Copy link
Author

Choose a reason for hiding this comment

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

Right, it's just Vector128<byte>.Count which I omitted because surrounding code is compact too, let me fix it. (to which we align - this is both for performance and for never crossing page boundary, whatever size it is, it occured to me we don't need actual page size here since it's always bigger than 16B and 16B alignment will never cross it)

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)
{
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
Expand Down Expand Up @@ -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());
}
}
2 changes: 1 addition & 1 deletion Cesium.Test.Framework/CSharpCompilationUtil.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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";

/// <summary>Semaphore that controls the amount of simultaneously running tests.</summary>
Expand Down
Loading