-
Notifications
You must be signed in to change notification settings - Fork 24.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Painless] Generate Bridge Methods #36097
Conversation
Pinging @elastic/es-core-infra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one naming suggestion
@@ -1296,4 +1314,166 @@ private void setFunctionalInterfaceMethod(Class<?> targetClass, PainlessClassBui | |||
} | |||
} | |||
} | |||
|
|||
private void generateRuntimeMethods() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use a more descriptive name for these methods? "runtime" is very generic. eg maybe "casting bridge methods" or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is actually runtime methods. I added a fairly in-depth description as a Javadoc for what this method is creating and what happens with the result.
@rjernst Thanks for the review! Will commit as soon as CI is complete. |
We use MethodHandles.asType to cast argument types into the appropriate parameter types for method calls when the target of the call is a def type at runtime. Currently, certain implicit casts using the def type are asymmetric. It is possible to cast Integer -> float as an argument to parameter, but not from int -> Float (boxed to primitive with upcasting is okay, but primitive to boxed with upcasting is not). This PR introduces a solution to the issue by generating bridge methods for all whitelisted methods that have at least a single boxed type as an argument. The bridge method will conduct appropriate casts and then call the original method. This adds a bit of overhead for correctness. It should not be used often as Painless avoids boxed types as much as possible. Note that a large portion of this change is adding methods to do the appropriate def to boxed type casts and a few mechanical changes as well. The most important method for review is generateBridgeMethod in PainlessLookupBuilder.
We use MethodHandles.asType to cast argument types into the appropriate parameter types for method calls when the target of the call is a
def
type at runtime. Currently, certain implicit casts using thedef
type are asymmetric. It is possible to castInteger
->float
as an argument to parameter, but not fromint
->Float
(boxed to primitive with upcasting is okay, but primitive to boxed with upcasting is not).This PR introduces a solution to the issue by generating bridge methods for all whitelisted methods that have at least a single boxed type as an argument. The bridge method will conduct appropriate casts and then call the original method. This adds a bit of overhead for correctness. It should not be used often as Painless avoids boxed types as much as possible.
Note that a large portion of this change is adding methods to do the appropriate def to boxed type casts and a few mechanical changes as well. The most important method for review is generateBridgeMethod in PainlessLookupBuilder.