-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add interop between serializer and DOMs #56112
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,7 +23,13 @@ public sealed partial class JsonDocument : IDisposable | |
{ | ||
private ReadOnlyMemory<byte> _utf8Json; | ||
private MetadataDb _parsedData; | ||
private byte[]? _extraRentedBytes; | ||
|
||
private byte[]? _extraRentedArrayPoolBytes; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are the extra flags needed? They seem to be equivalent to checking whether the rented references are null. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The existing pattern was thread-safe using |
||
private bool _hasExtraRentedArrayPoolBytes; | ||
|
||
private PooledByteBufferWriter? _extraPooledByteBufferWriter; | ||
private bool _hasExtraPooledByteBufferWriter; | ||
|
||
private (int, string?) _lastIndexAndString = (-1, null); | ||
|
||
internal bool IsDisposable { get; } | ||
|
@@ -36,19 +42,34 @@ public sealed partial class JsonDocument : IDisposable | |
private JsonDocument( | ||
ReadOnlyMemory<byte> utf8Json, | ||
MetadataDb parsedData, | ||
byte[]? extraRentedBytes, | ||
byte[]? extraRentedArrayPoolBytes = null, | ||
PooledByteBufferWriter? extraPooledByteBufferWriter = null, | ||
bool isDisposable = true) | ||
{ | ||
Debug.Assert(!utf8Json.IsEmpty); | ||
|
||
// We never have both rented fields. | ||
Debug.Assert(extraRentedArrayPoolBytes == null || extraPooledByteBufferWriter == null); | ||
|
||
_utf8Json = utf8Json; | ||
_parsedData = parsedData; | ||
_extraRentedBytes = extraRentedBytes; | ||
|
||
if (_extraRentedArrayPoolBytes != null) | ||
{ | ||
_hasExtraRentedArrayPoolBytes = true; | ||
_extraRentedArrayPoolBytes = extraRentedArrayPoolBytes; | ||
} | ||
else if (extraPooledByteBufferWriter != null) | ||
{ | ||
_hasExtraPooledByteBufferWriter = true; | ||
_extraPooledByteBufferWriter = extraPooledByteBufferWriter; | ||
} | ||
|
||
|
||
IsDisposable = isDisposable; | ||
|
||
// extraRentedBytes better be null if we're not disposable. | ||
Debug.Assert(isDisposable || extraRentedBytes == null); | ||
// Both rented fields better be null if we're not disposable. | ||
Debug.Assert(isDisposable || (_extraRentedArrayPoolBytes == null && _extraPooledByteBufferWriter == null)); | ||
} | ||
|
||
/// <inheritdoc /> | ||
|
@@ -65,12 +86,20 @@ public void Dispose() | |
|
||
// When "extra rented bytes exist" they contain the document, | ||
// and thus need to be cleared before being returned. | ||
byte[]? extraRentedBytes = Interlocked.Exchange(ref _extraRentedBytes, null); | ||
if (_hasExtraRentedArrayPoolBytes) | ||
{ | ||
byte[]? extraRentedBytes = Interlocked.Exchange(ref _extraRentedArrayPoolBytes, null); | ||
|
||
if (extraRentedBytes != null) | ||
if (extraRentedBytes != null) | ||
{ | ||
extraRentedBytes.AsSpan(0, length).Clear(); | ||
ArrayPool<byte>.Shared.Return(extraRentedBytes); | ||
} | ||
} | ||
else if (_hasExtraPooledByteBufferWriter) | ||
{ | ||
extraRentedBytes.AsSpan(0, length).Clear(); | ||
ArrayPool<byte>.Shared.Return(extraRentedBytes); | ||
PooledByteBufferWriter? extraBufferWriter = Interlocked.Exchange(ref _extraPooledByteBufferWriter, null); | ||
extraBufferWriter?.Dispose(); | ||
} | ||
} | ||
|
||
|
@@ -184,7 +213,7 @@ internal int GetEndIndex(int index, bool includeEndElement) | |
return endIndex; | ||
} | ||
|
||
private ReadOnlyMemory<byte> GetRawValue(int index, bool includeQuotes) | ||
internal ReadOnlyMemory<byte> GetRawValue(int index, bool includeQuotes) | ||
{ | ||
CheckNotDisposed(); | ||
|
||
|
@@ -764,7 +793,12 @@ internal JsonElement CloneElement(int index) | |
ReadOnlyMemory<byte> segmentCopy = GetRawValue(index, includeQuotes: true).ToArray(); | ||
|
||
JsonDocument newDocument = | ||
new JsonDocument(segmentCopy, newDb, extraRentedBytes: null, isDisposable: false); | ||
new JsonDocument( | ||
segmentCopy, | ||
newDb, | ||
extraRentedArrayPoolBytes: null, | ||
extraPooledByteBufferWriter: null, | ||
isDisposable: false); | ||
|
||
return newDocument.RootElement; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the
SerializetTo*
methods also come in async variants?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we could have added async; it wasn't part of the ask. The approved list of API permutations were already greater than originally expected.
Today,
JsonDocument
only hasParseAsync()
(no serialize behavior) so the workaround for that is something like: