-
Notifications
You must be signed in to change notification settings - Fork 55
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
Implement NUID #147
Implement NUID #147
Changes from all commits
0ece8ea
874e582
00d7086
e10dba0
b8c7d4e
0455339
420a338
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,51 @@ | ||
using System.Runtime.CompilerServices; | ||
using BenchmarkDotNet.Attributes; | ||
using BenchmarkDotNet.Jobs; | ||
using NATS.Client.Core; | ||
using NATS.Client.Core.Internal; | ||
|
||
namespace MicroBenchmark; | ||
|
||
[MemoryDiagnoser] | ||
[SimpleJob(RuntimeMoniker.Net60)] | ||
[SimpleJob(RuntimeMoniker.Net70, baseline: true)] | ||
[SimpleJob(RuntimeMoniker.Net80)] | ||
[SimpleJob(RuntimeMoniker.NativeAot80)] | ||
public class NewInboxBenchmarks | ||
{ | ||
private static readonly NatsOpts LongPrefixOpt = NatsOpts.Default | ||
with | ||
{ | ||
InboxPrefix = "this-is-a-rather-long-prefix-that-we-use-here", | ||
}; | ||
|
||
private static readonly NatsConnection ConnectionDefaultPrefix = new(); | ||
private static readonly NatsConnection ConnectionLongPrefix = new(LongPrefixOpt); | ||
|
||
private char[] _buf = new char[32]; | ||
|
||
[GlobalSetup] | ||
public void Setup() | ||
{ | ||
NuidWriter.TryWriteNuid(new char[100]); | ||
} | ||
|
||
[Benchmark(Baseline = true)] | ||
[SkipLocalsInit] | ||
public bool TryWriteNuid() | ||
{ | ||
return NuidWriter.TryWriteNuid(_buf); | ||
} | ||
|
||
[Benchmark] | ||
public string NewInbox_ShortPrefix() | ||
{ | ||
return ConnectionDefaultPrefix.NewInbox(); | ||
} | ||
|
||
[Benchmark] | ||
public string NewInbox_LongPrefix() | ||
{ | ||
return ConnectionLongPrefix.NewInbox(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,135 @@ | ||
using System.Diagnostics.CodeAnalysis; | ||
using System.Runtime.CompilerServices; | ||
using System.Runtime.InteropServices; | ||
using System.Security.Cryptography; | ||
|
||
namespace NATS.Client.Core.Internal; | ||
|
||
[SkipLocalsInit] | ||
internal sealed class NuidWriter | ||
{ | ||
internal const nuint NuidLength = PrefixLength + SequentialLength; | ||
private const nuint Base = 62; | ||
private const ulong MaxSequential = 839299365868340224; // 62^10 | ||
private const uint PrefixLength = 12; | ||
private const nuint SequentialLength = 10; | ||
private const int MinIncrement = 33; | ||
private const int MaxIncrement = 333; | ||
|
||
[ThreadStatic] | ||
private static NuidWriter? _writer; | ||
|
||
private char[] _prefix; | ||
private ulong _increment; | ||
private ulong _sequential; | ||
|
||
private NuidWriter() | ||
{ | ||
Refresh(out _); | ||
} | ||
|
||
private static ReadOnlySpan<char> Digits => "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"; | ||
|
||
public static bool TryWriteNuid(Span<char> nuidBuffer) | ||
{ | ||
if (_writer is not null) | ||
{ | ||
return _writer.TryWriteNuidCore(nuidBuffer); | ||
} | ||
|
||
return InitAndWrite(nuidBuffer); | ||
} | ||
|
||
private static bool TryWriteNuidCore(Span<char> buffer, Span<char> prefix, ulong sequential) | ||
{ | ||
if ((uint)buffer.Length < NuidLength || prefix.Length != PrefixLength) | ||
{ | ||
return false; | ||
} | ||
|
||
Unsafe.CopyBlockUnaligned(ref Unsafe.As<char, byte>(ref buffer[0]), ref Unsafe.As<char, byte>(ref prefix[0]), PrefixLength * sizeof(char)); | ||
|
||
// NOTE: We must never write to digitsPtr! | ||
ref var digitsPtr = ref MemoryMarshal.GetReference(Digits); | ||
|
||
for (nuint i = PrefixLength; i < NuidLength; i++) | ||
{ | ||
var digitIndex = (nuint)(sequential % Base); | ||
Unsafe.Add(ref buffer[0], i) = Unsafe.Add(ref digitsPtr, digitIndex); | ||
sequential /= Base; | ||
} | ||
|
||
return true; | ||
} | ||
|
||
private static uint GetIncrement() | ||
{ | ||
return (uint)Random.Shared.Next(MinIncrement, MaxIncrement + 1); | ||
} | ||
|
||
private static ulong GetSequential() | ||
{ | ||
return (ulong)Random.Shared.NextInt64(0, (long)MaxSequential + 1); | ||
} | ||
|
||
private static char[] GetPrefix(RandomNumberGenerator? rng = null) | ||
{ | ||
Span<byte> randomBytes = stackalloc byte[(int)PrefixLength]; | ||
|
||
// TODO: For .NET 8+, use GetItems for better distribution | ||
if (rng == null) | ||
{ | ||
RandomNumberGenerator.Fill(randomBytes); | ||
} | ||
else | ||
{ | ||
rng.GetBytes(randomBytes); | ||
} | ||
|
||
var newPrefix = new char[PrefixLength]; | ||
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. Just curious. Is there a technical reason why _prefix shouldn't be preallocated since it'd be on TLS? I assume it'd be over optimization since this isn't on hot path. Only a question really. 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. No, there is no reason. I implemented NUID a long time ago using locks and never changed this. |
||
|
||
for (var i = 0; i < randomBytes.Length; i++) | ||
{ | ||
var digitIndex = (int)(randomBytes[i] % Base); | ||
newPrefix[i] = Digits[digitIndex]; | ||
} | ||
|
||
return newPrefix; | ||
} | ||
|
||
[MethodImpl(MethodImplOptions.NoInlining)] | ||
private static bool InitAndWrite(Span<char> span) | ||
{ | ||
_writer = new NuidWriter(); | ||
return _writer.TryWriteNuidCore(span); | ||
} | ||
|
||
private bool TryWriteNuidCore(Span<char> nuidBuffer) | ||
{ | ||
var sequential = _sequential += _increment; | ||
|
||
if (sequential < MaxSequential) | ||
{ | ||
return TryWriteNuidCore(nuidBuffer, _prefix, sequential); | ||
} | ||
|
||
return RefreshAndWrite(nuidBuffer); | ||
|
||
[MethodImpl(MethodImplOptions.NoInlining)] | ||
bool RefreshAndWrite(Span<char> buffer) | ||
{ | ||
var prefix = Refresh(out sequential); | ||
return TryWriteNuidCore(buffer, prefix, sequential); | ||
} | ||
} | ||
|
||
[MethodImpl(MethodImplOptions.NoInlining)] | ||
[MemberNotNull(nameof(_prefix))] | ||
private char[] Refresh(out ulong sequential) | ||
{ | ||
var prefix = _prefix = GetPrefix(); | ||
_increment = GetIncrement(); | ||
sequential = _sequential = GetSequential(); | ||
return prefix; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,7 @@ | ||
using System.Diagnostics; | ||
using System.Diagnostics.CodeAnalysis; | ||
using System.Runtime.CompilerServices; | ||
using NATS.Client.Core.Internal; | ||
|
||
namespace NATS.Client.Core; | ||
|
||
|
@@ -7,7 +10,7 @@ public partial class NatsConnection | |
private static readonly NatsSubOpts DefaultReplyOpts = new() { MaxMsgs = 1 }; | ||
|
||
/// <inheritdoc /> | ||
public string NewInbox() => $"{InboxPrefix}{Guid.NewGuid():n}"; | ||
public string NewInbox() => NewInbox(InboxPrefix); | ||
|
||
/// <inheritdoc /> | ||
public async ValueTask<NatsMsg<TReply?>?> RequestAsync<TRequest, TReply>( | ||
|
@@ -61,6 +64,42 @@ public partial class NatsConnection | |
} | ||
} | ||
|
||
[SkipLocalsInit] | ||
private static string NewInbox(ReadOnlySpan<char> prefix) | ||
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. This method is a bit of a mess. It is rather complicated and still slow and allocates quite heavily. Allocations could be reduced by renting buffers instead of new'ing them, but that reduces throughput. Maybe it would be possible to not operate with strings (apart from public interfaces) for inbox prefix/subjects and instead use only 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. E.g. this alternative implementation which looks much more reasonable [SkipLocalsInit]
private static string NewInbox(ReadOnlySpan<char> prefix)
{
Span<char> buffer = stackalloc char[22];
NuidWriter.TryWriteNuid(buffer);
if (prefix.IsEmpty)
{
return new string(buffer);
}
else
{
return $"{prefix}.{buffer}";
}
} is slower but also reduces the allocations for long prefixes:
|
||
{ | ||
Span<char> buffer = stackalloc char[64]; | ||
var separatorLength = prefix.Length > 0 ? 1u : 0u; | ||
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'm not sure if support for empty prefixes is really needed, but right now nothing is preventing users from doing it. 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. AFAIK, practice is to have a prefix so the accounts can be permissined accordingly. 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. fwiw, Actually just realized an empty prefix might create interesting behaviours in subscription manager i.e. |
||
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) | ||
{ | ||
prefix.CopyTo(buffer); | ||
buffer[prefix.Length] = '.'; | ||
var remaining = buffer.Slice((int)totalPrefixLength); | ||
var didWrite = NuidWriter.TryWriteNuid(remaining); | ||
Debug.Assert(didWrite, "didWrite"); | ||
return new string(buffer); | ||
} | ||
|
||
return Throw(); | ||
|
||
[DoesNotReturn] | ||
string Throw() | ||
{ | ||
Debug.Fail("Must not happen"); | ||
throw new InvalidOperationException("This should never be raised!"); | ||
} | ||
} | ||
|
||
private NatsSubOpts SetReplyOptsDefaults(NatsSubOpts? replyOpts) | ||
{ | ||
var opts = replyOpts ?? DefaultReplyOpts; | ||
|
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.
The usage of
Unsafe
is necessary here to avoid range checks, both when accessing buffer and Digits.