Runtime: About 30% performance regression for GetTypeCode() test

[maryamariyan]

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

[maryamariyan]

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.

[kouvel]

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

[maryamariyan]

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

[keeratsingh]

@maryamariyan
May I know why you think this is a System.Data.SqlClient issue ?

[kouvel]

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.

[danmosemsft]

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

[sandreenko]

@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.

[danmosemsft]

@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.

[jkotas]

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.

[danmosemsft]

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?

[jkotas]

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

[stephentoub]

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]

@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> and IComparable<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.

[stephentoub]

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?

[jkotas]

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 .

[stephentoub]

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.