-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Add Performance Tests from the CLR Perf Lab #7152
Conversation
@stephentoub, can you please review this? |
@dotnet-bot, test this please. |
private static int? s_value; | ||
private static int? s_item; | ||
|
||
private static Dictionary<long?, long?> s_collection1; |
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.
Why do any of these need to be statics rather than just being locals in the tests?
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.
They don't need to be statics as far as i can tell. FYI, I did not do the original porting, so I may not have all of the context, but I'll be happy to make appropriate changes.
I've fixed this.
@dotnet-bot test this please |
{ | ||
foreach (var iteration in Benchmark.Iterations) | ||
using (iteration.StartMeasurement()) | ||
String.Compare(s1, s2); |
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.
Do we care about the fact that we are using ambient state here? Should we instead pick some fixed culture (or set of fixed cultures, e.g. Invariant, en-US, tr-TR, ja-JP)?
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.
This one makes me pause, because it's possible that a developer who has a different default culture will see different results than I'd see. However, I like the idea of capturing current culture differences in build-to-build runs on stable hardware. So, I'm inclined to keep this as is.
Of course, the origin here is that we ran this in a stable lab whose culture was always en-us.
Do you have concerns leaving this as is?
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.
The implementation of String.Compare just forwards to something off current culture:
public static int Compare(String strA, String strB) {
return CultureInfo.CurrentCulture.CompareInfo.Compare(strA, strB, CompareOptions.None);
}
Which is what the Compare that takes a CultureInfo does, when called with CompareOptions.None
public static int Compare(String strA, String strB, CultureInfo culture, CompareOptions options) {
if (culture==null) {
throw new ArgumentNullException("culture");
}
Contract.EndContractBlock();
return culture.CompareInfo.Compare(strA, strB, options);
}
We often have specific optimizations for en-US and Invariant because we know things abort sort orders we can exploit to do less work.
I think you get the same measurement of String comparison performance build to build if you use a fixed culture, and by using multiple cultures we also get coverage across different locales. The only way this invariant would be violated is if these two methods started having radically different implementations instead of thunking to the same place. I don't expect that's something that would happen long term.
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.
Fair enough. I've split this into 3 tests - invariant culture, en-us, ja-jp.
I've just updated this PR with the latest code based on your feedback. Please let me know if you have any more questions or comments. Thanks. |
Test Innerloop OSX Release Build and Test please |
dict.ContainsKey(key); dict.ContainsKey(key); dict.ContainsKey(key); | ||
dict.ContainsKey(key); dict.ContainsKey(key); dict.ContainsKey(key); | ||
dict.ContainsKey(key); dict.ContainsKey(key); dict.ContainsKey(key); | ||
item = (int)collection[j]; |
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'm still not clear on why in the test for indexer perf we're using a nullable, getting its Value in order to cast it to an int, and then constructing a new nullable from it.
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.
Not sure... I'm wondering if it works around a dead store optimization in the JIT. Otherwise, I would expect this loop to only do one fetch and store.
I skimmed through and left a few more comments. Generally looks good. Feel free to address the nits however you see fit and then merge once CI is green. Thanks. |
Thanks much. I've made all of the changes and will merge once things are green. |
Add Performance Tests from the CLR Perf Lab Commit migrated from dotnet/corefx@9d2ae87
Add a set of performance tests from the desktop performance lab targeted at corefx surface.