Create optimized ODataPath cloning constructor

[habbes]

Issues

*This pull request fixes #2488

Description

Creates a new constructor that takes ODataPath as input and uses odataPath.segments as input to copy the list of segments to the new ODataPath. Since we're passing a List to a List, it uses a faster copy using Array.Copy instead of iterating through the IEnumerable to add items to the list one at a time, which is less memory and CPU efficient. See List constructor and InsertRange implementations.

I made the new constructor internal to be on the safe side. To me it appears safe enough to be public. But, If I make it public, a different method overload will get called without the user having to opt-in to it or without the user having to change arguments. I'm not sure whether that's desirable or not.

On the following benchmarks, it sheds off about 2MB of allocations. About 0.8% reduction. Not that significant here. But I expect it will reduce around 0.3-0.4% in AGS based on how much these allocations already contribute in AGS (>.6%). I think that's not bad for such a small change.

Before

Method	WriterName	Mean	Error	StdDev	Gen 0	Gen 1	Allocated
WriteToFileAsync	ODataMessageWriter	289.481 ms	1.1232 ms	1.0507 ms	41000.0000	-	254,692 KB
WriteToFileAsync	ODataMessageWriter-Async	923.776 ms	3.8008 ms	3.3693 ms	66000.0000	1000.0000	406,539 KB
WriteToFileAsync	ODataMessageWriter-Utf8JsonWriter	273.308 ms	0.5061 ms	0.4734 ms	40000.0000	1000.0000	248,268 KB
WriteToFileAsync	ODataMessageWriter-Utf8JsonWriter-Async	494.572 ms	0.4876 ms	0.4561 ms	44000.0000	1000.0000	273,436 KB

After

Method	WriterName	Mean	Error	StdDev	Gen 0	Gen 1	Allocated
WriteToFileAsync	ODataMessageWriter	287.736 ms	0.4565 ms	0.4270 ms	41000.0000	-	252,739 KB
WriteToFileAsync	ODataMessageWriter-Async	896.619 ms	9.6695 ms	9.0448 ms	66000.0000	1000.0000	404,596 KB
WriteToFileAsync	ODataMessageWriter-Utf8JsonWriter	268.738 ms	0.3572 ms	0.3341 ms	40000.0000	1000.0000	246,315 KB
WriteToFileAsync	ODataMessageWriter-Utf8JsonWriter-Async	477.963 ms	0.6127 ms	0.5731 ms	44000.0000	1000.0000	271,482 KB

This approach eliminates array resizes in the ODataPath constructor. Most of the time the path when we're about to add a new segment. To avoid resizing the list when the new segment is added, I also create the new list with enough room for the new segment. I tried a number of alternatives to evaluate the trade-offs between avoiding resizing and pre-allocation/copying arrays that are too big. I evaluated the different options against the following benchmarks and picked the one that seemed to have the best balance

• Cloning a long path with many segments
• Adding a segment to short path (this implies cloning since cloned before segments are added)
• Adding a segment to a long path
• Adding a few segments (4 segments) to a short path
• Adding many segments to a short path (10 segments)

Here are the results:

1. Baseline: Existing implementation

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
CloneShortPath	73.50 ns	0.368 ns	0.307 ns	0.0242	-	-	152 B
CloneLongPath	251.93 ns	0.286 ns	0.267 ns	0.0625	-	-	392 B
AddSegmentShortPath	122.39 ns	1.325 ns	1.239 ns	0.0331	-	-	208 B
AddSegmentLongPath	301.54 ns	0.211 ns	0.187 ns	0.0710	-	-	448 B
AppendFewSegments	863.19 ns	1.161 ns	1.086 ns	0.2213	-	-	1392 B
AppendManySegments	2,414.98 ns	2.244 ns	1.990 ns	0.6485	-	-	4080 B

2. Create a new List with capacity equal to the length of the existing list

public ODataPath(ODataPath odataPath)
{
    this.segments = new List<ODataPathSegments>(odataPath.segments);
}

The new list only has enough capacity for the current segments. This avoids memory waste if we don't add new segments. But it always leads to a resize when the next segment is added.

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
CloneShortPath	21.17 ns	0.187 ns	0.175 ns	0.0166	-	-	104 B
CloneLongPath	27.70 ns	0.118 ns	0.104 ns	0.0319	-	-	200 B
AddSegmentShortPath	78.71 ns	0.039 ns	0.037 ns	0.0370	-	-	232 B
AddSegmentLongPath	99.99 ns	0.182 ns	0.152 ns	0.0829	0.0001	-	520 B
AppendFewSegments	439.69 ns	0.351 ns	0.328 ns	0.2232	0.0005	-	1400 B
AppendManySegments	1,004.09 ns	6.451 ns	5.387 ns	0.6161	0.0019	-	3872 B

3. Set the capacity of the new List to the capacity of the existing List

public ODataPath(ODataPath odataPath)
{
   this.segments = new List<ODataPathSegments>(odataPath.segments.Capacity);
   this.segments.AddRange(odataPath.segments);
}

This is a slight optimization from the previous case. It avoids resizes by relying on the assumption that in most cases the capacity will be greater than the number of segments in the list. However, if the input is already at capacity, the next segment will force a resize.

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
CloneShortPath	23.79 ns	0.068 ns	0.064 ns	0.0166	-	-	104 B
CloneLongPath	32.05 ns	0.033 ns	0.030 ns	0.0318	-	-	200 B
AddSegmentShortPath	80.59 ns	0.050 ns	0.042 ns	0.0370	-	-	232 B
AddSegmentLongPath	98.98 ns	0.206 ns	0.193 ns	0.0829	-	-	520 B
AppendFewSegments	381.81 ns	0.153 ns	0.128 ns	0.1807	0.0005	-	1136 B
AppendManySegments	833.21 ns	1.897 ns	1.774 ns	0.4520	0.0019	-	2840 B

4. Set the capacity to one more than the size of the list: THIS PR

public ODataPath(ODataPath odataPath)
{
   this.segments = new List<ODataPathSegment>(odataPath.segments.Count + 1);
   this.segments.AddRange(odataPath.segments);
}

This creates a new List with enough room to store one more element. This ensures the next segment after the clone will not resize the array. Of course we allocate more memory than needed if we don't resize. But all but the 2nd alternative have that issue.

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
CloneShortPath	24.19 ns	0.044 ns	0.041 ns	0.0179	-	-	112 B
CloneLongPath	30.34 ns	0.169 ns	0.158 ns	0.0331	-	-	208 B
AddSegmentShortPath	64.77 ns	0.055 ns	0.046 ns	0.0267	-	-	168 B
AddSegmentLongPath	72.42 ns	0.051 ns	0.045 ns	0.0421	-	-	264 B
AppendFewSegments	339.62 ns	0.210 ns	0.186 ns	0.1464	-	-	920 B
AppendManySegments	763.95 ns	1.093 ns	0.969 ns	0.3643	0.0019	-	2288 B

5. Set the capacity to 4 more than the size of the list

public ODataPath(ODataPath odataPath)
{
   int capacity = odataPath.segments.Count < odataPath.segments.Capacity ? 4 : odataPath.segments.Capacity + 4;
   this.segments = new List<ODataPathSegments>(capacity);
   this.segments.AddRange(odataPath.segments);
}

This is a slight variation from the previous one suggested by @gathogojr. We wanted to see whether creating more room will reduce the number of resizes if we have many consecutive new segments. However, it performs worse than the previous one on the benchmarks:

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
CloneShortPath	25.66 ns	0.050 ns	0.042 ns	0.0217	-	-	136 B
CloneLongPath	31.86 ns	0.093 ns	0.087 ns	0.0370	-	-	232 B
AddSegmentShortPath	65.33 ns	0.051 ns	0.048 ns	0.0305	-	-	192 B
AddSegmentLongPath	75.15 ns	0.042 ns	0.037 ns	0.0459	-	-	288 B
AppendFewSegments	339.67 ns	0.174 ns	0.155 ns	0.1578	-	-	992 B
AppendManySegments	778.90 ns	1.636 ns	1.451 ns	0.3872	0.0019	-	2432 B

6. Double the capacity before of the input list

public ODataPath(ODataPath odataPath)
{
   int capacity = odataPath.Segments.Count < odataPath.Segments.Capacity ? odataPath.Segments.Capacity : odataPathSegments.Capacity * 2;
   this.segments = new List<ODataPathSegments>(capacity);
   this.segments.AddRange(odataPath.segments);
}

This is also a variation of the previous 2, aimed at checking whether pre-allocating a larger capacity will reduce resizes.

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
CloneShortPath	24.88 ns	0.031 ns	0.028 ns	0.0204	-	-	128 B
CloneLongPath	34.99 ns	0.119 ns	0.159 ns	0.0510	0.0001	-	320 B
AddSegmentShortPath	65.59 ns	0.054 ns	0.051 ns	0.0293	-	-	184 B
AddSegmentLongPath	76.73 ns	0.101 ns	0.090 ns	0.0598	0.0001	-	376 B
AppendFewSegments	342.32 ns	0.363 ns	0.340 ns	0.1616	0.0005	-	1016 B
AppendManySegments	763.89 ns	0.680 ns	0.603 ns	0.4139	0.0019	-	2600 B

The conclusion I made is that while the last 2 may reduce resizes, each new segment added has to clone the path first. That means that an array of a similar size or greater has to be allocated each time a new item is added. For that reason, it's not beneficial to ensure capacity for more than the new segment being added.

This chosen approach is still fairly inefficient as it requires copying the entire array when adding a new segment. There are more efficient implementations we could consider, for example different clones could share the same list of segments or a linked-list of segments like persistent. But the purpose of this PR was a quick, low-hanging optimization with minimal changes while we continue to conduct more investigations in this area.

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 10 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       : +10 -0
Percentile : 4%

Total files changed: 2

Change summary by file extension:
.cs : +10 -0

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]

Not sure I understand what you mean by this statement:

If I make it public, a different method overload will get called without the user having to opt-in to it or without the user having to change arguments.

Are you saying that if you make this constructor public that will result into existing user code automatically invoking it instead of the constructor that is being invoked presently?

[gathogojr]

@habbes The following statement was confusing to me at first:

2. Create a new List with the same capacity as the length of the existing list

"same capacity" is what threw me off (almost). I think you should say "Create a new List with the capacity equal to the length of the existing list"

[gathogojr]

@habbes Under scenario 4, what does odataPath.Segments.Length represent on line 1 of the code snippet:

int capacity = odataPath.Segments.Length < odataPath.segments.Count ? odataPath.segments.Capacity : odataPathSegments.Capacity + 1;

Did you intend:

int capacity = odataPath.segments.Count < odataPath.segments.Capacity ? odataPath.segments.Capacity : odataPathSegments.Capacity + 1;

You also say that you based this PR on scenario 4 but the code in this PR is different

[gathogojr]

@habbes Curious whether there are places in the existing code that could make use of this new constructor.

[gathogojr]

@habbes I'm not sure how much we do equality checks for ODataPath but maybe we could also optimize by using a for loop in place of Where and Any [here]that (

odata.net/src/Microsoft.OData.Core/UriParser/SemanticAst/ODataPath.cs

Line 152 in 4e263ca

return !this.segments.Where((t, i) => !t.Equals(other.segments[i])).Any();

)

[habbes]

Not sure I understand what you mean by this statement:

If I make it public, a different method overload will get called without the user having to opt-in to it or without the user having to change arguments.

Are you saying that if you make this constructor public that will result into existing user code automatically invoking it instead of the constructor that is being invoked presently?

Yes, that is precisely what I mean.

Currently we have a constructor with the existing signature:

public ODataPath(IEnumerable<ODataPathSegment>);

ODataPath also implements IEnumerable.

So we have code internally that create a copy of the ODataPath using the following pattern:

ODataPath oldPath;
var newPath = new ODataPath(oldPath);

Look at the AddSegment extension method for example. Currently this calls the constructor above.

This PR adds a new a constructor with the signature:

internal ODataPath(ODataPath);

Now the AddSegment method's new ODataPath(odataPath) will go to this new constructor instead of the one that takes an IEnumerable. But External code will still go to the old IEnumerable constructor because the new one is internal. But if I made it public, then even external code with the same pattern will start going to the new constructor.

Note that this change won't affect calls that pass a list of segments directly, or any other enumerable for the matter. It only "hijacks" interal calls that pass an ODataPath directly.

Let me know if that clears things up.

[habbes]

@habbes Under scenario 4, what does odataPath.Segments.Length represent on line 1 of the code snippet:

int capacity = odataPath.Segments.Length < odataPath.segments.Count ? odataPath.segments.Capacity : odataPathSegments.Capacity + 1;

Did you intend:

int capacity = odataPath.segments.Count < odataPath.segments.Capacity ? odataPath.segments.Capacity : odataPathSegments.Capacity + 1;

You also say that you based this PR on scenario 4 but the code in this PR is different

Yes, you're right, that is what I meant:

int capacity = odataPath.segments.Count < odataPath.segments.Capacity ? odataPath.segments.Capacity : odataPathSegments.Capacity + 1;

Also you're right that this slightly differs from the actual code in the PR. I must have changed after writing the description. The initial thought was to set the same capacity as the input list if was already greater than the length. But I didn't notice any improvements in the benchmarks from doing that. The thought was that doing that may reduce resizing. But that benefit didn't materialize because only one segment is going to be added to this newly created list. The next segment after that is going to be added to a whole new ODataPath instance with a whole new list that's a whole new copy. So it didn't make sense to optimize beyond the immediate next segment to be added, otherwise we may just end up with a bunch of arrays that are larger than necessary. Let me update the description to match the code.

[habbes]

@habbes Curious whether there are places in the existing code that could make use of this new constructor.

Yes, I have mentioned an example in a comment above: #2489 (comment)

[habbes]

@habbes I'm not sure how much we do equality checks for ODataPath but maybe we could also optimize by using a for loop in place of Where and Any [here]that (

odata.net/src/Microsoft.OData.Core/UriParser/SemanticAst/ODataPath.cs

Line 152 in 4e263ca

return !this.segments.Where((t, i) => !t.Equals(other.segments[i])).Any();

)

Yeah, we could make that improvement. But I have seen a couple of other places that could benefit from using plain loops to iterate over the ODataPath. I think I could bundle them up in a separate PR, what do you think?

[gathogojr]