Implement NUID #147
Implement NUID #147
Conversation
| @@ -61,6 +64,42 @@ public partial class NatsConnection | |||
| } | |||
| } | |||
|
|
|||
| [SkipLocalsInit] | |||
| private static string NewInbox(ReadOnlySpan<char> prefix) | |||
There was a problem hiding this comment.
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 byte[]. But that would be a more invasive change.
There was a problem hiding this comment.
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:
| Method | Job | Runtime | Mean | Error | StdDev | Ratio | RatioSD | Gen0 | Allocated | Alloc Ratio |
|--------------------- |-------------- |-------------- |---------:|---------:|---------:|------:|--------:|-------:|----------:|------------:|
| NewInbox_ShortPrefix | .NET 6.0 | .NET 6.0 | 79.85 ns | 1.617 ns | 2.370 ns | 0.91 | 0.05 | 0.0612 | 128 B | 1.00 |
| NewInbox_ShortPrefix | .NET 7.0 | .NET 7.0 | 87.93 ns | 1.789 ns | 2.732 ns | 1.00 | 0.00 | 0.0612 | 128 B | 1.00 |
| NewInbox_ShortPrefix | .NET 8.0 | .NET 8.0 | 72.40 ns | 1.353 ns | 1.266 ns | 0.82 | 0.03 | 0.0612 | 128 B | 1.00 |
| NewInbox_ShortPrefix | NativeAOT 8.0 | NativeAOT 8.0 | 66.20 ns | 1.358 ns | 1.270 ns | 0.75 | 0.03 | 0.0612 | 128 B | 1.00 |
| | | | | | | | | | | |
| NewInbox_LongPrefix | .NET 6.0 | .NET 6.0 | 82.68 ns | 0.958 ns | 1.025 ns | 0.97 | 0.02 | 0.0994 | 208 B | 1.00 |
| NewInbox_LongPrefix | .NET 7.0 | .NET 7.0 | 85.93 ns | 0.881 ns | 0.688 ns | 1.00 | 0.00 | 0.0994 | 208 B | 1.00 |
| NewInbox_LongPrefix | .NET 8.0 | .NET 8.0 | 78.37 ns | 1.593 ns | 1.705 ns | 0.91 | 0.02 | 0.0994 | 208 B | 1.00 |
| NewInbox_LongPrefix | NativeAOT 8.0 | NativeAOT 8.0 | 72.60 ns | 1.492 ns | 1.532 ns | 0.85 | 0.01 | 0.0994 | 208 B | 1.00 |
| for (nuint i = PrefixLength; i < NuidLength; i++) | ||
| { | ||
| var digitIndex = (nuint)(sequential % Base); | ||
| Unsafe.Add(ref buffer[0], i) = Unsafe.Add(ref digitsPtr, digitIndex); |
There was a problem hiding this comment.
The usage of Unsafe is necessary here to avoid range checks, both when accessing buffer and Digits.
| private static string NewInbox(ReadOnlySpan<char> prefix) | ||
| { | ||
| Span<char> buffer = stackalloc char[64]; | ||
| var separatorLength = prefix.Length > 0 ? 1u : 0u; |
There was a problem hiding this comment.
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.
AFAIK, practice is to have a prefix so the accounts can be permissined accordingly.
There was a problem hiding this comment.
fwiw, Actually just realized an empty prefix might create interesting behaviours in subscription manager i.e. SubscriptionManager.IsInboxSubject() would always return true causing all subscriptions to go through an internal muxed 'inbox' subscription.
|
Thanks @jasper-d looks awesome! Sorry for the delay. Started reviewing today and trying to get my head around it. |
No worries, this isn't exactly urgent. |
There was a problem hiding this comment.
LGTM
Nicely follows the Go implementation.
Thanks @jasper-d. I have a question below out of curiosity, but please feel free to ignore it if you don't have time. It's not a merge-stopper.
Also a quick note on using subject as string: performance benefit seems negligible to me (e.g. about 1-2% overall allocations) at some point we should revisit though.
| rng.GetBytes(randomBytes); | ||
| } | ||
|
|
||
| var newPrefix = new char[PrefixLength]; |
There was a problem hiding this comment.
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.
No, there is no reason. I implemented NUID a long time ago using locks and never changed this.
Fixes #34
Opted to go with base-62 here to follow nuid.go. base-64 (as implemented and reviewed in #360) would avoid some bias and be about twice as fast.