Use string.Create when building a new inbox string (#551)

* Use GetItems for Nuid prefix * Use string.Create when building inbox names * Fix SA1122
nats-io · Jul 15, 2024 · bbab5a8 · bbab5a8
1 parent 6833ecf
commit bbab5a8
Show file tree

Hide file tree

Showing 5 changed files with 59 additions and 44 deletions.
diff --git a/sandbox/MicroBenchmark/NewInboxBenchmarks.cs b/sandbox/MicroBenchmark/NewInboxBenchmarks.cs
@@ -7,9 +7,7 @@
 namespace MicroBenchmark;
 
 [MemoryDiagnoser]
-[SimpleJob(RuntimeMoniker.Net60)]
-[SimpleJob(RuntimeMoniker.Net70, baseline: true)]
-[SimpleJob(RuntimeMoniker.Net80)]
+[SimpleJob(RuntimeMoniker.Net80, baseline: true)]
 [SimpleJob(RuntimeMoniker.NativeAot80)]
 public class NewInboxBenchmarks
 {

diff --git a/src/NATS.Client.Core/Internal/NuidWriter.cs b/src/NATS.Client.Core/Internal/NuidWriter.cs
@@ -99,6 +99,13 @@ private static ulong GetSequential()
 
     private static char[] GetPrefix(RandomNumberGenerator? rng = null)
     {
+#if NET8_0_OR_GREATER
+        if (rng == null)
+        {
+            return RandomNumberGenerator.GetItems(Digits, (int)PrefixLength);
+        }
+#endif
+
 #if NETSTANDARD2_0
         var randomBytes = new byte[(int)PrefixLength];
 
@@ -109,8 +116,6 @@ private static char[] GetPrefix(RandomNumberGenerator? rng = null)
         }
 #else
         Span<byte> randomBytes = stackalloc byte[(int)PrefixLength];
-
-        // TODO: For .NET 8+, use GetItems for better distribution
         if (rng == null)
         {
             RandomNumberGenerator.Fill(randomBytes);

diff --git a/src/NATS.Client.Core/NatsConnection.RequestReply.cs b/src/NATS.Client.Core/NatsConnection.RequestReply.cs
@@ -1,5 +1,4 @@
 using System.Diagnostics;
-using System.Diagnostics.CodeAnalysis;
 using System.Runtime.CompilerServices;
 using NATS.Client.Core.Internal;
 
@@ -105,44 +104,29 @@ public async IAsyncEnumerable<NatsMsg<TReply>> RequestManyAsync<TRequest, TReply
         }
     }
 
-#if NETSTANDARD2_0
-    internal static string NewInbox(string prefix) => NewInbox(prefix.AsSpan());
-#endif
-
     [SkipLocalsInit]
-    internal static string NewInbox(ReadOnlySpan<char> prefix)
+    internal static string NewInbox(string prefix)
     {
-        Span<char> buffer = stackalloc char[64];
-        var separatorLength = prefix.Length > 0 ? 1u : 0u;
-        var totalLength = (uint)prefix.Length + (uint)NuidWriter.NuidLength + separatorLength;
-        if (totalLength <= buffer.Length)
-        {
-            buffer = buffer.Slice(0, (int)totalLength);
-        }
-        else
-        {
-            buffer = new char[totalLength];
-        }
-
-        var totalPrefixLength = (uint)prefix.Length + separatorLength;
-        if ((uint)buffer.Length > totalPrefixLength && (uint)buffer.Length > (uint)prefix.Length)
+        static void WriteBuffer(Span<char> buffer, (string prefix, int pfxLen) state)
         {
-            prefix.CopyTo(buffer);
-            buffer[prefix.Length] = '.';
-            var remaining = buffer.Slice((int)totalPrefixLength);
+            state.prefix.AsSpan().CopyTo(buffer);
+            buffer[state.prefix.Length] = '.';
+            var remaining = buffer.Slice(state.pfxLen);
             var didWrite = NuidWriter.TryWriteNuid(remaining);
             Debug.Assert(didWrite, "didWrite");
-            return buffer.ToString();
         }
 
-        return Throw();
+        var separatorLength = prefix.Length > 0 ? 1 : 0;
+        var totalLength = prefix.Length + (int)NuidWriter.NuidLength + separatorLength;
+        var totalPrefixLength = prefix.Length + separatorLength;
 
-        [DoesNotReturn]
-        string Throw()
-        {
-            Debug.Fail("Must not happen");
-            throw new InvalidOperationException("This should never be raised!");
-        }
+#if NET6_0_OR_GREATER || NETSTANDARD2_1
+        return string.Create(totalLength, (prefix, totalPrefixLength), (buf, state) => WriteBuffer(buf, state));
+#else
+        Span<char> buffer = new char[totalLength];
+        WriteBuffer(buffer, (prefix, totalPrefixLength));
+        return buffer.ToString();
+#endif
     }
 
     private NatsSubOpts SetReplyOptsDefaults(NatsSubOpts? replyOpts)

diff --git a/tests/NATS.Client.Core.Tests/NatsConnectionTest.cs b/tests/NATS.Client.Core.Tests/NatsConnectionTest.cs
@@ -1,10 +1,6 @@
-using System.Diagnostics;
 using System.Reflection;
 using System.Text;
 using System.Text.Json.Serialization;
-using System.Text.RegularExpressions;
-using System.Threading.Channels;
-using Xunit.Sdk;
 
 namespace NATS.Client.Core.Tests;
 
@@ -383,6 +379,39 @@ public void InterfaceShouldHaveSamePublicPropertiesEventsAndMethodAsClass()
             interfaceMethods.Select(m => m.Name).Should().Contain(name);
         }
     }
+
+    [Fact]
+    public void NewInboxEmptyPrefixReturnsNuid()
+    {
+        var opts = NatsOpts.Default with { InboxPrefix = string.Empty };
+        var conn = new NatsConnection(opts);
+
+        var inbox = conn.NewInbox();
+
+        Assert.Matches("[A-z0-9]{22}", inbox);
+    }
+
+    [Fact]
+    public void NewInboxNonEmptyPrefixReturnsPrefixWithNuid()
+    {
+        var opts = NatsOpts.Default with { InboxPrefix = "PREFIX" };
+        var conn = new NatsConnection(opts);
+
+        var inbox = conn.NewInbox();
+
+        Assert.Matches("PREFIX\\.[A-z0-9]{22}", inbox);
+    }
+
+    [Fact]
+    public void NewInboxVeryLongPrefixReturnsPrefixWithNuid()
+    {
+        var opts = NatsOpts.Default with { InboxPrefix = new string('A', 512) };
+        var conn = new NatsConnection(opts);
+
+        var inbox = conn.NewInbox();
+
+        Assert.Matches("A{512}\\.[A-z0-9]{22}", inbox);
+    }
 }
 
 [JsonSerializable(typeof(SampleClass))]

diff --git a/tests/NATS.Client.Core.Tests/NuidWriterTests.cs b/tests/NATS.Client.Core.Tests/NuidWriterTests.cs
@@ -17,20 +17,19 @@ public NuidWriterTests(ITestOutputHelper outputHelper)
     }
 
     [Theory]
-    [InlineData(default(string))]
     [InlineData("")]
     [InlineData("__INBOX")]
     [InlineData("long-inbox-prefix-above-stackalloc-limit-of-64")]
-    public void NewInbox_NuidAppended(string? prefix)
+    public void NewInbox_NuidAppended(string prefix)
     {
         var natsOpts = NatsOpts.Default with { InboxPrefix = prefix! };
         var sut = new NatsConnection(natsOpts);
 
         var inbox = sut.InboxPrefix;
         var newInbox = sut.NewInbox();
 
-        Assert.Matches($"{prefix ?? string.Empty}{(prefix?.Length > 0 ? "." : string.Empty)}[A-z0-9]{{22}}", inbox);
-        Assert.Matches($"{prefix ?? string.Empty}{(prefix?.Length > 0 ? "." : string.Empty)}[A-z0-9]{{22}}.[A-z0-9]{{22}}", newInbox);
+        Assert.Matches($"{prefix}{(prefix.Length > 0 ? "." : string.Empty)}[A-z0-9]{{22}}", inbox);
+        Assert.Matches($"{prefix}{(prefix.Length > 0 ? "." : string.Empty)}[A-z0-9]{{22}}.[A-z0-9]{{22}}", newInbox);
         _outputHelper.WriteLine($"Prefix:   '{prefix}'");
         _outputHelper.WriteLine($"Inbox:    '{inbox}'");
         _outputHelper.WriteLine($"NewInbox: '{newInbox}'");