Skip to content
This repository has been archived by the owner on Aug 2, 2023. It is now read-only.

Update UTF-8 JsonWriter APIs based on previous API feedback #2612

Merged
merged 21 commits into from Dec 10, 2018

Conversation

ahsonkhan
Copy link
Member

@ahsonkhan ahsonkhan commented Dec 7, 2018

Addressing most of dotnet/apireviews#80 + other changes.

API proposal updated: https://github.com/dotnet/corefx/issues/33552

TODO:

  • Finish implementation of API skeletons.
  • Add more tests (especially exceptions and edge cases) - outside this PR.
  • Add BytesWritten support.
  • Figure out how to deal with escaping (either implement from scratch or leverage exising APIs) - cc @GrabYourPitchforks - outside this PR.
  • Fixup and remove the TODOs left in code. - outside this PR.
  • Add state-passing and write sample/test for async support.
  • Re-evaluate size of JsonWriter and maybe reduce it by packing more info in available bits/custom struct layout. - outside this PR.
  • XML comments/documentation - outside this PR.
  • Consider making the type non-generic with a ctor overload that accepts span (or memory) - outside this PR.

cc @steveharter, @stephentoub, @bartonjs, @KrzysztofCwalina


namespace System.Text.JsonLab
{
public struct BitStack
Copy link
Member Author

Choose a reason for hiding this comment

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

Ignore this. It is a copy of the internal API from https://github.com/dotnet/corefx/blob/master/src/System.Text.Json/src/System/Text/Json/BitStack.cs which I just copied for convenience. This will go away once the code moves to corefx.

public const int MaximumUInt64Length = 20; // i.e. 18446744073709551615
public const int MaximumDoubleLength = 32; // default (i.e. 'G') TODO: Should it be 22?
public const int MaximumSingleLength = 32; // default (i.e. 'G') TODO: Should it be 13?
public const int MaximumDecimalLength = 32; // default (i.e. 'G') TODO: Should it be 31?
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.

The NumberBufferLength values are large enough to represent the exact string representation of any given input plus one for a rounding digit and one for the null-terminating character.

For example, the input with the longest exact string for a double value is double.Epsilon, whose exact representation has 767 significant digits (there are an additional 307 leading zeros, giving 1074 digits total; but we don't need to track leading or trailing zeros). If a user gives us an input string that contains all 1074 digits of this value, we need to be able to parse that. If they provide additional digits, we also have to consider them to determine which direction we round (do we round down to double.Epsilon or do we round up to the next representable value above double.Epsilon). If the 768th digit is a 5, then we may be in "midpoint-rounding" and we have to continue processing the rest of the string until we find a non-zero digit to determine if this is actually midpoint (the rest of the digits in the input were 0, in which case we round up or down to the value with the first bit set) or if we are slightly above (one or more of the digits after the 768th were non-zero) in which case we round up.


public static OperationStatus EscapeString(ReadOnlySpan<char> value, Span<byte> destination, out int consumed, out int bytesWritten)
{
// // TODO:
Copy link
Member Author

Choose a reason for hiding this comment

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

Intentionally commented out for now.

{
ValidateWritingPropertyWithEncoding(propertyName);
}
WriteStringFormattedWithEncoding(propertyName, value)
Copy link
Member Author

Choose a reason for hiding this comment

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

Any suggestions on reducing code duplication here (or in general)? The code for Guid, DateTime, and DateTimeOffset is essentially identical (all calling into the Utf8Formatter).

if (values.Length > 0 && values.Length < (int.MaxValue - 2) / (JsonConstants.MaximumInt64Length + 1))
{
// Calculated based on the following: '[number0,number1,...,numberN]'
int bytesNeeded = 2 + values.Length * (1 + JsonConstants.MaximumInt64Length);
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a concern with pessimistically asking IBufferWriter<byte> for more bytes than needed (i.e. get the bytes needed for the worst case)?

Copy link
Member Author

@ahsonkhan ahsonkhan Dec 10, 2018

Choose a reason for hiding this comment

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

cc @davidfowl, @pakrym - any thoughts on the approach of over-asking for the amount of bytes from IBufferWriter<byte> for the worst case? For instance, if we are writing a 4, single digit integers as a JSON array, I ask for 4*20 + 3 + 2 =85 bytes rather than 4 + 3+ 2 = 9 (for example: [1, 2, 3, 4]). This way I don't have to count the digits of each element (at most it would be 20 digits, for an Int64).

Is there a scenario where IBufferWriter has only enough space to fit the actual payload, exactly, and asking for more is incorrect, even if we only use what was actually needed?

Copy link
Contributor

@pakrym pakrym Dec 10, 2018

Choose a reason for hiding this comment

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

Over-asking a little bit is fine and especially considering that IBufferWriter is free to give you less than requested if it doesn't have enough space.

@ahsonkhan ahsonkhan changed the title [WIP] Update UTF-8 JsonWriter APIs based on previous API feedback Update UTF-8 JsonWriter APIs based on previous API feedback Dec 10, 2018
@ahsonkhan ahsonkhan merged commit 5de823f into dotnet:master Dec 10, 2018
@ahsonkhan ahsonkhan deleted the UpdateJsonWriterAPIs branch December 10, 2018 11:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants