Skip to content

Support multiple rights vs same left scenario with a simple call to ApiComparer #17295

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 5, 2021

Conversation

safern
Copy link
Member

@safern safern commented Apr 29, 2021

With these changes, comparing the same left against multiple rights is optimized ~30% in time, here are the benchmark results:

Method Mean Error StdDev
UseOneApiCompatPerPath 442.8 ms 5.12 ms 4.79 ms
CompareOnePerSide 107.5 ms 0.64 ms 0.53 ms
UseOneApiCompatOverall 291.4 ms 2.05 ms 1.92 ms

UseOneApiCompatPerPath is invoking ApiComparer.GetDifferences for each combination of (left, right) which takes 442.8ms, vs now with this changes you would only need to do what I do in UseOneApiCompatOverall which calls ApiComparer.GetDifferences once with an IList for the rights, which takes 291.4ms for comparing System.Runtime.dll ref assembly vs System.Runtime.dll implementation with 4 different instances of it, that's a 151ms improvement.

Also, the regular scenario of comparing one left against one right, is about the same, this is before these changes:

Method Mean Error StdDev
CompareOnePerSide 101.7 ms 1.03 ms 0.86 ms

Here are the benchmarks I used to measure this: https://gist.github.com/safern/6a1354ee3e3d1e13272c5c3119a6e466

TODO after this change is being able to customize the NoWarn and IgnoredDifferences for the different rights that are being passed as at the moment it uses the same, which might not be desirable in some cases. I'll address that as part of the error baselining story.

Also, change how the Rules are loaded and invoked, by following a similar model as Roslyn, where an Initialize method is invoked and we pass in a context. Then Rules can register to context callbacks, that way the Rule just register to whatever callbacks it is important to run on based on the Mappers it runs for. This change fixes: #16918

ApiComparer.

Also, change how the Rules are loaded and invoked, by following a
similar model as Roslyn, where an Initialize method is invoked and we
pass in a context. Then Rules can register to context callbacks, that
way the Rule just register to whatever callbacks it is important to run
on based on the Mappers it runs for.

namespace Microsoft.DotNet.ApiCompatibility.Abstractions
{
public class ElementContainer<T>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started with this as a Generic type even though it is now currently used with IAssemblySymbol only as this might be useful to use it to store the elements in the mappers and that way we could customize the diagnostic messages using the metadata information for each element in the mapper.

However I don't know if it is too much overhead and instead just have a setting to customize the left and right names when raising diagnostics.

@safern safern enabled auto-merge (squash) May 5, 2021 18:57
@safern safern merged commit fafd384 into dotnet:main May 5, 2021
@safern safern deleted the Optimize branch May 5, 2021 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ApiCompatibility.RuleDriver logic to load the rules and run them on the APIs should be revisited
2 participants