-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 CopyString and ValueIsEscaped APIs to Utf8JsonReader #69580
Add CopyString and ValueIsEscaped APIs to Utf8JsonReader #69580
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsThis PR
The changes improve performance in a few applications, most notably when reading escaped strings or when deserializing converters that would until now allocate strings. String-based enum deserialization without escaping
Utf8JsonReader.GetString() call on escaped string
|
src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.Unescaping.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.Unescaping.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.cs
Show resolved
Hide resolved
…cument/JsonDocument.cs" This reverts commit 5ba7ab5.
src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.Unescaping.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs
Outdated
Show resolved
Hide resolved
…8JsonReader.TryGet.cs Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.Unescaping.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.Unescaping.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.Unescaping.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.Unescaping.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.Unescaping.cs
Show resolved
Hide resolved
return true; | ||
|
||
DestinationTooShort: | ||
return false; |
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.
why not just return false
directly? do we need to use goto DestinationTooShort
? False return means DestinationTooShort
anyway. If you think we might change that in the future perhaps consider:
const bool DestinationTooShort = false;
and return DestinationTooShort
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's a micro-optimization. We sometimes do this in hot path methods to minimize the number of instructions being emitted:
It might not be super critical for the unescaper though.
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.
I'd suggest filing issue on improving this rather than making code harder to read
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's fairly pervasive in the hot code paths in both STJ and STEW, so we can look at it later.
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.
We should at least open the issue to track this work
This PR
Utf8JsonReader.ValueIsEscaped
property. Fix AddUtf8JsonReader.ValueIsEscaped
property #45167.UtfJsonReader.CopyString
methods. Fix Support non-allocating view on string properties values of JSON in System.Text.Json.Utf8JsonReader #54410.Unescape
implementation to better take advantage of theSpan.IndexOf
andCopyTo
methods, resulting in improved performance for certain scenaria because of vectorization.The changes improve performance in a few applications, most notably when reading escaped strings or when deserializing converters that would until now allocate strings.
String-based enum deserialization without escaping
Utf8JsonReader.GetString() call on escaped string
cc @davidfowl @bartonjs @steveharter @stephentoub