Allow skipping the processing of a reflection call if its MarkStep's ProcessReflectionDependency() returns true #1548
Conversation
|
/azp run |
|
Commenter does not have sufficient privileges for PR 1548 in repo mono/linker |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -487,14 +487,18 @@ static IntrinsicId GetIntrinsicIdForMethod (MethodDefinition calledMethod) | |||
|
|
|||
| public override bool HandleCall (MethodBody callingMethodBody, MethodReference calledMethod, Instruction operation, ValueNodeList methodParams, out ValueNode methodReturnValue) | |||
| { | |||
| methodReturnValue = null; | |||
|
|
|||
| var reflectionProcessed = _markStep.ProcessReflectionDependency (callingMethodBody, operation); | |||
There was a problem hiding this comment.
It seems the method will be called twice for each call. Once here and second time in the existing MarkStep.MarkReflectionLikeDependencies.
I don't know exactly what Unity wants here - maybe it would be easier to add an extension point which disables the data flow analysis completely (my understanding is that Unity has its own version of that anyway).
There's no virtual method to do that yet, but it would be pretty simple to add one like that.
There was a problem hiding this comment.
See https://discordapp.com/channels/732297728826277939/751137004007456849/763083086782529556
Open to ideas. We were struggling to find a clean way to accomplish our goal.
As for being called twice, we should remove that. This entire loop is pointless https://github.com/mono/linker/blob/9a4a5a06d54ed13f2215a7e0d0e39663ee10b51d/src/linker/Linker.Steps/MarkStep.cs#L2965 . @brian-taylor-unity Can you update this PR to remove that entire loop over the instructions and calling ProcessReflectionDependency?
There was a problem hiding this comment.
Thanks - I read up on the Discord and it makes much more sense now. Once we remove the useless loop I'm fine with this change - if it works for you.
There was a problem hiding this comment.
Nit: Do we need the local variable?
There was a problem hiding this comment.
Not necessary, I added it mainly to encapsulate some of the meaning of why it would cause an early-return in the method, but I don't really have a strong opinion either way.
There was a problem hiding this comment.
OK - I don't have strong feeling about it either
…n instruction if its MarkStep's ProcessReflectionDependency() returns true.
brian-taylor-unity
force-pushed
the
process-reflection-dependency-check
branch
from
fac6433 to
5006e58
Compare
| @@ -487,14 +487,18 @@ static IntrinsicId GetIntrinsicIdForMethod (MethodDefinition calledMethod) | |||
|
|
|||
| public override bool HandleCall (MethodBody callingMethodBody, MethodReference calledMethod, Instruction operation, ValueNodeList methodParams, out ValueNode methodReturnValue) | |||
| { | |||
| methodReturnValue = null; | |||
|
|
|||
| var reflectionProcessed = _markStep.ProcessReflectionDependency (callingMethodBody, operation); | |||
There was a problem hiding this comment.
Nit: Do we need the local variable?
Allow ReflectionMethodBodyScanner's HandleCall() to skip processing an instruction if its MarkStep's ProcessReflectionDependency() returns true.