String and byte[] converters using partial reads/writes #112129
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis |
| /// This is used when the Stream-based serialization APIs are used. | ||
| /// </summary> | ||
| /// <typeparam name="T"></typeparam> | ||
| internal abstract class JsonHybridResumableConverter<T> : JsonConverter<T> |
There was a problem hiding this comment.
How is this different compared to the existing resumable converter base?
There was a problem hiding this comment.
It exposes WriteWithoutStackFrame which doesn't push/pop stack frames by default but based on the value (if it's long or serialization is done via streaming) there's a method to push/pop the stack frames. This keeps the common case of short strings/byte[] fast by not needing a stack frame but still allows large strings to opt into stack management.
| @@ -46,6 +46,8 @@ public ref partial struct Utf8JsonReader | |||
| private SequencePosition _currentPosition; | |||
| private readonly ReadOnlySequence<byte> _sequence; | |||
|
|
|||
| internal bool _hasPartialStringValue; | |||
There was a problem hiding this comment.
Like with Utf8JsonWriter, couldn't this just use the same special JsonTokenType value to reflect segmented reads?
| @@ -1300,6 +1302,7 @@ private bool ConsumeString() | |||
| ValueIsEscaped = false; | |||
| _tokenType = JsonTokenType.String; | |||
| _consumed += idx + 2; | |||
| _hasPartialStringValue = false; | |||
There was a problem hiding this comment.
This flags is introducing a new state for Utf8JsonReader that could break user-defined converters not expecting it. We should make absolutely sure instances setting this flag should never leak via public APIs. I would recommend adding a few debug asserts e.g. in the Read() method checking that the flag is always unset.
| @@ -29,6 +33,176 @@ public override void Write(Utf8JsonWriter writer, byte[]? value, JsonSerializerO | |||
| } | |||
| } | |||
|
|
|||
| internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, scoped ref ReadStack state, out byte[]? value) | |||
| { | |||
| if (state.Current.ObjectState < StackFrameObjectState.CreatedObject) | |||
There was a problem hiding this comment.
Like with existing resumable converters, there should be a fast path that runs when SupportContinuation is set to false. In this case we can assume that the string has been fully populated and therefore we can read it using the non-segmenting APIs.
There was a problem hiding this comment.
This is handled in JsonConverter.TryRead where we check ConverterStrategy == ConverterStrategy.SegmentableValue && !state.IsContinuation && !reader._hasPartialStringValue which guarantees that only partially populated buffers will go through this path since fully populated buffers would have reader._hasPartialStringValue == false. It's too late to do the check here since the stack already is pushed at this point so it needs to be in JsonConverter.TryRead or earlier.
There was a problem hiding this comment.
JsonConverter.TryRead is a hot path method and historically changing it has been causing a great deal of performance regressions. I would recommend moving as much of the logic as possible inside the resumable converters themselves. I would also recommend keeping as much consistency as possible with the existing resumable converters, since their code is extremely nontrivial and difficult to follow.
No description provided.