Move Enum and MdImport to shared

[EgorBo]

Also, I replaced half-native CompareTo with managed version from CoreRT, is it ok or I should attach benchmarks? (if it looks good - I'll delete the native impl for InternalCompare)
CoreRT implementation uses EETypePtr here and there in Enum so it's not easy to reuse this implementation (I managed to extract ~40% of common code between CoreCLR and CoreRT and 95% between CoreCLR and Mono).

Naive Enum.Mono.cs implementation.

/cc @marek-safar @jkotas

[jkotas]

is it ok or I should attach benchmarks?

Yes, benchmarks to double check would be good.

[jkotas]

Note that the Enum implementation overall is not particularly great. #22161 has good ideas how to make it better, but it uses generics too much. Once we get to sorting this out, I expect that it will change a lot.

[EgorBo]

From a quick glance that implementation looks mono-friendly

[jkotas]

From a quick glance that implementation looks mono-friendly

But I expect that the ties (FCalls, etc.) it is going to have with the rest of the runtime will be very different.

[MichalStrehovsky]

I thought we don't put methods marked InternalCall into shared partition.

Also RuntimeType is not a shared type and we haven't been using it in the shared partition (except for some odd .RuntimeType.cs dot files).

[EgorBo]

@MichalStrehovsky I removed the internalcalls from the shared part but I can't remove RuntimeType stuff without it I won't be able to share 95% of code with mono.

[EgorBo]

@jkotas
It seems the the managed impl of CompareTo is slower
Benchmark - https://gist.github.com/EgorBo/033432f3e1fb5f419ffb6b38c6ffb558
Results:

| description |           New |           Old |
|------------ |--------------:|--------------:|
|      case 1 |      9.908 ns |      6.367 ns |
|      case 2 |      9.875 ns |      6.371 ns |
|      case 3 | 12,054.080 ns | 12,028.336 ns |
|      case 4 |     10.137 ns |      6.756 ns |
|      case 5 |     10.135 ns |      6.529 ns |
|      case 6 | 12,032.973 ns | 11,741.258 ns |

New - my branch
Old - master

[jkotas]

That is not that surprising given how it is implemented. I do not think we want to take this regression.

[EgorBo]

@jkotas yeah I undid that change

[filipnavara]

I can't remove RuntimeType stuff without it I won't be able to share 95% of code with mono.

You can follow the pattern taken with Activator.RuntimeType.cs - ie. split the part using RuntimeType into separate file in shared partition and guard it appropriately in the shared project.

[jkotas]

You can follow the pattern taken with Activator.RuntimeType.cs - ie. split the part using RuntimeType into separate file in shared partition and guard it appropriately in the shared project.

Thoughts about this?

[marek-safar]

Thoughts about this?

Improving CoreRT sharing was nice to have and not required so it can be done later, right?

[jkotas]

Right, but should we use consistent patterns for this?

[MichalStrehovsky]

Improving CoreRT sharing was nice to have and not required so it can be done later, right?

Both files would stay excluded for CoreRT, but this is a structure (putting things requiring RuntimeType into separate dot files) we already have in place. Doing this later just messes up git history for the individual lines. IMHO it's better to do the break in one go.

[marek-safar]

What will be in shared can be shared with CoreRT too, probably not much but code like

coreclr/src/System.Private.CoreLib/shared/System/Enum.cs

Line 320 in 56d9776

public static object Parse(Type enumType, string value) =>

is not fully Enum.RuntimeType.cs specific. We could do the Enum.cs vs Enum.RuntimeType.cs split if that's something you see useful.

[MichalStrehovsky]

I did a search and replace of RuntimeType to Type in this file to get a quick sense of how much this code actually relies on RuntimeType. The only method that actually accesses members is GetCachedValuesAndNames.

How would people feel if we just replace RuntimeType with Type and use the cross-runtime portable IsRuntimeImplemented method for argument validation. We would cast to RuntimeType at boundaries? We used that pattern elsewhere.

[EgorBo]

@jkotas @MichalStrehovsky I tried to share some non-RuntimeType related stuff with CoreRT in my recent commits (tested on CoreRT locally).

[MichalStrehovsky]

I tried to share some non-RuntimeType related stuff with CoreRT

How about the replacement of RuntimeType with Type I suggested above? Is it problematic? It looks like we could move even more to the "shared with everything" part (to the point where the RuntimeType.cs file might no longer be needed). The parsing code is one of the most actively worked on parts, so having it in the right long-term place would be nice.

[jkotas]

You may be able to do this by doing using Runtime = Type; to keep CoreCLR on the RuntimeType plan if it is important.

[EgorBo]

@jkotas @MichalStrehovsky done! Here is the Enum.CoreRT.cs I tested with these changes.
Also, had to change Type a bit (the Enum-related methods) and EnumInfo.

[jkotas]

Here is the Enum.CoreRT.cs I tested with these changes. Also, had to change Type a bit (the Enum-related methods) and EnumInfo.

Could you please share link to CoreRT commit or send WIP PR to CoreRT repo that we will cherry pick once the change is mirrored?

[EgorBo]

@jkotas ok, so here is the PR dotnet/corert#7180 (looks green, the failing lanes as far as I understand are not related)
the only thing I am not sure is how to properly implement GetCachedValuesAndNames for CoreRT, CoreCLR has GenericCache property in RuntimeType

[jkotas]

TypeValuesAndNames in CoreCLR is the same thing as EnumInfo in CoreRT. EnumInfo is cached on the runtime type same way as TypeValuesAndNames is cached on the runtime type in CoreCLR.

So the implement the caching in CoreRT, make EnumInfo look like TypeValuesAndNames.

[jkotas]

Thanks! Let me know if you are going to finish the CoreRT side. If not, I can take it from here.