-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Replace core reflection usage with MethodHandles #5046
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
Replace core reflection usage with MethodHandles #5046
Conversation
src/main/java/tools/jackson/databind/introspect/AnnotatedCreatorCollector.java
Show resolved
Hide resolved
3c91514 to
efa845a
Compare
src/test/java/tools/jackson/databind/mixins/MixinForCreators2795Test.java
Show resolved
Hide resolved
|
Also looks like Android compatibility is broken? (wrt animal-sniffer failure for JDK 17 build) |
|
Java 17 compile issues |
I believe those animal sniffer warnings are erroneous & can be suppressed: mojohaus/animal-sniffer#67 also the min android sdk for MethodHandles is 26, which is already applied |
Can't we increase the min Android SDK? |
To clarify, jackson 3 declares compatibility with sdk 34+ while 2.14 - 2.18 declares sdk 26+, both of which support MethodHandles - I don't follow your question |
|
I'd prefer to find out from animal-sniffer team why these methods are causing issues for it instead of just ignoring the issues that animal sniffer seems to think there are. |
|
Maybe there are newer versions of Animal sniffer configs (dep version from pom.xml)? |
|
A new sniffer config is not sufficient, as PolymorphicSignature methods can have an unlimited number of signatures. It needs real plugin support. I also don't think we can expect a response from the maintainers anytime soon, judging by that issue and the linked issues. Netty even removed the sniffer plugin because of it: netty/netty#14032 I think the the only sensible path forward is to add an exclusion for MethodHandle for now |
|
I added an ignore for MethodHandle. I checked the definition and it seems that the method is part of the java spec since Java 9. Surely Android is past Java 9 baseline at this point - should I add another ignore for this one too? |
f655493 to
6af6725
Compare
android doesn't seem to implement that method on any SDK: https://developer.android.com/reference/java/lang/reflect/AccessibleObject you'll have to switch to |
|
Rats, that sucks. I'll make that update... |
|
Ok, I reworked the |
|
WTF, Android does not implement |
b56e779 to
12f909a
Compare
Hmm. Ok, that might be ok. Was thinking that maybe "sun.*" was not checked as system class by I think it'd be good to try to avoid these calls to be sure (instead of having to catch exception) but that might be yet bigger undertaking. |
Yeah, that's why I tried to replace it with The good news is this only happens during inspection, not at deserialization time, so the performance impact would be muted. |
|
@stevenschlansker Excellent work! I am bit hesitant to merge this before 3.0.0-rc2, but could be merged right after -- esp. assuming we could get some performance numbers, maybe for https://github.com/FasterXML/jackson-benchmarks/ (I'll see if I could run some myself). EDIT: from quick test runs, it looks like there's maybe +5% for regular JSON read via databind for this PR vs 3.0.0-rc1. Hoping to test out writes too. |
|
Maybe you can use a method handle to call trySetAccessible 😄 |
806c369 to
5d52967
Compare
|
Thanks @cowtowncoder I am excited to finally get this work in :)
That's fine by me. Were I making the call, I'd think that having the change in users' hands in an earlier RC for more testing would be better than delaying merging and finding problems closer to release - but again, 100% your call :)
I'm not too surprised. There was a time, a long time ago, when reflection was quite slow. The JVM has come a long way and now reflective calls are much, much faster than they used to be. That said, 5% is 5%, so I will take it :) especially since it comes with little downside.
@yawkat yes, that would work :) I think since this is called only during (de)serializer instantiation, though, it's not worth the effort for now. |
|
@stevenschlansker Yeah, I plan on having quite a few RCs out, so by "not next" just means 2-3 week delay. Sort of to isolate things. But I get the point of as-early-as-possible wrt weeding out bugs. On performance, yeah, +5% is measurable and has some value. But odd thing is this -- now testing serialization I see no difference whatsoever. That is... odd. Would suggest deserialization speed up would have something to do with setters (parameter) maybe? Finally: one question on MethodHandles -- I noticed removal of some Field-based classes. I assume field access still exists; how is that now handled? |
src/main/java/tools/jackson/databind/introspect/AnnotatedMethod.java
Outdated
Show resolved
Hide resolved
MethodHandles unify method calls and field get / set with unreflectGetter |
15ecab2 to
5fbc064
Compare
It can be really hard to nail down performance on this kind of thing... my understanding from reading the literature is that it should help, but maybe hard to see among all the other work any particular workload does. It's possible different shapes have different costs. For example when deserializing, |
| // 08-Sep-2016, tatu: wonder if we should verify it is `AnnotatedField` to be safe? | ||
| prop = new FieldProperty(propDef, type, typeDeser, | ||
| beanDesc.getClassAnnotations(), (AnnotatedField) mutator); | ||
| if (!ClassUtil.checkAndFixAccess(mutator.getMember(), ctxt.getConfig().isEnabled(MapperFeature.OVERRIDE_PUBLIC_ACCESS_MODIFIERS))) { |
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.
I think ctxt.getConfig().isEnabled(...) can be shortened to ctxt.isEnabled(...) (there should be convenience accessor).
But aside from that, could you add a brief comment explaining reasons why code short-circuits here?
(cannot access I assume but... is that then quietly swallowing cases causing surprises?)
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.
Added a brief comment. I agree, some conditions regarding module visibility could quietly swallow members here, if we cannot call setAccessible on them. I think in those cases though the whole operation will fail anyway until you allow Jackson module to open your module with beans in it.
There might be some follow-up tweaking to this logic necessary. I got all the existing tests passing but I am sure there are new exciting corner cases to discover with module visibility.
Come to think of it, can we remove the MapperFeature.OVERRIDE_PUBLIC_ACCESS_MODIFIERS now? I believe with MethodHandles used, there should no longer be any performance benefit to calling setAccessible unconditionally. I am happy to file a follow-up PR if this is acceptable. If not, we could consider turning it off by default now.
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.
Good idea -- I'll file an issue for changing defaults, will merge this PR and we'll go from there.
Actual removal of that feature is an option too, but at least start by changing its
default.
EDIT: issue -> #5074
src/main/java/tools/jackson/databind/deser/BeanDeserializerFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/tools/jackson/databind/deser/impl/MethodProperty.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| constructors.removeIf(constructor -> | ||
| constructor == null || (_collectAnnotations && _intr.hasIgnoreMarker(_config, constructor))); |
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.
Why is null check needed? It wasn't there earlier?
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 now necessary because access checks have been pushed up from the point of use (when constructor is called) to earlier in deserializer creation (when constructors and factories are collected) so they are now filtered out earlier too. I added a comment.
cowtowncoder
left a comment
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.
Looks pretty good, only minor final touches and I can merge it.
364448b to
3e2d96b
Compare
|
@stevenschlansker Ok, looks like this also broke most downstream modules... :-( Will file issues, add a notes/refs here. |
|
|
Ugh. Seems like almost everything downstream fails, more or less catastrophically. Will likely need to revert this PR later tonight. |
|
😢 |
|
I'll try to find some time in the next week or two to check out the downstream modules and get those working. |
I have been trying to think of that over time and haven't thought of good maintainable solution. Resource-wise full rebuild job for all PRs just for databind might be doable, although not sure how heavy resource-wise it'd be wrt Github charging (FasterXML has paid, not free, account). |
|
Will revert now. |
)" This reverts commit b47a371.
|
Reverted for now. Although failure was wide, it is possible there might be just small number of actual issues so maybe focusing on solving problems by just one downstream repo (like |
| // 08-Sep-2016, tatu: wonder if we should verify it is `AnnotatedField` to be safe? | ||
| prop = new FieldProperty(propDef, type, typeDeser, | ||
| beanDesc.getClassAnnotations(), (AnnotatedField) mutator); | ||
| // 06-04-2025, scs: we cannot always see members if e.g. they are in a different module that does not |
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.
Do we need to take a performance hit here? Why not let this fail if access is not setup? Null seems like a hack to me. The Java exceptions are fairly informative about the right add-opens are needed. @cowtowncoder wdyt? This looks like something we could do without. I only have a mobile to check this so the small screen view might not be the right way to check this PR.
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.
Not sure what performance hit you are thinking -- this is not perf sensitive path (it's during deserializer construction).
I agree it seems odd to return null but I don't remember reasoning over straight failure.
@stevenschlansker is author here.
One thing to note tho is that ClassUtil.checkAndFixAccess just blindly overrides access if I recall and does not attempt to determine if call would likely fail -- it is just based on configuration settings. That could probably be improved, but we cannot test call here (we are not deserializing, we are building deserializer) to make sure.
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.
From memory, the reason for this is because the access checks moved from "Method invoke time" to "MethodHandle create time" - this is one of the potential benefits of MH, lifting access / visibility checks from each-invocation to once-at-creation.
Some JDK classes in the modular world are not visible to user code - you're not allowed to crack open java.lang anymore, for example, without various --add-opens directives.
Jackson creates deserializers for types that are not used in some cases (I don't recall which) but basically this is skipping a failure in the case where we see some JDK type that we cannot crack open (Date IIRC?), but it does not matter, because the deserializer does not get used. So we skip an irrelevant failure.
I agree null is a bit weird. Improvements welcome, but the code is that way for a reason at least :)
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. @pjfanning if you wanted to try a PR with explicit fail, we could see if any of unit tests fail? (or just test locally).
@stevenschlansker Yeah I do not doubt there was (is) a reason (I vaguely recall discussions). But also do not remember context well enough to know the "right way" to go.
This should improve performance due to the new design of method handles.
Core reflection has to evaluate everything at the time that you call
method.invoke- it has to do access checks, determine what parameter conversions are necessary, etc - and does not have any local state to save the result of that work.With MethodHandle, you do more of the work up-front - determine access, determine what parameter boxing or casts are necessary - and store that computed state in a new MethodHandle. Later, when you
invokeExact, the method handle is able to make the call much faster.