This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add Utf8JsonWriter along with unit tests #34425
Add Utf8JsonWriter along with unit tests #34425
Changes from 10 commits
aeb2103
76a459b
f1eae6f
0e1a283
fd8bd6d
39fc64d
41b7b2c
7988444
8276f4c
9bec478
5385574
a9f99be
65b38cb
8d62b8a
c4fc141
534ebf2
424835a
1c09b09
a91c702
f1cdc28
baa5ac2
644118c
baa4931
2b91e6c
6c43954
eb33db6
171f6c3
a14a57e
712502b
3bc364c
56382db
2900bc5
6f41164
c3c83e4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Are these commonly accessed on the writer? Wondering when you'd use these rather than getting the CurrentState and using its BytesCommitted/Written.
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 can see them being useful if the user is doing synchronous writing and wants to get how much has been written so far. For that pattern, the user never really has to deal with state, so forcing them to call state to get meaningful data that the writer already tracks seems unnecessary.
For example, @KrzysztofCwalina needs it for his scenario: https://github.com/Azure/azure-sdk-for-net-lab/blob/4d8369fe96e99514c108dacb2cd39bc589b0042a/Configuration/Azure.Configuration/ConfigurationSettingParser.cs#L30
With the change from #34425 (comment), BytesCommitted == BytesWritten when you access them on the CurrentState (since it forces you to flush). If the user wants to know the difference between how much has been written versus committed so far (without flushing). they would need the properties on Utf8JsonWriter.
We could remove one of BytesCommitted or BytesWritten from the CurrentState 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.
Isn't the double negative of supressEscaping a bit strange? I know we generally prefer default arguments to be false rather than true, but it seems like this would be better as
bool escape = true
orbool escapeIfNecessary = true
or some such thing.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.
If we are OK with making an exception here, I am fine with calling it
escape
instead and avoid the double negative. How do others feel about the change? I want to avoid going back and forth.cc @terrajobst
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.
Given we already have
isFinalBlock = true
as the default argument in Flush(), I am going to make this change as well.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.
This API design with a default argument value has same performance bug as the default format argument for integer formatters. This argument has to be always passed in (makes every callsite bigger) and then checked in the implementation.
I think these Write APIs should be split into 2 categories:
The APIs for the core types: strings and primitive numbers. These APIs should have the full variety of overloads to make them as lean and fast as possible in all cases. String and Span overloads, as well as overloads with the suppress-escaping argument.
The APIs for the fringe types: Guid, DateTimes, decimal, etc. These APIs should have just the single most general overload that takes Spans and bool suppressEscaping with default value.
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 considered splitting the APIs and removing optional arguments.
However, in my benchmarks, I didn't notice the same performance regression as
Utf8Formatter
(based on the current implementation). Having an API without the optional bool did not show any significant improvement.Also, the common/high performance scenario is one where
suppressEscaping
is true. I would assume the APIs that don't acceptsuppressEscaping
will behave the same as ifsuppressEscaping
is false (i.e. they will do the escaping check). So, it won't really help the common case.