Skip to content
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

Merged
merged 12 commits into from
May 24, 2022

Conversation

eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented May 19, 2022

This 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

Method Branch Mean Error StdDev Median Min Max Ratio MannWhitney(5%) RatioSD Gen 0 Allocated Alloc Ratio
DeserializeFromUtf8Bytes main 200.1 ns 9.58 ns 11.03 ns 193.7 ns 190.5 ns 221.4 ns 1.00 Base 0.00 0.0128 136 B 1.00
DeserializeFromUtf8Bytes PR 172.9 ns 1.14 ns 0.95 ns 172.9 ns 171.6 ns 175.2 ns 0.86 Faster 0.05 - - 0.00

Utf8JsonReader.GetString() call on escaped string

Method Branch Mean Error StdDev Median Min Max Ratio MannWhitney(5%) Gen 0 Allocated Alloc Ratio
GetString main 10.643 us 0.1048 us 0.0929 us 10.627 us 10.527 us 10.848 us 1.00 Base 1.1797 11.72 KB 1.00
GetString PR 7.678 us 0.0615 us 0.0546 us 7.658 us 7.615 us 7.765 us 0.72 Faster 1.1651 11.72 KB 1.00

cc @davidfowl @bartonjs @steveharter @stephentoub

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

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.

@ghost ghost assigned eiriktsarpalis May 19, 2022
@ghost
Copy link

ghost commented May 19, 2022

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

This 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

Method Branch Mean Error StdDev Median Min Max Ratio MannWhitney(5%) RatioSD Gen 0 Allocated Alloc Ratio
DeserializeFromUtf8Bytes main 200.1 ns 9.58 ns 11.03 ns 193.7 ns 190.5 ns 221.4 ns 1.00 Base 0.00 0.0128 136 B 1.00
DeserializeFromUtf8Bytes PR 172.9 ns 1.14 ns 0.95 ns 172.9 ns 171.6 ns 175.2 ns 0.86 Faster 0.05 - - 0.00

Utf8JsonReader.GetString() call on escaped string

Method Branch Mean Error StdDev Median Min Max Ratio MannWhitney(5%) Gen 0 Allocated Alloc Ratio
GetString main 10.643 us 0.1048 us 0.0929 us 10.627 us 10.527 us 10.848 us 1.00 Base 1.1797 11.72 KB 1.00
GetString PR 7.678 us 0.0615 us 0.0546 us 7.658 us 7.615 us 7.765 us 0.72 Faster 1.1651 11.72 KB 1.00
Author: eiriktsarpalis
Assignees: -
Labels:

area-System.Text.Json, new-api-needs-documentation

Milestone: -

@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone May 19, 2022
…8JsonReader.TryGet.cs

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
return true;

DestinationTooShort:
return false;
Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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

@eiriktsarpalis eiriktsarpalis merged commit 02ad981 into dotnet:main May 24, 2022
@eiriktsarpalis eiriktsarpalis deleted the utf8jsonreader-copystring branch May 24, 2022 15:43
@ghost ghost locked as resolved and limited conversation to collaborators Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants