[Json] JsonSerializer+PipeReader support #116947
Conversation
...s/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.ReadHelper.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Pipe.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/PipeReadBufferState.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/PipeReadBufferState.cs
Show resolved
Hide resolved
...s/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.ReadHelper.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IReadBufferState.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Pipe.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Pipe.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Pipe.cs
Outdated
Show resolved
Hide resolved
BrennanConroy
force-pushed
the
brecon/jsonpipereader
branch
from
4028ca5 to
cbfc005
Compare
Given we're very close to GA we should be doing this sooner rather than later. |
| public readonly bool IsFinalBlock => _isFinalBlock; | ||
|
|
||
| public readonly ReadOnlySpan<byte> Bytes => _buffer.AsSpan(_offset, _count); | ||
| public readonly ReadOnlySequence<byte> Bytes => new(_buffer.AsMemory(_offset, _count)); |
There was a problem hiding this comment.
I take it we're switching to sequence-based Utf8JsonReaders for streams with this change? Given we're close to RC, I'm slightly concerned that any issues with that implementation would surface as regressions in the stream-based APIs. Is there any chance we could keep this implementation in span mode? One way to do that is delegate Utf8JsonReader construction to a IReadBufferState method.
There was a problem hiding this comment.
We look at whether the ReadOnlySequence is a single segment and use the Span overload in that case. Stream will always be a single segment so there won't be any regressions there.
There was a problem hiding this comment.
Sure, but even so the instance is constructed using the multi-segnent modality so different code paths are being entered.
There was a problem hiding this comment.
It is still constructing the Utf8JsonReader with the ReadOnlySpan overload
The only difference is that we first create a ReadOnlySequence https://source.dot.net/#System.Memory/System/Buffers/ReadOnlySequence.cs,113
We can change IReadBufferState to have a construction method like you mentioned, but it doesn't change what is happening.
There was a problem hiding this comment.
If you feel confident that it won't cause any regressions, go for it. I just want us to be extra cautious given we're so close to RC.
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/Stream.WriteTests.cs
Outdated
Show resolved
Hide resolved
Do you want a PR opened for it that targets this branch? |
I don't think we need to, but any validation must be run before RC snap. |
eiriktsarpalis
left a comment
eiriktsarpalis
left a comment
There was a problem hiding this comment.
I think we're past the snap deadline, so this will probably need to be backported to the p7 branch. @tarekgh can you confirm?
...System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.WriteHelpers.cs
Show resolved
Hide resolved
|
/ba-g timeouts are known issues with azurelinux queue, test failures are also known test failures |
|
/backport to release/10.0-preview7 |
|
Started backporting to release/10.0-preview7: https://github.com/dotnet/runtime/actions/runs/16430376722 |
3 participants
Closes #68586
Didn't add any new tests for multi-segment deserialize as there is decent coverage in https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Utf8JsonReaderTests.MultiSegment.cs already and this PR doesn't add much on top of the
Utf8JsonReaderlayer. And we should be using fuzzing (separate PR after this one) to get more randomized segment size coverage.The
DeserializeAsyncEnumerableAPIs technically weren't part of the approved API, but they're just a copy of the Stream ones so should probably be fine.Added
IReadBufferState<TReadBufferState>to abstract processingPipeReaderandStream.Updated the test Pipe serializer to use the new APIs and added it to a bunch of the base tests.