Implement asynchronous support in BufferingJsonReader #2264

gathogojr · 2021-11-25T05:54:36Z

Issues

This pull request partially fulfills #2019.

Description

Implement asynchronous support in BufferingJsonReader

Checklist (Uncheck if it is not completed)

Test cases added
Build and test with one-click build and test script passed

Additional work necessary

If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.

src/Microsoft.OData.Core/Json/BufferingJsonReader.cs

marabooy · 2021-11-25T13:52:09Z

src/Microsoft.OData.Core/Json/BufferingJsonReader.cs

+                bool result = await this.ReadInternalAsync()
+                    .ConfigureAwait(false);
+
+                if (this.asyncInnerReader.NodeType == JsonNodeType.StartObject)


Maybe invert this to have the nesting level reduced.

marabooy · 2021-11-25T13:55:20Z

src/Microsoft.OData.Core/Json/BufferingJsonReader.cs

+                            .ConfigureAwait(false);
+                        if (!readErrorStringPropertyResult.Item1)
+                        {
+                            return Tuple.Create(false, error);


Not sure if nullable would be better for these cases?

@marabooy To explain the logic, the only time this method returns the second component of the tuple as null is before entering this while loop. Within this loop, we return the error object with as many properties we have read before coming across some unintelligible property. For example, if successfully read code, message, target, and innererror properties and the come across details and we're unhappy, we don't return a tuple with false as first component and null as second component. Instead, the second component will contain the error object with the properties we had read successfully populated.

habbes · 2021-11-30T04:29:45Z

src/Microsoft.OData.Core/Json/BufferingJsonReader.cs

+                if (this.isBuffering)
+                {
+                    Debug.Assert(this.currentBufferedNode != null, "this.currentBufferedNode != null");
+                    return Task.FromResult(this.currentBufferedNode.Value);


Out of curiosity, you opted not to use ValueTask in this case because this is a public method or because the synchronous code-path is less frequent than the async one?

src/Microsoft.OData.Core/Json/BufferingJsonReader.cs

habbes · 2021-11-30T04:35:52Z

src/Microsoft.OData.Core/Json/BufferingJsonReader.cs

+
+            if (value == null)
+            {
+                result = new MemoryStream(new byte[0]);


I think we can use Array.Empty<byte>() to avoid allocating an empty array each time. Consider using it in the sync method as well.

@habbes Array.Empty<byte>() is not available in net45 and netstandard1.1. I don't know if it'd be worthwhile to do this:

#if NETSTANDARD2_0 result = new MemoryStream(Array.Empty<byte>()); #else result = new MemoryStream(new byte[0]); #endif

@gathogojr this is an issue that is causing too many warnings. Will probably fix it in 8.x where we remove support for net45

src/Microsoft.OData.Core/Json/BufferingJsonReader.cs

habbes · 2021-11-30T04:41:40Z

src/Microsoft.OData.Core/Json/BufferingJsonReader.cs

+            if (this.bufferedNodesHead == null)
+            {
+                // capture the current state of the reader as the first item in the buffer (if there are none)
+                object value = await this.asyncInnerReader.GetValueAsync()


Seems a lot of these methods are async because of getting the value. I wonder if we could avoid some of the overhead when the value is immediately available. But that's probably a discussion for another day.

src/Microsoft.OData.Core/Json/BufferingJsonReader.cs

corranrogue9 · 2021-11-30T18:27:14Z

src/Microsoft.OData.Core/Json/BufferingJsonReader.cs

+        {
+            this.AssertAsynchronous();
+
+            if (!this.isBuffering && this.asyncInnerReader is IJsonStreamReaderAsync asyncStreamReader)


I see this check for IJsonStreamReaderAsync in a few different places. I wonder if it makes sense to have a field where this is stored so that the casting only has to happen once. I also wonder if that field should be set in the constructor. I can see there being different use cases where it makes sense to do it on the fly versus doing it in the constructor. Any thoughts?

corranrogue9 · 2021-11-30T18:28:37Z

src/Microsoft.OData.Core/Json/BufferingJsonReader.cs

+
+            object value = await this.GetValueAsync()
+                .ConfigureAwait(false);
+            return value is string || value == null || this.NodeType == JsonNodeType.StartArray || this.NodeType == JsonNodeType.StartObject;


I don't think the subsequent conditions are predicated on this first one for any reason, so I'm wondering if it makes sense to do the cast last due to short circuit evaluation

@corranrogue9 Does a cast actually happen here given that value is already boxed and string is a reference type? In addition, supposing StartArray and StartObject occur rarely, do we not end up almost always doing the "cheap" comparisons as well as evaluating value is string expression?

pull-request-quantifier-deprecated · 2021-12-01T11:39:25Z

This PR has 679 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Extra Large
Size       : +670 -9
Percentile : 89.3%

Total files changed: 5

Change summary by file extension:
.cs : +670 -9

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detetcted.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

KenitoInc · 2021-12-02T11:53:24Z

src/Microsoft.OData.Core/Json/BufferingJsonReader.cs

+
+            if (value == null)
+            {
+                result = new MemoryStream(new byte[0]);


@gathogojr this is an issue that is causing too many warnings. Will probably fix it in 8.x where we remove support for net45

gathogojr requested review from marabooy, KenitoInc, habbes, xuzhg, corranrogue9, ElizabethOkerio and Sreejithpin November 25, 2021 05:54

pull-request-quantifier-deprecated bot added the Extra Large label Nov 25, 2021

habbes reviewed Nov 25, 2021

View reviewed changes

src/Microsoft.OData.Core/Json/BufferingJsonReader.cs Outdated Show resolved Hide resolved

marabooy reviewed Nov 25, 2021

View reviewed changes

habbes reviewed Nov 30, 2021

View reviewed changes

gathogojr force-pushed the feature/bufferingjsonreader-async branch 2 times, most recently from 196deee to 19cc4d6 Compare November 30, 2021 04:46

habbes requested changes Nov 30, 2021

View reviewed changes

corranrogue9 reviewed Nov 30, 2021

View reviewed changes

gathogojr force-pushed the feature/bufferingjsonreader-async branch 2 times, most recently from 7217b6c to f6e08da Compare November 30, 2021 18:40

habbes approved these changes Dec 1, 2021

View reviewed changes

gathogojr force-pushed the feature/bufferingjsonreader-async branch 2 times, most recently from 428f491 to 00f1b67 Compare December 1, 2021 10:07

Implement asynchronous support in BufferingJsonReader

ec90ede

gathogojr force-pushed the feature/bufferingjsonreader-async branch from 00f1b67 to ec90ede Compare December 1, 2021 11:39

marabooy approved these changes Dec 1, 2021

View reviewed changes

KenitoInc approved these changes Dec 2, 2021

View reviewed changes

gathogojr merged commit d643a05 into OData:master Dec 3, 2021

gathogojr deleted the feature/bufferingjsonreader-async branch December 3, 2021 09:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement asynchronous support in BufferingJsonReader #2264

Implement asynchronous support in BufferingJsonReader #2264

gathogojr commented Nov 25, 2021

marabooy Nov 25, 2021

marabooy Nov 25, 2021

gathogojr Dec 1, 2021

habbes Nov 30, 2021

habbes Nov 30, 2021

gathogojr Nov 30, 2021

KenitoInc Dec 2, 2021

habbes Nov 30, 2021

corranrogue9 Nov 30, 2021

corranrogue9 Nov 30, 2021

gathogojr Dec 1, 2021 •

edited

Loading

pull-request-quantifier-deprecated bot commented Dec 1, 2021

What can I do to optimize my changes

How to interpret the change counts in git diff output

KenitoInc Dec 2, 2021

Implement asynchronous support in BufferingJsonReader #2264

Implement asynchronous support in BufferingJsonReader #2264

Conversation

gathogojr commented Nov 25, 2021

Issues

Description

Checklist (Uncheck if it is not completed)

Additional work necessary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gathogojr Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

pull-request-quantifier-deprecated bot commented Dec 1, 2021

What can I do to optimize my changes

How to interpret the change counts in git diff output

Choose a reason for hiding this comment

gathogojr Dec 1, 2021 •

edited

Loading