ODataUriSlim struct

[habbes]

Issues

*This pull request fixes #2163

Description

This PR introduces a new internal struct called ODataUriSlim to be used by ODataWriterCore instead of cloning ODataUri each time it enters a new scope. Now the scope stores a ODataUriSlim struct instead of a reference to the cloned ODataUri.

Making the type a struct avoids allocating instances to the heap. I have tried to keep the struct small by only storing properties that can be modified from one writer scope to another, namely the SelectAndExpand and Path properties. The other properties are never changed by the writer, and therefore do not need to be cloned.

To avoid excessive copies of the struct, I passed it by reference to all methods using the in keyword. Some copies were inevitable. For example, a copy will be made when assigning the struct value to a property of a class instance. Also, storing the struct in a class makes instance of that class larger because the memory of struct is embedded in the class, instead of a reference to a different memory location. Another case of inevitable copy is when retrieving the value from a property. You cannot pass property value by reference. For example, the following snippet will not compile:

SomeMethodThatAcceptsAStructByReference(in someObject.ODataUri); // someObject.ODataUri is a struct

In such cases I ended doing something like the following:

ODataUriSlim odataUri = someObject.ODataUri
SomeMethodThatAcceptsAStructByReference(in odataUri);

For ease of use, I have added a reference to the ODataUri to the ODataUriSlim and made the properties of ODataUri accessible through ODataUriSlim. Technically, this makes the ODataUriSlim slightly bigger by adding a reference, but this might prevent bugs. If these properties were not accessible through ODataUriSlim, then I would have had to pass ODataUri and ODataUriSlim to some methods which need to access the other properties. Using both ODataUri and ODataUriSlim can lead to bugs because someone can accidentally access the ODataUri.Path property, which would be different from the ODataUriSlim.Path property.

I have had to refactor a few other methods and classes that the writer passes the ODataUri to. In some cases I have changed the input or property type to ODataUriSlim? from ODataUri. In other cases I have created new method overloads that use ODataUriSlim as input instead of ODataUri. Luckily, there were not many places where I needed to do this. I avoided adding both ODataUri and ODataUriSlim properties to the same class because that would introduce a lot more complexity and maintainability issues than is necessary. I also avoided changing classes and methods beyond what the writer needs, because that's beyond the scope of this PR.

I also experimented with making ODataUriSlim a class instead of a struct (see: https://github.com/habbes/odata.net/blob/odataurislim-class-experiment/src/Microsoft.OData.Core/Uri/ODataUriSlim.cs). This would still lead to heap allocations, but the objects allocated would be smaller in size. This avoids the inevitable copies of the struct. We can also avoid code duplication by creating a common interface between ODataUriSlim and ODataUri, say IODataUri which we use as the argument for methods that can accept either type. We could have done the same with the struct, but passing a struct to a method that accepts an interface would lead to boxing, which implies heap allocations, which would have defeated my purpose for using a struct. ODataUriSlim class experiment ended up allocating slightly more memory than ODataUriSlim struct, but the benchmark results were pretty close.

Benchmark comparisons

According to the SerializationComparisonTests benchmarks in our repo, use of ODataUriSlim struct has resulted in an 11% reduction in allocated memory when using ODataMessageWriter synchronous API. When using the async API, the reduction was 7% when using the default JsonWriter and 10% when using ODataUtf8JsonWriter (more impact because it has a much smaller memory footprint compared to JsonWriter async). I did not observe significant improvement in speed, the figures in the time column fluctuated between different runs of the benchmarks, in some cases they were almost the same.

Full benchmark results

Baseline (before the PR)

SerializationComparisons benchmarks

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.20348
Intel Xeon E-2336 CPU 2.90GHz, 1 CPU, 12 logical and 6 physical cores
.NET SDK= 5.0.404 [C:\Program Files\dotnet\sdk]
[Host] : .NET 6.0.8 (6.0.822.36306), X64 RyuJIT

Toolchain=InProcessEmitToolchain InvocationCount=1 UnrollFactor=1

Method	WriterName	Mean	Error	StdDev	Gen 0	Gen 1	Allocated
WriteToFileAsync	ODataMessageWriter	294.143 ms	0.7563 ms	0.7075 ms	41000.0000	-	251,958 KB
WriteToFileAsync	ODataMessageWriter-Async	897.045 ms	2.4466 ms	2.1689 ms	66000.0000	1000.0000	403,831 KB
WriteToFileAsync	ODataMessageWriter-Utf8JsonWriter	276.190 ms	0.5155 ms	0.4822 ms	40000.0000	1000.0000	245,533 KB
WriteToFileAsync	ODataMessageWriter-Utf8JsonWriter-Async	466.277 ms	0.7230 ms	0.6409 ms	44000.0000	1000.0000	270,701 KB

ODataWriterFeedTests benchmarks

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.20348
Intel Xeon E-2336 CPU 2.90GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK= 5.0.404 [C:\Program Files\dotnet\sdk]

Toolchain=InProcessEmitToolchain InvocationCount=1 UnrollFactor=1

Method	isModelImmutable	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
WriteFeed	True	83.66 ms	0.533 ms	0.498 ms	9000.0000	-	-	57.68 MB
WriteFeedIncludeSpatial	True	263.05 ms	0.353 ms	0.313 ms	51000.0000	2000.0000	-	308.71 MB
WriteFeedWithExpansions	True	91.39 ms	0.062 ms	0.058 ms	10000.0000	-	-	63.11 MB
WriteFeedIncludeSpatialWithExpansions	True	109.92 ms	0.073 ms	0.068 ms	14000.0000	-	-	88.23 MB
WriteFeed_NoValidation	True	73.14 ms	0.073 ms	0.064 ms	8000.0000	-	-	53.47 MB
WriteFeedIncludeSpatial_NoValidation	True	244.18 ms	0.290 ms	0.271 ms	50000.0000	1000.0000	-	304.04 MB
WriteFeedWithExpansions_NoValidation	True	75.56 ms	0.060 ms	0.053 ms	8000.0000	-	-	52.61 MB
WriteFeedIncludeSpatialWithExpansions_NoValidation	True	94.08 ms	0.292 ms	0.273 ms	12000.0000	-	-	77.68 MB

With ODataUriSlim struct

SerializationComparisons benchmarks

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.20348
Intel Xeon E-2336 CPU 2.90GHz, 1 CPU, 12 logical and 6 physical cores
.NET SDK= 5.0.404 [C:\Program Files\dotnet\sdk]
[Host] : .NET 6.0.8 (6.0.822.36306), X64 RyuJIT

Toolchain=InProcessEmitToolchain InvocationCount=1 UnrollFactor=1

Method	WriterName	Mean	Error	StdDev	Median	Gen 0	Gen 1	Allocated
WriteToFileAsync	ODataMessageWriter	277.459 ms	0.6373 ms	0.5961 ms	277.459 ms	36000.0000	-	224,025 KB
WriteToFileAsync	ODataMessageWriter-Async	877.478 ms	4.2997 ms	3.8116 ms	877.849 ms	61000.0000	1000.0000	375,895 KB
WriteToFileAsync	ODataMessageWriter-Utf8JsonWriter	255.648 ms	0.2981 ms	0.2788 ms	255.745 ms	35000.0000	1000.0000	217,600 KB
WriteToFileAsync	ODataMessageWriter-Utf8JsonWriter-Async	470.307 ms	0.6589 ms	0.6163 ms	470.346 ms	39000.0000	1000.0000	242,768 KB

ODataWriterFeedTests benchmarks

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.20348
Intel Xeon E-2336 CPU 2.90GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK= 5.0.404 [C:\Program Files\dotnet\sdk]
[Host] : .NET Core 3.1.28 (CoreCLR 4.700.22.36202, CoreFX 4.700.22.36301), X64 RyuJIT

Toolchain=InProcessEmitToolchain InvocationCount=1 UnrollFactor=1

Method	isModelImmutable	Mean	Error	StdDev	Median	Gen 0	Gen 1	Gen 2	Allocated
WriteFeed	True	83.77 ms	0.142 ms	0.119 ms	83.77 ms	9000.0000	-	-	55.61 MB
WriteFeedIncludeSpatial	True	266.62 ms	0.362 ms	0.339 ms	266.68 ms	51000.0000	2000.0000	-	306.34 MB
WriteFeedWithExpansions	True	92.23 ms	0.109 ms	0.102 ms	92.19 ms	10000.0000	-	-	60.11 MB
WriteFeedIncludeSpatialWithExpansions	True	110.49 ms	0.078 ms	0.073 ms	110.50 ms	14000.0000	-	-	85.2 MB
WriteFeed_NoValidation	True	73.12 ms	0.315 ms	0.280 ms	73.13 ms	8000.0000	-	-	51.4 MB
WriteFeedIncludeSpatial_NoValidation	True	246.96 ms	0.387 ms	0.362 ms	246.92 ms	50000.0000	1000.0000	-	301.67 MB
WriteFeedWithExpansions_NoValidation	True	75.21 ms	0.312 ms	0.292 ms	75.01 ms	8000.0000	-	-	49.61 MB
WriteFeedIncludeSpatialWithExpansions_NoValidation	True	93.38 ms	0.108 ms	0.090 ms	93.36 ms	12000.0000	-	-	74.65 MB

With ODataUriSlim class

SerializationComparisons benchmarks

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.20348
Intel Xeon E-2336 CPU 2.90GHz, 1 CPU, 12 logical and 6 physical cores
.NET SDK= 5.0.404 [C:\Program Files\dotnet\sdk]
[Host] : .NET 6.0.8 (6.0.822.36306), X64 RyuJIT

Toolchain=InProcessEmitToolchain InvocationCount=1 UnrollFactor=1

Method	WriterName	Mean	Error	StdDev	Gen 0	Gen 1	Allocated
WriteToFileAsync	ODataMessageWriter	284.516 ms	0.7395 ms	0.6918 ms	37000.0000	-	227,321 KB
WriteToFileAsync	ODataMessageWriter-Async	890.175 ms	7.5653 ms	6.7064 ms	62000.0000	1000.0000	379,179 KB
WriteToFileAsync	ODataMessageWriter-Utf8JsonWriter	265.688 ms	0.4744 ms	0.4438 ms	36000.0000	1000.0000	220,905 KB
WriteToFileAsync	ODataMessageWriter-Utf8JsonWriter-Async	479.593 ms	0.2380 ms	0.1987 ms	40000.0000	1000.0000	246,073 KB

ODataWriterFeedTests benchmarks

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.20348
Intel Xeon E-2336 CPU 2.90GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK= 5.0.404 [C:\Program Files\dotnet\sdk]
[Host] : .NET Core 3.1.28 (CoreCLR 4.700.22.36202, CoreFX 4.700.22.36301), X64 RyuJIT

Toolchain=InProcessEmitToolchain InvocationCount=1 UnrollFactor=1

Method	isModelImmutable	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
WriteFeed	True	84.19 ms	0.425 ms	0.398 ms	9000.0000	-	-	55.89 MB
WriteFeedIncludeSpatial	True	264.23 ms	0.259 ms	0.229 ms	51000.0000	2000.0000	-	306.65 MB
WriteFeedWithExpansions	True	92.59 ms	0.116 ms	0.097 ms	10000.0000	-	-	60.51 MB
WriteFeedIncludeSpatialWithExpansions	True	110.50 ms	0.105 ms	0.093 ms	14000.0000	-	-	85.6 MB
WriteFeed_NoValidation	True	72.41 ms	0.192 ms	0.180 ms	8000.0000	-	-	51.68 MB
WriteFeedIncludeSpatial_NoValidation	True	243.16 ms	0.200 ms	0.178 ms	50000.0000	1000.0000	-	301.98 MB
WriteFeedWithExpansions_NoValidation	True	75.64 ms	0.075 ms	0.067 ms	8000.0000	-	-	50.01 MB
WriteFeedIncludeSpatialWithExpansions_NoValidation	True	93.07 ms	0.129 ms	0.107 ms	12000.0000	-	-	75.05 MB

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.

[gathogojr]

Maybe throw an ArgumentNullException?

[habbes]

I'm a bit conflicted since this adds a null check for something that is called often but should never be null unless we have an internal bug (not a user-input error).

[pull-request-quantifier-deprecated]

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

---

Quantification details

Label      : Large
Size       : +160 -64
Percentile : 62.4%

Total files changed: 13

Change summary by file extension:
.cs : +160 -64

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 detected.
• 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.

[gathogojr]