Ensure that JSON properties can be skipped without buffering their entire contents.#96856
Conversation
ghost
added
the
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsMakes the following changes:
Fix #96559.
|
| return reader.HasValueSequence ? reader.ValueSequence.ToArray() : reader.ValueSpan; | ||
| } | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
Logic moved essentially unchanged from JsonConverter.ReadAhead.cs
| /// </summary> | ||
| /// <param name="targetDepth">The target depth we want to eventually skip to.</param> | ||
| /// <returns>True if the entire JSON value has been skipped.</returns> | ||
| internal bool TrySkipPartial(int targetDepth) |
There was a problem hiding this comment.
The existing TrySkipHelper has been retrofitted to the resumable TrySkipPartial method, minus the checkpointing semantics.
| { | ||
| // For a continuation, read ahead here to avoid having to build and then tear | ||
| // down the call stack if there is more than one buffer fetch necessary. | ||
| if (!SingleValueReadWithReadAhead(requiresReadAhead: true, ref reader, ref state)) |
There was a problem hiding this comment.
Here's the crux of the second change made by this PR: re-entrant serialization operations no longer perform read-ahead of the next value (the converter doesn't require it in this case). We let the JsonSerializerOptions.DefaultBufferSize be the only guide of how much data we add to the buffer before re-entering deserialization.
| #if DEBUG | ||
| // For performance, only perform token type validation of converters on debug builds. | ||
| Debug.Assert(state.Current.OriginalTokenType == JsonTokenType.None); | ||
| state.Current.OriginalTokenType = reader.TokenType; |
There was a problem hiding this comment.
We now need to store this value in all builds to track resumable skip operations.
| [Fact] | ||
| [SkipOnCoreClr("https://github.com/dotnet/runtime/issues/45464", ~RuntimeConfiguration.Release)] | ||
| public async Task ReadSimpleObjectAsync() | ||
| [Theory] |
There was a problem hiding this comment.
Converted this test suite to use theories because it was causing me grief while debugging.
| value = default; | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Changing the order of state update operations since we no longer rely on the root method to advance the reader for us on resumed operations.
2 participants
Makes the following changes:
Fix #96559.