Use string.Create when building a new inbox string

[jasper-d]

Use a more idiomatic implementation for NewInbox and improve performance.

Method	Rev	Runtime	Mean	Error	StdDev	Gen0	Allocated
TryWriteNuid	Base	.NET 8.0	31.92 ns	0.672 ns	0.800 ns	-	-
TryWriteNuid	PR	.NET 8.0	30.66 ns	0.650 ns	1.171 ns	-	-
NewInbox_ShortPrefix	Base	.NET 8.0	59.64 ns	1.258 ns	1.921 ns	0.0612	128 B
NewInbox_ShortPrefix	PR	.NET 8.0	46.89 ns	0.945 ns	1.865 ns	0.0612	128 B
NewInbox_LongPrefix	Base	.NET 8.0	79.69 ns	1.674 ns	4.230 ns	0.1988	416 B
NewInbox_LongPrefix	PR	.NET 8.0	49.29 ns	1.052 ns	2.331 ns	0.0994	208 B
TryWriteNuid	Base	NativeAOT 8.0	27.82 ns	0.468 ns	0.520 ns	-	-
TryWriteNuid	PR	NativeAOT 8.0	26.19 ns	0.527 ns	0.467 ns	-	-
NewInbox_ShortPrefix	Base	NativeAOT 8.0	54.16 ns	0.955 ns	0.981 ns	0.0612	128 B
NewInbox_ShortPrefix	PR	NativeAOT 8.0	46.18 ns	0.925 ns	0.908 ns	0.0612	128 B
NewInbox_LongPrefix	Base	NativeAOT 8.0	71.15 ns	0.700 ns	0.621 ns	0.1988	416 B
NewInbox_LongPrefix	PR	NativeAOT 8.0	49.10 ns	0.748 ns	0.625 ns	0.0994	208 B

Looks like tests target .NET6/8 only. I think that the NETSTANDARD2_0 path works too, but I don't have a setup to test that 🤞

6299995 is only tangentially related but I thought I could sneak it in.

[mtmk]

@jasper-d

Looks like tests target .NET6/8 only. I think that the NETSTANDARD2_0 path works too, but I don't have a setup to test that 🤞

We have NATS.Client.Platform.Windows.Tests on Windows net481 runs as part of memory tests (since they were running on Windows host already): https://github.com/nats-io/nats.net.v2/actions/runs/9848378574/job/27205978145?pr=551#step:9:1

[jasper-d]

We have NATS.Client.Platform.Windows.Tests on Windows net481 runs as part of memory tests

Great, then it's covered :)

There are two other places I found that could be optimized in a similar way, but I'm not sure if they are hot enough to justify the #if hell and general lack of readability:

From 6f061898810a4fb923882ee8b4bb07ba77c3bbbc Mon Sep 17 00:00:00 2001
From: jasper-d <jasper-d@users.noreply.github.com>
Date: Wed, 10 Jul 2024 20:43:11 +0200
Subject: [PATCH] Allocate less

---
 src/NATS.Client.Core/Internal/NuidWriter.cs   | 15 ++++++----
 .../NatsJSContext.Consumers.cs                | 29 +++++++++++++++++--
 2 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/src/NATS.Client.Core/Internal/NuidWriter.cs b/src/NATS.Client.Core/Internal/NuidWriter.cs
index e8a5785..30edeee 100644
--- a/src/NATS.Client.Core/Internal/NuidWriter.cs
+++ b/src/NATS.Client.Core/Internal/NuidWriter.cs
@@ -54,13 +54,16 @@ internal sealed class NuidWriter
 
     public static string NewNuid()
     {
-        Span<char> buffer = stackalloc char[22];
-        if (TryWriteNuid(buffer))
+#if NET6_0_OR_GREATER || NETSTANDARD2_1
+        return string.Create((int)NuidLength, default(object), (buffer, _) =>
         {
-            return buffer.ToString();
-        }
-
-        throw new InvalidOperationException("Internal error: can't generate nuid");
+            TryWriteNuid(buffer);
+        });
+#else
+        Span<char> buffer = stackalloc char[(int)NuidLength];
+        TryWriteNuid(buffer);
+        return buffer.ToString();
+#endif
     }
 
     private static bool TryWriteNuidCore(Span<char> buffer, Span<char> prefix, ulong sequential)
diff --git a/src/NATS.Client.JetStream/NatsJSContext.Consumers.cs b/src/NATS.Client.JetStream/NatsJSContext.Consumers.cs
index 45a7864..384d36d 100644
--- a/src/NATS.Client.JetStream/NatsJSContext.Consumers.cs
+++ b/src/NATS.Client.JetStream/NatsJSContext.Consumers.cs
@@ -1,3 +1,4 @@
+using System.Diagnostics;
 using System.Runtime.CompilerServices;
 using NATS.Client.Core.Internal;
 using NATS.Client.JetStream.Models;
@@ -228,8 +229,32 @@ public partial class NatsJSContext : INatsJSContext
             request.Config.FilterSubjects = opts.FilterSubjects;
         }
 
-        var name = NuidWriter.NewNuid();
-        var subject = $"{Opts.Prefix}.CONSUMER.CREATE.{stream}.{name}";
+#if NET6_0_OR_GREATER || NETSTANDARD2_1
+        // This could also be written like this to avoid the extra string alloc
+        // from NuidWriter.NewNuid at the costs of being ~2x as slow as
+        // string.Create.
+        //
+        // Span<char> nuid = stackalloc char[(int)NuidWriter.NuidLength];
+        // NuidWriter.TryWriteNuid(nuid);
+        // var subject = $"{Opts.Prefix}.CONSUMER.CREATE.{stream}.{nuid}";
+        const string consumerCreate = ".CONSUMER.CREATE.";
+        var len = consumerCreate.Length + (int)NuidWriter.NuidLength + stream.Length + Opts.Prefix.Length;
+
+        var subject = string.Create(len, (Opts.Prefix, stream), static (buffer, state) =>
+        {
+            state.Prefix.AsSpan().CopyTo(buffer);
+            var remaining = buffer.Slice(state.Prefix.Length);
+            consumerCreate.AsSpan().CopyTo(remaining);
+            remaining = remaining.Slice(consumerCreate.Length);
+            state.stream.AsSpan().CopyTo(remaining);
+            remaining = remaining.Slice(state.stream.Length);
+            var didWrite = NuidWriter.TryWriteNuid(remaining);
+            Debug.Assert(remaining.Length == NuidWriter.NuidLength, "remaining.Length == NuidWriter.NuidLength");
+        });
+#else
+        var nuid = NuidWriter.NewNuid();
+        var subject = $"{Opts.Prefix}.CONSUMER.CREATE.{stream}.{nuid}";
+#endif
 
         return JSRequestResponseAsync<ConsumerCreateRequest, ConsumerInfo>(
             subject: subject,
-- 
2.45.2

[mtmk]

We have NATS.Client.Platform.Windows.Tests on Windows net481 runs as part of memory tests

Great, then it's covered :)

yeah I think so too ;)

There are two other places I found that could be optimized in a similar way, but I'm not sure if they are hot enough to justify the #if hell and general lack of readability:

I can't see them being on a hot path, maybe NewNuid() more than the consumer creation. (string.Create() is pretty cool btw) #ifs don't look too bad. it's up to you 💯

[jasper-d]

Let's keep it the way it is then. You'll have to maintain it.

[mtmk]

LGTM thanks @jasper-d