-
Notifications
You must be signed in to change notification settings - Fork 272
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
port TypeDescriptor benchmarks #83
Conversation
[Arguments(typeof(ClassIBase), typeof(IBaseConverter))] | ||
[Arguments(typeof(ClassIDerived), typeof(IBaseConverter))] | ||
[Arguments(typeof(Uri), typeof(UriTypeConverter))] | ||
public TypeConverter GetConverter(Type typeToConvert, Type expectedConverter) // the expectedConverter argument is not used anymore, but kept to remain BenchView ID, do NOT remove |
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.
// [](start = 86, length = 2)
Should we remove this and start a fresh/clean benchmark? #Closed
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.
You mean the expectedConverter
argument?
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.
Or in general?
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.
In this case, the expectedConverter
unused variable. #Closed
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.
But, this would be a good opportunity to start a cleaner/newer benchmark
too. #Closed
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.
But, this would be a good opportunity to start a cleaner/newer benchmark too.
hmm, since after removing the Asserts from the benchmark the time is 1/7 of what it was before, I can remove this argument and rename the benchmark
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.
@jorive I have removed the unused argument and removed the inner iteration count to let BDN do the job (it's a new benchmark after rename so I could do that)
# Conflicts: # src/benchmarks/Benchmarks.csproj
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.
Fixes #48
This an excellent example why benchmarks MUST NOT contain any Asserts. I have ported the benchmarks and removed the asserts and BDN reported 1/7 of the time reported by xunit-performance.
I was shocked!
And then I used ILSpy to see what Assert was being used in the benchmark. The one from full xunit framework ;)
So Asserts were 6/7 of the benchmarked time ;)
I did not have to port the Asserts to CoreFX because they are already there (each case has it's own test class)
I also removed duplicated test cases, which were benchmarking same execiton path.
/cc @jorive