Avoid creating unused objects when calling ODataUri.Clone()

[habbes]

Issues

This pull request (partially) addresses #2163 .

Description

The fix

ODataUri.Clone() creates a new instance of ODataUri using new ODataUri() then shallow copies the properties (references) from the "old" ODataUri to the new one. However, the default ODataUri constructor creates a new instance of Dictionary<string, string> and a new instance of ParameterAliasValueAccessor. The ODataUri.ParameterAliasValueaAccessor property is immediately replaced by ODataUri.Clone() with the reference from the old uri. So the instances created in the default constructor are a waste of memory and unnecessary allocation/GC overhead.

I fixed this issue by creating the ParameterAliasValueAccessor instance lazily in the property getter. I thought this to be the safest solution, below are other solutions that I considered:

• Removing the new instances from the constructor without modifying the property getter
  • I cannot guarantee that this would not lead to null reference exceptions in cases where the default constructor is used outside of Clone(), and I did not have time to investigate all those usage scenarios
• Using the overloaded constructor that accepts more arguments (including ParameterAliasValueAccessor) that are then directly passed to properties. This constructor seems close to what ODataUri.Clone is doing, except that when setting the CustomQueryOptions property: this.CustomQueryOptions = new ReadOnlyCollection<QueryNode>(customQueryOptions.ToList());, where as in ODataUri.Clone() the property reference is simply copied without creating any new object. So this would not be a suitable substitute.
• Creating a new constructor that does not create new instances. This felt unnecessary.

Expected impact

While this change does not reduce the number of ODataUri instances allocated (which is what the original issue pointed to in the screenshot), it does significantly reduce the number of Dictionary instances created as a result of ODataUri.Clone().

This CPR screenshot shows that Dictionary<string, string> created from ODataUri.Clone() account for and 1.3% (0.69 + 0.61) of allocated size:

The following screenshot shows Dictionary<string, SingleValueNode> from ODataUri.Clone() account for 0.65%.

The following screenshot shows ParameterAliasValueAccessor from ODataUri.Clone() accounts for 0.23%

So in total, there's up to 2.18% that we can potentially shed off. This is based on the assumptions that the sizes indicated in the screenshots do not include sizes of referenced objects (otherwise ParameterAliasValueAccessor would have a larger inclusive size than the dictionaries it contains).

I've done some local profiling before and after the fix to get a better estimate of how much we can expect to reduce. The profiling were based on running a simple service that uses that OData writer to write a response of 5000 entities (base on this experments project) and collecting allocation data using the .NET Object Allocation Tracker tool that's part of the Visual Studio Performance Profiler toolset.

The following estimates may not hold true in production given the difference in usage patterns and data, but it may be a good idea to compare the results once we measure them in production with the estimates here later on. I think that will help us make better estimates over time.

Impact on number of allocations

The first 2 screenshots below show the total allocations from ODataUri.Clone() from my local profile before and after this change. You can see total allocations dropped from 175k to 35k, i.e. about 140k drop in allocations, which is about 80%. ODataUri.Clone() accounts in total for 4.13% of allocations (inclusive), 80% drop would should shed off about 3.3% of allocations in AGS assuming the experiment data is a reflection of what happens in production.

Impact on allocation size

Impact on allocation size is trickier to estimate cause the .NET allocation tracker in VS doesn't display the inclusive allocation size for a given type. I'm also not sure how the inclusive size is computed in CPR (on the stack tab, both inclusive and exclusive columns have the same value), on the Frames tab, they have different values, but this tab doesn't show the breakdown of the types allocated by a function.

That said, I thin we can realistically estimate that at least all Dictionary<string, string>, Dictionary<string, SingleValueNode> and ParameterAliasValueAccessor allocated from ODataUri.Clone() will be eliminated, and I've already calculated that above to be 2.18% of total allocated bytes.

Below are some screenshots from my local profile showing the allocated size of Dictionary instances before and after the change.

Impact on collection

On my local profile, there were 3 fewer GC collections after the change. But I'm not sure if this is reliable.

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.

[pull-request-quantifier-deprecated]

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

---

Quantification details

Label      : Extra Small
Size       : +9 -3
Percentile : 4.8%

Total files changed: 1

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

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.

[joaocpaiva]

Fix loos good to me. Lazy initialization in the getter minimizes the changes and given this class is not meant to be used concurrently it works just fine. It also improves any scenarios where ODataUri is created, but ParameterAliasValueAccessor is never used.

[joaocpaiva]

Line 130, ParameterAliasNodes has a null check on the property. That null check does not seem required since it will always evaluate true? Or if it is, it should be replaced with the field, instead of the property?

[joaocpaiva]

This is for the ParameterAliasNodes getter - it has a null check for ParameterAliasValueAccessor which is redundant in the current iteration, since ParameterAliasValueAccessor is never null. Hence should check against the field instead of the property, or perhaps remove the null check.

[Sreejithpin]

Moving initialization from constructor to getter looks fine . Can you see how much % improvement we got . Let me know if you want me to run a local benchmark test

[chrisspre]

Moving initialization from constructor to getter looks fine . Can you see how much % improvement we got . Let me know if you want me to run a local benchmark test

If we do this kind of work more often I don't think it is good to wait for deployment and measurement via Graph. I would recommend writing benchmark tests (one sdk should be sufficient) and compare the before and after. graph measurements are a moving target and it takes a long time to collect that information.

[g2mula]

This is a change of behaviour - could be for the better, but still a change :-) - and there probably should be a test to expect this current behaviour...

• Previously: value is set in ctor, you could then be able to set a null to that value and a subsequent get would give null.
• Currently: even if you ever set a null value, you would never get it
  It could be that current behaviour is fixing unwanted null gets but yeah, probably a test to document this...

[habbes]

Hmm, good point

[chrisspre]

Would it be possible to change the setter from internal to private ?

[joaocpaiva]

Even the getter, does not seem it needs to be public/internal as of now.

[mikepizzo]

Yes; the setter is currently internal and only called when initialized to a non-null value in the constructor, so we should be able to assume it is never null. We can change the setter (and getter) to private and change the null check on 128-133 with an non-null assert.

[mikepizzo]