Covariant returns for RuntimeType #45355
Conversation
benaadams
requested review from
akoeplinger,
EgorBo,
marek-safar,
steveisok and
vargaz
as code owners
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
| } | ||
| return a; | ||
| } | ||
| } | ||
|
|
||
| public static new RuntimeType GetTypeFromHandle(RuntimeTypeHandle handle) |
There was a problem hiding this comment.
Some of these are going to break analysis within IL Linker whenever they're encountered. IL Linker doesn't hardcode runtime implementation details such as these optimizations - we wouldn't want to hardcode it into linker. Linker operates on public APIs only.
E.g. this one is normally an intrinsic, so is e.g. EmptyTypes. This will make it more difficult for IL Linker to statically analyze what's going on in these spots.
Cc @vitek-karas
There was a problem hiding this comment.
🤔 The covariant returns allow a lot of the casts back to RuntimeType to be dropped (haven't applied all the changes in dropping the casts); and also to devirtualized as direct calls as its a sealed type, rather than virtual calls via Type
There was a problem hiding this comment.
Do you have any perf numbers for this?
The cast should be pretty cheap because it's an exact match (the cast will trivially succeed in the part of the type check that RyuJIT typically inlines - it succeeds before actually calling into the casting logic).
There was a problem hiding this comment.
Type checks then might not mean much?
Though it does also drop the virtual calls:
There was a problem hiding this comment.
I meant the non-R2R case: https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQxNMAfAAgZgARRwQA2cAJnlgEx4DCeA3gLABQe7l+WAjAGx14AsgAoA9sABWcAMYw8ogJSNWHVZQDseYbQWiA3Co4BfVkaA
RyuJIT generates two checks before calling the casting helper - is it a null object, or is it exactly the same type that we're casting to. If yes, it skips calling into runtime completely.
This change is still an improvement, but is it an improvement that would justify likely having to implement & maintain a IL linker feature? E.g. just earlier today I implemented intrinsic recognition of calls to Type.UnderlyingSystemType within linker motivated by the use within CoreLib - after this change, any call to get_UnderlyingSystemType on a RuntimeType would no longer be intrinsically recognized without linker work because this is a callvirt to a different method.
| @@ -769,12 +769,12 @@ public sealed override void ReorderArgumentArray(ref object?[] args, object stat | |||
|
|
|||
| // Return any exact bindings that may exist. (This method is not defined on the | |||
| // Binder and is used by RuntimeType.) | |||
| public static MethodBase? ExactBinding(MethodBase[] match, Type[] types, ParameterModifier[]? modifiers) | |||
| public static RuntimeConstructorInfo? ExactBinding(RuntimeConstructorInfo[] match, Type[] types, ParameterModifier[]? modifiers) | |||
There was a problem hiding this comment.
Using RuntimeConstructorInfo will make sharing of this code problematic with NativeAOT.
DefaultBinder is a super slow path. An extra casts here won't make difference.
| { | ||
| return GetFieldCandidates(null, bindingAttr, false).ToArray(); | ||
| } | ||
|
|
||
| public override Type[] GetInterfaces() | ||
| public override RuntimeType[] GetInterfaces() |
There was a problem hiding this comment.
Note that this is potential breaking change if the caller mutates the array and tries to store non-RuntimeType types into the array.
Ben Adams has tried to add usage of covariant returns for the RuntimeType as the overriding return type and it has uncovered an ordering issue in the SystemDomain::LoadBaseSystemClasses w.r.t. This change fixes it by shuffling parts of initialization around a bit.
|
// Auto-generated message 69e114c which was merged 12/7 removed the intermediate src/coreclr/src/ folder. This PR needs to be updated as it touches files in that directory which causes conflicts. To update your commits you can use this bash script: https://gist.github.com/ViktorHofer/6d24f62abdcddb518b4966ead5ef3783. Feel free to use the comment section of the gist to improve the script for others. |
|
Hi @benaadams -- I wanted to see if this was still on your radar. There's some feedback remaining and a change to our folder structure would also need to be accommodated. If you still want to move this forward, we can leave it open; otherwise we could close this PR--either is fine with us; just let us know. Thanks! |
|
It interesting; but merge conflicts aside, sounds like there might be issues doing for runtime types specifically. Also will cause additional merge conflicts for #45121; might revisit another time |
ghost
locked as resolved and limited conversation to collaborators
Unfortunately crashes during crossgen