Add flags to NatsMsg and JetStream Request #652
Conversation
src/NATS.Client.Core/NatsMsg.cs
Outdated
| { | ||
| Subject = subject; | ||
| ReplyTo = replyTo; | ||
| Size = size; |
There was a problem hiding this comment.
Is it worth setting _flagsAndSize directly here? (Sharplab is not cooperating so I can't check whether it gets optimized as is or not)
Edit: Got it working, do double check my bitshift but the ctor on NatsMsg2 looks way cleaner
|
|
||
| namespace NATS.Client.JetStream.Internal; | ||
|
|
||
| internal sealed class NatsJSForcedJsonDocumentSerializer<T> : INatsDeserialize<T> |
There was a problem hiding this comment.
Worth making this internal sealed class NatsJSForcedJsonDocumentSerializer : INatsDeserialize<JsonDocument>?
| } | ||
|
|
||
| [Fact] | ||
| public void Check_struct_size() |
There was a problem hiding this comment.
Awesome to see this added.
src/NATS.Client.Core/NatsMsg.cs
Outdated
|
|
||
| public bool IsEmpty => (Flags & NatsMsgFlags.Empty) == NatsMsgFlags.Empty; | ||
|
|
||
| public bool HasNoResponders => (Flags & NatsMsgFlags.NoResponders) == NatsMsgFlags.NoResponders; |
There was a problem hiding this comment.
Super cool to note , IsEmpty reduces to very simple ASM even as-is on modern runtimes.
NatsMsg`1[[System.Int32, System.Private.CoreLib]].get_IsEmpty()
L0000: xor eax, eax
L0002: test dword ptr [rcx+0x20], 0x40000000
L0009: setne al
L000c: ret
Strangely, HasNoResponders does reduce quite as well; I wonder if it's due to it's spot being a sign bit? 🤔
It's fine as is but still an interesting curiosity.
NatsMsg`1[[System.Int32, System.Private.CoreLib]].get_HasNoResponders()
L0000: mov eax, [rcx+0x20]
L0003: shr eax, 0x1e
L0006: test al, 2
L0008: setne al
L000b: movzx eax, al
L000e: ret
There was a problem hiding this comment.
interesting. if I changed getter to:
public bool HasNoResponders => (_flagsAndSize & 0x80000000) != 0;I get the same code:
NATS.Client.Core.dll!NATS.Client.Core.NatsMsg<T>.HasNoResponders.get():
00007FF8841C5B80 xor eax,eax
00007FF8841C5B82 test dword ptr [rcx+20h],80000000h
00007FF8841C5B89 setne almy guess is after inlined
public bool IsEmpty => ((_flagsAndSize >> 30) & 1) == 1;
public bool HasNoResponders => ((_flagsAndSize >> 30) & 2) == 2;there must be an optimization for x & 1 == 1 only.
| if (payloadBuffer.Length == 0) | ||
| { | ||
| flags |= NatsMsgFlags.Empty; | ||
| if (NatsSubBase.IsHeader503(headersBuffer)) |
There was a problem hiding this comment.
it's pretty specific where server sets the 503 header for no responders and Go client seems to check for data being empty too. tbh I think no-responders is the only usage of 503 so i suppose it would be ok to move it out idk.
There was a problem hiding this comment.
Oh nvm I guess I didn't realize they noresponders only came with empty 😅
Reordered error checks in `NatsJSContext.cs` to prioritize handling `HasNoResponders` and `null` data conditions early. Modified flag checks in `NatsMsg.cs` for `IsEmpty` and `HasNoResponders` by directly using bitwise operations instead of enum comparisons.
| // .NET 6 new APIs to the rescue: we can read the buffer once | ||
| // by deserializing into a document, inspect and using the new | ||
| // API deserialize to the final type from the document. | ||
| var jsonDocument = msg.Data; |
There was a problem hiding this comment.
There may be a concern here, due to the fact that JsonDocument is IDisposable and based on how the Parse function for ReadOnlySequence looks it is a case where we are likely pulling in a buffer.
There was a problem hiding this comment.
first just to clarify; the idea/assumption is that there may be two different types of JSON data coming in (the expected type like StreamConfig or an Error) and we are able to parse the binary data once, detecting if it's error, then convert the JsonDocument into the requested type. So all that said if there is a better way we can do this, we should try. Also this is assumption. I will see if I can benchmark this.
I kind of see what you mean with the extra buffer (need to look closer).
another option is that the good thing is the type information seems to be always at the front. with a short look-ahead maybe we can identify the document then use the correct json context to parse it.
I wonder if there is a solution for this already?
There was a problem hiding this comment.
TBH I think we are fine as long as we dispose our jsondocument after getting our part, but will check if I have time this weekend.
This is part of preparing to reduce exception usage as stated in #581.
Note that a similar attempt was made in #275 but due to NatsMsg struct size getting larger it wasn't merged. With this PR we're using the message size fields left most two bits to set the flags relying on the fact a 30-bit field would still be large enough to hold message size int.
JetStream request changed to handle API error responses without throwing exceptions.