[
https://issues.apache.org/jira/browse/JEXL-343?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
David Costanzo updated JEXL-343:
--------------------------------
Description:
When inspecting slow performance of my JEXL-enabled application, I came across
a strange line in {{MethodExecutor}}'s constructor that I think is a bug.
Specifically the {{formal != null}} is testing a condition that should never
happen because
[Method.getTypeParameters()|https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/reflect/Method.html#getTypeParameters()]
should never return null.
{noformat}
private MethodExecutor(Class<?> c, java.lang.reflect.Method m, MethodKey k) {
...
Class<?>[] formal = method.getParameterTypes();
// if the last parameter is an array, the method is considered as vararg
if (formal != null && MethodKey.isVarArgs(method)) { // <===== THIS LINE
vastart = formal.length - 1;
vaclass = formal[vastart].getComponentType();
}
...
}
{noformat}
I think {{formal != null}} is supposed to be {{formal.length != 0}}.
*Impact:*
Although this change may look trivial, my application completes in 25% less
time with it (one benchmark goes from 9:49 to 7:01).
For reference, this is the top of the call stack that takes 28% of the
execution time:
{noformat}
java.lang.Class.getMethod(String, Class[])
org.apache.commons.jexl3.internal.introspection.MethodKey.isVarArgs
(java.lang.reflect.Method)
org.apache.commons.jexl3.internal.introspection.MethodExecutor.<init>(Class,
java.lang.reflect.Method,
org.apache.commons.jexl3.internal.introspection.MethodKey)
org.apache.commons.jexl3.internal.introspection.MethodExecutor.discover(org.apache.commons.jexl3.internal.introspection.Introspector,
Object, String, Object[])
org.apache.commons.jexl3.internal.introspection.Uberspect.getMethod(Object,
String, Object[]){noformat}
I looked for a sanctioned way to apply the "formal.length != 0" change using
the provided hooks, but I couldn't find a good one. The code I want to change
is too deep within the "internal" package. My bad way of changing this code is
to use a custom {{Uberspect}} which calls a custom copy of {{MethodExecutor}}
which extends a custom copy {{AbstractExecutor}}.
was:
When inspecting slow performance of my JEXL-enabled application, I came across
a strange line in {{MethodExecutor}}'s constructor that I think is a bug.
Specifically the {{formal != null}} is testing a condition that should never
happen because
[Method.getTypeParameters()|https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/reflect/Method.html#getTypeParameters()]
should never return null.
{noformat}
private MethodExecutor(Class<?> c, java.lang.reflect.Method m, MethodKey k) {
...
Class<?>[] formal = method.getParameterTypes();
// if the last parameter is an array, the method is considered as vararg
if (formal != null && MethodKey.isVarArgs(method)) { // <===== THIS LINE
vastart = formal.length - 1;
vaclass = formal[vastart].getComponentType();
}
...
}
{noformat}
I think {{formal != null}} is supposed to be {{formal.length != 0}}.
*Impact:*
Although this change may look trivial, my application completes in 25% less
time with it (one benchmark goes from 9:49 to 7:01).
*Real Problem:*
This improvement ticket is a pretext for starting a conversation about a
related, deeper problem. Profiling shows that 28% of my application's execution
time is spent in {{MethodKey.isVarArgs()}}. Most of the time, my JEXL
programmers call simple methods that have no parameters, like
{{Instant.toString()}} or {{BigDecimal.toString()}}, but they do this a hundred
million times. Each time, JEXL walks the hierarchy of the same class looking
for the same method and determines that it's not a vararg method.
I suspect that performance wasn't considered when the hierarchy walking code
was added in {{MethodKey.isVarArgs()}}. While my change does improve
performance, it only helps for methods with no parameters. It would be better
if the "isVarArgs()" result could be cached somewhere, perhaps in
{{MethodKey}}. Or maybe {{MethodKey.isVarArgs()}} could be given the
{{Introspector}} for faster lookup of the class method.
For reference, this is the top of the call stack that takes 28% of the
execution time:
{noformat}
java.lang.Class.getMethod(String, Class[])
org.apache.commons.jexl3.internal.introspection.MethodKey.isVarArgs
(java.lang.reflect.Method)
org.apache.commons.jexl3.internal.introspection.MethodExecutor.<init>(Class,
java.lang.reflect.Method,
org.apache.commons.jexl3.internal.introspection.MethodKey)
org.apache.commons.jexl3.internal.introspection.MethodExecutor.discover(org.apache.commons.jexl3.internal.introspection.Introspector,
Object, String, Object[])
org.apache.commons.jexl3.internal.introspection.Uberspect.getMethod(Object,
String, Object[]){noformat}
I looked for a sanctioned way to apply the "formal.length != 0" change using
the provided hooks, but I couldn't find a good one. The code I want to change
is too deep within the "internal" package. My bad way of changing this code is
to use a custom {{Uberspect}} which calls a custom copy of {{MethodExecutor}}
which extends a custom copy {{AbstractExecutor}}.
> MethodExecutor ctor has strange null check before calling
> MethodKey.isVarArgs()
> -------------------------------------------------------------------------------
>
> Key: JEXL-343
> URL: https://issues.apache.org/jira/browse/JEXL-343
> Project: Commons JEXL
> Issue Type: Improvement
> Affects Versions: 3.1
> Environment: jdk-11.0.5_10-hotspot, GNU/Linux
> Reporter: David Costanzo
> Priority: Minor
>
> When inspecting slow performance of my JEXL-enabled application, I came
> across a strange line in {{MethodExecutor}}'s constructor that I think is a
> bug. Specifically the {{formal != null}} is testing a condition that should
> never happen because
> [Method.getTypeParameters()|https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/reflect/Method.html#getTypeParameters()]
> should never return null.
>
> {noformat}
> private MethodExecutor(Class<?> c, java.lang.reflect.Method m, MethodKey k) {
> ...
> Class<?>[] formal = method.getParameterTypes();
> // if the last parameter is an array, the method is considered as
> vararg
> if (formal != null && MethodKey.isVarArgs(method)) { // <===== THIS
> LINE
> vastart = formal.length - 1;
> vaclass = formal[vastart].getComponentType();
> }
> ...
> }
> {noformat}
>
> I think {{formal != null}} is supposed to be {{formal.length != 0}}.
>
> *Impact:*
> Although this change may look trivial, my application completes in 25% less
> time with it (one benchmark goes from 9:49 to 7:01).
>
> For reference, this is the top of the call stack that takes 28% of the
> execution time:
> {noformat}
> java.lang.Class.getMethod(String, Class[])
> org.apache.commons.jexl3.internal.introspection.MethodKey.isVarArgs
> (java.lang.reflect.Method)
> org.apache.commons.jexl3.internal.introspection.MethodExecutor.<init>(Class,
> java.lang.reflect.Method,
> org.apache.commons.jexl3.internal.introspection.MethodKey)
> org.apache.commons.jexl3.internal.introspection.MethodExecutor.discover(org.apache.commons.jexl3.internal.introspection.Introspector,
> Object, String, Object[])
> org.apache.commons.jexl3.internal.introspection.Uberspect.getMethod(Object,
> String, Object[]){noformat}
>
> I looked for a sanctioned way to apply the "formal.length != 0" change using
> the provided hooks, but I couldn't find a good one. The code I want to change
> is too deep within the "internal" package. My bad way of changing this code
> is to use a custom {{Uberspect}} which calls a custom copy of
> {{MethodExecutor}} which extends a custom copy {{AbstractExecutor}}.
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
