Closed maryamariyan closed 4 years ago
Because of the same corefx diff above, we have about 10% performance regression for ToDateTime_String() test as well. Chart below illustrates this regression:
Refer to benchview for more details.
It looks like this is the diff range for the first point of regression: https://github.com/dotnet/corefx/compare/e1f304886a8315fd3303ff0e75dc7595c42b8cd0...dotnet:5cc8f36bec8ee6e67f8576bdc698bef1a280e9b9
It may be the coreclr update
Thanks @kouvel I am reinvestigating the diff and also corrected the above description.
@kouvel the coreclr diff contained in the diff above is: dotnet/coreclr@a8d2e3f06ce3c345ff79537e395c95e1e51fa9be
@maryamariyan
May I know why you think this is a System.Data.SqlClient
issue ?
It looks like https://github.com/dotnet/coreclr/commit/a8d2e3f06ce3c345ff79537e395c95e1e51fa9be is the commit hash of the baseline coreclr package. The updated package I believe would contain these changes: https://github.com/dotnet/coreclr/compare/a8d2e3f06ce3c345ff79537e395c95e1e51fa9be...58b404512a589a9b7cfc917d7217596c5607fe13. From that list nothing is popping out at me as the source, probably needs some investigation. Updated area path to location of tests for now.
The test is extremely simple, it does Convert.GetTypeCode("Hello world")
which casts the string to IConvertible
and calls GetTypeCode()
which just returns a member of an enum.
@sandreenko is there any possibility this could be related? It is the only possibly interesting JIT change in this period. The test is not using the return value of Convert.GetTypeCode(value);
if that matters?
https://github.com/dotnet/coreclr/commit/8559a6de5e10706bd8a96d19779054867bfd4e46
@danmosemsft It looks unlikely, but still possible. That PR could change live intervals and affected register allocations (it should affect it in a good way, because with this PR we detect and delete more dead code that should have good effect on RA).
I think asm diffs will be able to find what caused that.
@stephentoub do you believe this is worth tracking for 2.1 ? It seems to me probably not. The code is not doing much and it appears likely to be a code gen related change that left other scenarios (with the possible exception of 8% on ToDateTime_String) unaffected.
I believe that this was introduced by https://github.com/dotnet/coreclr/pull/16749/
This change modified interface declaration order on String (reconciling diffs w/ CoreRT):
Before: https://github.com/dotnet/coreclr/pull/16749/files#diff-d2a641ec9b77a5ea1cc985bdec3e74efL44
After: https://github.com/dotnet/coreclr/pull/16749/files#diff-0c82b98e414434b145b24386045c47a8R24
Interface casting is implemented as linear search. IConvertible moved from position 3 to position 6. It made casting to IConvertible that dominates GetTypeCode implementation slower. and casting to other string interfaces comparatively faster.
Aha..I didn't notice that. Do you consider the new ordering to be best? It seems reasonable.
Is it feasible to improve that linear search?
Do you consider the new ordering to be best? It seems reasonable.
It is hard to tell whether the new ordering is the best without data. Overall, I think the new ordering is better than the old one. The old one has ICloneable early. ICloneable is legacy interface that documentation recommends to not use. The new one has it last that is certainly better.
Is it feasible to improve that linear search?
https://github.com/dotnet/coreclr/issues/603#issuecomment-215879810
Overall, I think the new ordering is better than the old one.
Certainly IEquatable<string>
, IComparable<string>
, and IEnumerable<char>
are more important than ICloneable
and IConvertible
, so favoring the former by moving the latter to the end certainly seems like the right decision.
@jkotas, would anything be changed if we moved IEnumerable
to the end? I'm wondering how things would be impacted, if at all, since IEnumerable<char>
inherits IEnumerable
.
do you believe this is worth tracking for 2.1 ?
No. Convert.GetTypeCode is not used on hot paths, or if it is, you don't care about performance.
@jkotas, would anything be changed if we moved IEnumerable to the end?
IEnumerable
would right after IEnumerable<char>
in the list. The type builder adds the declared interface to the list first and then expands it immediately.
IEquatable<string>
andIComparable<string>
are more important
I have run experiments to see what the interface casting tends to be called on. I did not see generic types like IEquatable<string>
or IComparable<string>
much. They are typically used as generic constrains and so the runtime casts to them are pretty rare.
I think it may be reasonable to move IConvertible
back to position 3 where it used to be to fix this regression.
I did not see generic types like IEquatable
or IComparable much.
Really? Won't they be used by collections a bunch, e.g. a hash set of strings? Or does EqualityComparer.Default special-case strings?
EqualityComparer.Default
The cast happens once (via reflection) when the comparer is created: https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Collections/Generic/ComparerHelpers.cs#L134 .
Ah, it's not that the interface isn't used much; it's actually used a lot. It's that we only cast to it rarely, as the cast result is then cached.
About 30% performance regression for GetTypeCode() test as seen in the chart below
Correction: Below is the corefx diff from early March: https://github.com/dotnet/corefx/compare/e1f304886a8315fd3303ff0e75dc7595c42b8cd0...dotnet:5cc8f36bec8ee6e67f8576bdc698bef1a280e9b9
Refer to benchview for more details.
cc: @kouvel @danmosemsft