-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
use MethodHandles instead of reflect.Method #4329
Conversation
can these be static final fields? those have special optimizations in hotspot |
they can't be static final the way the code is written at the moment but a bigger rewrite might be able to achieve that it looks like we won't be able to merge this anyway because animal-sniffer (Android compatibility checking) is rejecting the use of MethodHandles |
@@ -91,10 +104,10 @@ public String[] getRecordFieldNames(Class<?> recordType) throws IllegalArgumentE | |||
for (int i = 0; i < components.length; i++) { | |||
try { | |||
names[i] = (String) RECORD_COMPONENT_GET_NAME.invoke(components[i]); | |||
} catch (Exception e) { | |||
} catch (Throwable t) { |
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.
What would be the reason to catch Errors too?
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.
it's a required change unfortunately because invoke throws Throwable. this method shouldnt throw at all though
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.
Ok. Odd, not sure why it does that... never understood why method signature would do that (given Error
s need not be declared anyway so why not just throws Exception
). Oh well. At least this is for a reason then.
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.
Did you know that Java reflection API is now implemented using MethodHandle(s)? So by using Java reflection, you are already using MethodHandle(s) under the hood.
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.
Is this what you mean?
https://openjdk.org/jeps/416
I'll close this PR anyway because of the animal sniffer issues.
jackson-databind is very dependent on java reflection but we can start rewriting some pieces of code to avoid it