-
Couldn't load subscription status.
- Fork 5.2k
Improve JSON validation perf #111332
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
Improve JSON validation perf #111332
Changes from 7 commits
47748d1
f80bd84
71439c5
c6cff9b
b4b98ec
ef95e76
36df713
adca8ae
5901725
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 |
|---|---|---|
|
|
@@ -12,54 +12,113 @@ namespace System.Text.Json | |
| { | ||
| public sealed partial class Utf8JsonWriter | ||
| { | ||
| private bool HasPartialStringData => PartialStringDataLength != 0; | ||
| /// <summary> | ||
| /// Assuming that the writer is currently in a valid state, this returns true if a JSON value is allowed at the current position. | ||
PranavSenthilnathan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| /// <list type="bullet"> | ||
| /// <item> | ||
| /// If <see cref="_enclosingContainer"/> is an array then writing a value is always allowed. | ||
PranavSenthilnathan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| /// </item> | ||
| /// <item> | ||
| /// If <see cref="_enclosingContainer"/> is an object then writing a value is allowed only if <see cref="_tokenType"/> is a property name. | ||
| /// Because we designed <see cref="EnclosingContainerType.Object"/> == <see cref="JsonTokenType.PropertyName"/>, we can just check for equality. | ||
| /// </item> | ||
| /// <item> | ||
| /// If <see cref="_enclosingContainer"/> is none (the root level) then writing a value is allowed only if <see cref="_tokenType"/> is None (only | ||
| /// one value may be written at the root). This case is identical to the previous case. | ||
| /// </item> | ||
| /// <item> | ||
| /// If <see cref="_enclosingContainer"/> is a partial value, then it will never be a valid <see cref="_tokenType"/> by construction. | ||
| /// </item> | ||
| /// </list> | ||
| /// This method performs better without short circuiting (this often gets inlined so using simple branch free code seems to have some benefits). | ||
| /// </summary> | ||
| private bool CanWriteValue => _enclosingContainer == EnclosingContainerType.Array | (byte)_enclosingContainer == (byte)_tokenType; | ||
|
|
||
| private void ClearPartialStringData() => PartialStringDataLength = 0; | ||
| private bool HasPartialStringData => _partialStringDataLength != 0; | ||
|
|
||
| private void ValidateEncodingDidNotChange(SegmentEncoding currentSegmentEncoding) | ||
| private void ClearPartialStringData() => _partialStringDataLength = 0; | ||
|
|
||
| private void ValidateWritingValue() | ||
| { | ||
| if (PreviousSegmentEncoding != currentSegmentEncoding) | ||
| if (!CanWriteValue) | ||
| { | ||
| ThrowHelper.ThrowInvalidOperationException_CannotMixEncodings(PreviousSegmentEncoding, currentSegmentEncoding); | ||
| OnValidateWritingValueFailed(); | ||
| } | ||
| } | ||
|
|
||
| private void ValidateNotWithinUnfinalizedString() | ||
| [DoesNotReturn] | ||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
|
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.
These helpers should probably follow the pattern like |
||
| private void OnValidateWritingValueFailed() | ||
| { | ||
| if (_tokenType == StringSegmentSentinel) | ||
| Debug.Assert(!_options.SkipValidation); | ||
|
|
||
| if (IsWritingPartialString) | ||
| { | ||
| ThrowHelper.ThrowInvalidOperationException(ExceptionResource.CannotWriteWithinString, currentDepth: default, maxDepth: _options.MaxDepth, token: default, _tokenType); | ||
| ThrowInvalidOperationException(ExceptionResource.CannotWriteWithinString); | ||
| } | ||
|
|
||
| Debug.Assert(PreviousSegmentEncoding == SegmentEncoding.None); | ||
| Debug.Assert(!HasPartialStringData); | ||
|
|
||
| if (_enclosingContainer == EnclosingContainerType.Object) | ||
| { | ||
| Debug.Assert(_tokenType != JsonTokenType.PropertyName); | ||
| Debug.Assert(_tokenType != JsonTokenType.None && _tokenType != JsonTokenType.StartArray); | ||
| ThrowInvalidOperationException(ExceptionResource.CannotWriteValueWithinObject); | ||
| } | ||
| else | ||
| { | ||
| Debug.Assert(_tokenType != JsonTokenType.PropertyName); | ||
| Debug.Assert(CurrentDepth == 0 && _tokenType != JsonTokenType.None); | ||
| ThrowInvalidOperationException(ExceptionResource.CannotWriteValueAfterPrimitiveOrClose); | ||
| } | ||
| } | ||
|
|
||
| private void ValidateWritingValue() | ||
| private void ValidateWritingSegment(EnclosingContainerType currentSegmentEncoding) | ||
PranavSenthilnathan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| Debug.Assert(!_options.SkipValidation); | ||
| // A string segment can be written if either: | ||
| // 1) The writer is currently in a partial string of the same type. In this case the new segment | ||
| // will continue the partial string. | ||
| // - or - | ||
| // 2) The writer can write a value at the current position, in which case a new string can be started. | ||
| if (_enclosingContainer != currentSegmentEncoding && !CanWriteValue) | ||
| { | ||
| OnValidateWritingSegmentFailed(currentSegmentEncoding); | ||
| } | ||
| } | ||
|
|
||
| [DoesNotReturn] | ||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| private void OnValidateWritingSegmentFailed(EnclosingContainerType currentSegmentEncoding) | ||
| { | ||
| if (IsWritingPartialString) | ||
| { | ||
| ThrowHelper.ThrowInvalidOperationException_CannotMixEncodings(_enclosingContainer, currentSegmentEncoding); | ||
| } | ||
|
|
||
| // Make sure a new value is not attempted within an unfinalized string. | ||
| ValidateNotWithinUnfinalizedString(); | ||
| Debug.Assert(!HasPartialStringData); | ||
|
|
||
| if (_inObject) | ||
| if (_enclosingContainer == EnclosingContainerType.Object) | ||
| { | ||
| if (_tokenType != JsonTokenType.PropertyName) | ||
| { | ||
| Debug.Assert(_tokenType != JsonTokenType.None && _tokenType != JsonTokenType.StartArray); | ||
| ThrowHelper.ThrowInvalidOperationException(ExceptionResource.CannotWriteValueWithinObject, currentDepth: default, maxDepth: _options.MaxDepth, token: default, _tokenType); | ||
| } | ||
| Debug.Assert(_tokenType != JsonTokenType.PropertyName); | ||
| Debug.Assert(_tokenType != JsonTokenType.None && _tokenType != JsonTokenType.StartArray); | ||
| ThrowInvalidOperationException(ExceptionResource.CannotWriteValueWithinObject); | ||
| } | ||
| else | ||
| { | ||
| Debug.Assert(_tokenType != JsonTokenType.PropertyName); | ||
| Debug.Assert(CurrentDepth == 0 && _tokenType != JsonTokenType.None); | ||
| ThrowInvalidOperationException(ExceptionResource.CannotWriteValueAfterPrimitiveOrClose); | ||
| } | ||
| } | ||
|
|
||
| // It is more likely for CurrentDepth to not equal 0 when writing valid JSON, so check that first to rely on short-circuiting and return quickly. | ||
| if (CurrentDepth == 0 && _tokenType != JsonTokenType.None) | ||
| { | ||
| ThrowHelper.ThrowInvalidOperationException(ExceptionResource.CannotWriteValueAfterPrimitiveOrClose, currentDepth: default, maxDepth: _options.MaxDepth, token: default, _tokenType); | ||
| } | ||
| private void ValidateNotWithinUnfinalizedString() | ||
| { | ||
| if (IsWritingPartialString) | ||
| { | ||
| ThrowInvalidOperationException(ExceptionResource.CannotWriteWithinString); | ||
| } | ||
|
|
||
| Debug.Assert(!HasPartialStringData); | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
What are we peeking at in this case? It seems we're just returning a boolean so perhaps this could be expressed as a property, i.e.
IsInArrayor something?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.
It looks at the top of the stack assuming the stack is backed by an array.
IsInArraysounds like it's checking containment. I also like it being a method since it's symmetric with Push/PushToArray and Peek/PeekInArray. I'm open to better naming but I left it as is with more comments.