Fix performance regression in DataServiceContext DefaultResolveType method

[gathogojr]

Issues

This pull request fixes #2557, fixes #2514

Description

Fix performance regression in DataServiceContext DefaultResolveType method.

• By searching for the materialization type from the assembly containing the data service context instance first
• By searching for the materialization type from the executing assembly next - where it's different from the assembly in the first step
• By searching for the materialization type from the other loaded assemblies while skipping framework and OData libraries where the type wouldn't be expected to be

Performance improvements stats

Before this fix:

Run	Method	Mean	Error	StdDev
1	ResolveType	9.559 ms	0.2664 ms	0.7771 ms
2	ResolveType	10.82 ms	0.187 ms	0.511 ms
3	ResolveType	10.15 ms	0.218 ms	0.644 ms

After this fix:

Run	Method	Mean	Error	StdDev
1	ResolveType	3.173 ms	0.0407 ms	0.1153 ms
2	ResolveType	4.023 ms	0.0813 ms	0.2254 ms
3	ResolveType	3.161 ms	0.0625 ms	0.1772 ms

Before the change that introduced the performance regression:

Run	Method	Mean	Error	StdDev
1	ResolveType	4.901 ms	0.3239 ms	0.9396 ms
2	ResolveType	3.148 ms	0.0417 ms	0.1195 ms
3	ResolveType	5.302 ms	0.2001 ms	0.5511 ms
4	ResolveType	3.965 ms	0.2389 ms	0.6855 ms
5	ResolveType	3.495 ms	0.1477 ms	0.4354 ms

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 123 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

---

Quantification details

Label      : Medium
Size       : +101 -22
Percentile : 44.6%

Total files changed: 3

Change summary by file extension:
.cs : +101 -22

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.

[habbes]

The code looks good to me. I would request some clarifications on the performance measurements:

• Are the figures shared measuring the expected/best case (i.e., the type is in the targetAssembly)?
• If the provided figures are for the expected case, could you share figures for the worst case if possible (the type cannot be resolved)
• Could you run the tests on a version of the client before the changes that introduced the regression were made, just so we can have an idea how close it is to initial expectations, if not the same (or better), at least for the common case (the type does exist in the target or entry assembly).

[xuzhg]

can we cache the 'Resolving' ?

I mean once we resolved the type, we should cache it for next use, right?

[gathogojr]

@xuzhg The cache is in the DataServiceContext class.

[KenitoInc]

LGTM

[gathogojr]

The code looks good to me. I would request some clarifications on the performance measurements:

• Are the figures shared measuring the expected/best case (i.e., the type is in the targetAssembly)?
• If the provided figures are for the expected case, could you share figures for the worst case if possible (the type cannot be resolved)
• Could you run the tests on a version of the client before the changes that introduced the regression were made, just so we can have an idea how close it is to initial expectations, if not the same (or better), at least for the common case (the type does exist in the target or entry assembly).

@habbes Failure to find a type to use for materialization when deserializing a payload results into an exception. However, I was able to create a simple benchmark to measure the time the DefaultResolveType method takes if a type is not found. The DefaultResolveType method is protected that's why I did it this way.

public class DataServiceContextBenchmark : DataServiceContext
{
    [Benchmark]
    [IterationCount(targetIterationCount: 100)]
    public void BenchmarkResolveType()
    {
        for (var i = 0; i < 250; i++)
        {
            this.DefaultResolveType("NonExistentType", "NS.Models", "NS.Models");
        }
    }
}

Stats:

Before this fix:

Run	Method	Mean	Error	StdDev
1	BenchmarkResolveType	4.559 us	0.3813 us	1.124 us
2	BenchmarkResolveType	2.599 us	0.0847 us	0.2319 us
3	BenchmarkResolveType	5.856 us	0.7412 us	2.174 us
4	BenchmarkResolveType	4.339 us	0.1353 us
5	BenchmarkResolveType	3.541 us	0.2066 us	0.5828 us

After this fix:

Run	Method	Mean	Error	StdDev
1	BenchmarkResolveType	2.912 us	0.0930 us	0.2653 us
2	BenchmarkResolveType	3.260 us	0.1559 us	0.4572 us
3	BenchmarkResolveType	2.262 us	0.0394 us	0.1136 us
4	BenchmarkResolveType	4.269 us	0.3640 us	1.068 us
5	BenchmarkResolveType	2.603 us	0.1108 us	0.3160 us

The results are a bit inconsistent across runs but it'd be right to say their is an observable improvement after the fix

[habbes]

Thanks for adding the additional benchmark data. Looks good to me!