Move specialized execute method code into an external phase #51954
Conversation
jdconrad
added
:Core/Infra/Scripting
|
Pinging @elastic/es-core-infra (:Core/Infra/Scripting) |
| import java.util.Map; | ||
|
|
||
| /** | ||
| * This injects additional ir nodes required to for |
| callSubNode.addArgumentNode(unboundFieldNode); | ||
|
|
||
| for (Class<?> throwable : new Class<?>[] { | ||
| PainlessError.class, BootstrapMethodError.class, OutOfMemoryError.class, StackOverflowError.class, Exception.class}) { |
There was a problem hiding this comment.
this could be a class constant, like ERRORS_TO_CATCH?
There was a problem hiding this comment.
This is going to change in future PRs so I would prefer to leave this for now.
| } | ||
| } | ||
|
|
||
| protected DecorateExecutePhase() { |
There was a problem hiding this comment.
this can be private, no need to construct right?
|
|
||
| import static org.elasticsearch.painless.WriterConstants.CLASS_TYPE; | ||
|
|
||
| public class UnboundFieldNode extends ExpressionNode { |
There was a problem hiding this comment.
Would MemberFieldNode be a better name here?
|
|
||
| if (name.equals(declarationNode.getName())) { | ||
| if (scriptRoot.getUsedVariables().contains(name)) { | ||
| UnboundCallNode unboundCallNode = new UnboundCallNode(); |
There was a problem hiding this comment.
Maybe we should rename this to MemberCallNode? The meaning of unbound is unclear
| * that are used and adds additional sandboxing by wrapping | ||
| * the main "execute" block with several exceptions. | ||
| */ | ||
| public class DecorateExecutePhase { |
There was a problem hiding this comment.
This isn't really decorating execute (well the try/catch is, but it's still not decorating, it's wrapping in that case?). In the other case we have variables being converted to this.getXXX() calls...they seem unrelated, maybe they should be separate classes? In both these cases these seem to be things we need for correctness in the IR tree, so to me should be part of the creation of the IR tree, rather than phases operating on it. But I also appreciate this is just a step towards the overarching refactor, so if this frees up space to move towards that future, I'm fine with this as is for now.
There was a problem hiding this comment.
I renamed this ScriptInjectionPhase for now. But as we discussed this will change significantly, and I agree with your assessments of correctness. I would like to note that my line of thought was the user tree is what requires semantic checking and the ir tree is semantically correct (always other than bugs). I will also consider correctness moving forward in that the ir tree should always be in a state that can be written to ASM as any given time. We will get there, but there's a bunch more work in between to clean up enough to make those changes.
|
@rjernst Thank you for the review. I hope I've adequately addressed your concerns, and I think we continue to be on the same page for the end goals of these refactors. Will merge as soon as CI passes. |
This change moves almost all of the customized code required to decorate the execute method into an external phase. This external phase operates only on the ir tree after the user tree has generated the initial ir tree from user specified code. The external phase uses ir nodes to create the necessary customized code instead of writing asm directly. This is a first example of modifying the ir tree to customize it specifically for Elasticsearch and leaves the ir nodes in a more generic state.
Another change required for this was to remove the notion of auto-return from the ir tree completely. The user tree is now responsible for generating appropriate ir tree nodes to support auto-return. This is the first example of divergent user and ir trees as the user tree is intended to be higher level while the ir tree is supposed to provide lower level translation into asm.
Relates to #49869
Relates to #51841