-
Notifications
You must be signed in to change notification settings - Fork 173
Description
As reported in #369, when using graphql-java-tools with Java 9+, a warning is printed on startup: An illegal reflective access operation has occurred...
This is due to the reflectasm library being not fully compatible with newer versions of Java.
The bug is already reported to reflectasm since 2018 (EsotericSoftware/reflectasm#64), but there seems to be little interest in having it fixed. The author of reflectasm says in one of the comments:
Sorry I'm too busy with other things ...
which may also indicate that reflectasm is not being actively maintained anymore.
The good news is that graphql-java-tools depends only lightly on reflectasm. In fact, the library is used in only one place, in MethodFieldResolver.kt
to invoke resolver methods.
What is the impact of using standard Java reflection instead of reflectasm?
- Performance?
Reflectasm claims to be 50% faster for method invocation than java reflection. However, the benchmark was run on JDK 7. Once we run it on newer versions of Java, we see a completely different picture:
- in Java 8, the difference is only around 25%
- in Java 11 reflection is actually faster than reflectasm. Maybe the official reflectasm benchmark is flawed (I have run it nevertheless multiple times). In any case the trend is clear and in practice I believe that there won't be any performance impact ...
- Cleaner stacktraces
Reflectasm generates a bit cleaner stacktraces. This can be mitigated by catching the InvocationTargetException and re-throwing thecause
(if it's aRuntimeException
or anError
), or wrapping it as a runtime exception (if it's checked).
Before:
java.lang.RuntimeException: Expected
at graphql.kickstart.tools.Query.throwsRuntimeException(EndToEndSpecHelper.kt:304)
at graphql.kickstart.tools.QueryMethodAccess.invoke(Unknown Source)
at graphql.kickstart.tools.MethodFieldResolverDataFetcher.get(MethodFieldResolver.kt:199)
at graphql.execution.ExecutionStrategy.fetchField(ExecutionStrategy.java:272)
...
After:
java.lang.RuntimeException: Expected
at graphql.kickstart.tools.Query.throwsRuntimeException(EndToEndSpecHelper.kt:304)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at graphql.kickstart.tools.MethodFieldResolverDataFetcher.get(MethodFieldResolver.kt:258)
at graphql.execution.ExecutionStrategy.fetchField(ExecutionStrategy.java:272)
...
The Java reflection stacktrace contains a few more lines, but it's clean enough nevertheless.
I will propose a pull request to remove reflectasm.