Fix reflection invoke file format issue for generic methods #83438
Conversation
The file format that mapped reflection metadata to runtime artifacts had a limitation that required generic methods to be always instantiated over concrete types - we could not place canonical method bodies there. The limitation stemmed from the fact that the mapping table was placing a generic dictionary in it. This switches the table to use NameAndSignature + an array of instantiation arguments. The rest of this change is just deleting code that was working around the problem. The fixes in NativeLayoutVertexNode and NodeFactory.NativeLayout are fixing analysis holes that were exposed by our ability to be more lazy. Also saves 0.4% on BasicWebApi, which is always nice.
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsThe file format that mapped reflection metadata to runtime artifacts had a limitation that required generic methods to be always instantiated over concrete types - we could not place canonical method bodies there. The limitation stemmed from the fact that the mapping table was placing a generic dictionary in it. This switches the table to use NameAndSignature + an array of instantiation arguments. The rest of this change is just deleting code that was working around the problem. The fixes in NativeLayoutVertexNode and NodeFactory.NativeLayout are fixing analysis holes that were exposed by our ability to be more lazy. Also saves 0.4% on BasicWebApi, which is always nice. Cc @dotnet/ilc-contrib
|
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Linq tests need to be looked at: |
|
I had a chance to sleep on this a couple time and I have one question that I can't make my mind on: Currently, if reflection analysis/RD.XML/whatever comes up with the idea that we need The question is whether I went too far with turning all analysis to happen with canonical forms. Whether it would be enough to take the part of this change that allows canonical forms of generic methods (like we already allow for canonical forms of types), but don't force the entire analysis to happen on top of Canon. Leave the option to force concrete instantiations open. |
|
If I understand correctly, these are benefits of each option:
Is this right? |
|
Yep. And it can be used to work around type loader bugs. |
|
"Analysis based on cannon" option looks better to me (ie what you have there right now). I would not optimize for having easier workarounds for type loader bugs. We can use #81741 for that once it becomes available. |
Oh, not a regression, this is an improvement. Not being reflection visible at all might be a separate bug... |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ghost
locked as resolved and limited conversation to collaborators
The file format that mapped reflection metadata to runtime artifacts had a limitation that required generic methods to be always instantiated over concrete types - we could not place canonical method bodies there. The limitation stemmed from the fact that the mapping table was placing a generic dictionary in it.
This switches the table to use NameAndSignature + an array of instantiation arguments.
The rest of this change is just deleting code that was working around the problem. The fixes in NativeLayoutVertexNode and NodeFactory.NativeLayout are fixing analysis holes that were exposed by our ability to be more lazy.
Also saves 0.4% on BasicWebApi, which is always nice.
Cc @dotnet/ilc-contrib