-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
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> |
There was a problem hiding this comment.
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.
With these changes, comparing the same left against multiple rights is optimized ~30% in time, here are the benchmark results:
UseOneApiCompatPerPath
is invokingApiComparer.GetDifferences
for each combination of (left, right) which takes442.8ms
, vs now with this changes you would only need to do what I do inUseOneApiCompatOverall
which callsApiComparer.GetDifferences
once with anIList
for the rights, which takes291.4ms
for comparingSystem.Runtime.dll
ref assembly vsSystem.Runtime.dll
implementation with 4 different instances of it, that's a151ms
improvement.Also, the regular scenario of comparing one left against one right, is about the same, this is before these changes:
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
andIgnoredDifferences
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