You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "David Costanzo (Jira)" <ji...@apache.org> on 2021/02/01 22:19:00 UTC
[jira] [Comment Edited] (JEXL-343) MethodExecutor ctor has strange
null check before calling MethodKey.isVarArgs()
[ https://issues.apache.org/jira/browse/JEXL-343?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17276694#comment-17276694 ]
David Costanzo edited comment on JEXL-343 at 2/1/21, 10:18 PM:
---------------------------------------------------------------
I removed a bogus section from the description about most of my program calling zero-parameter methods as well as my baseless claim that MethodKey.isVarArgs() wasn't written for performance and my suggestion to catch the return value of MethodKey.isVarArgs().
I subsequently determined that my JEXL programmers call a variety of methods:
||Total Calls||Method||
|8864420|java.util.regex.Pattern.compile|
|14380624|java.lang.Class.forName|
|5516204|java.lang.Class.of|
|8274276|java.lang.String.format|
|5516184|java.lang.String.matches|
|121356928|java.lang.String.replace|
|11032448|java.lang.String.replaceAll|
|2758092|java.lang.String.toLowerCase|
|2758112|java.math.BigDecimal.add|
|22064796|java.math.BigDecimal.intValue|
|2758045|java.math.BigDecimal.multiply|
|4485347|java.math.BigDecimal.remainder|
|1629691|java.math.BigDecimal.scale|
|4422486|java.math.BigDecimal.setScale|
|1727235|java.math.BigDecimal.stripTrailingZeros|
|2758112|java.math.BigDecimal.toBigInteger|
|1190278|java.math.BigDecimal.toPlainString|
|5516184|java.time.LocalDate.toEpochDay|
|2758092|java.util.HashMap.containsKey|
|24822888|java.util.regex.Matcher.group|
|8864420|java.util.regex.Matcher.matches|
|8864420|java.util.regex.Pattern.matcher|
In looking at Method.isVarArgs(), I realized that it could include this:
{noformat}
if (ptypes.length == 0) {
return false;
}{noformat}
either before or after this:
{noformat}
if (ptypes.length > 0 && ptypes[ptypes.length - 1].getComponentType() == null) {
return false;
}
{noformat}
was (Author: david_costanzo):
I removed a bogus section from the description about most of my program calling zero-parameter methods as was as my baseless claim that MethodKey.isVarArgs() wasn't written for performance. I subsequently determined that this is not true, just that it calls such methods a lot.
||Total Calls||Method||
|8864420|java.util.regex.Pattern.compile|
|14380624|java.lang.Class.forName|
|5516204|java.lang.Class.of|
|8274276|java.lang.String.format|
|5516184|java.lang.String.matches|
|121356928|java.lang.String.replace|
|11032448|java.lang.String.replaceAll|
|2758092|java.lang.String.toLowerCase|
|2758112|java.math.BigDecimal.add|
|22064796|java.math.BigDecimal.intValue|
|2758045|java.math.BigDecimal.multiply|
|4485347|java.math.BigDecimal.remainder|
|1629691|java.math.BigDecimal.scale|
|4422486|java.math.BigDecimal.setScale|
|1727235|java.math.BigDecimal.stripTrailingZeros|
|2758112|java.math.BigDecimal.toBigInteger|
|1190278|java.math.BigDecimal.toPlainString|
|5516184|java.time.LocalDate.toEpochDay|
|2758092|java.util.HashMap.containsKey|
|24822888|java.util.regex.Matcher.group|
|8864420|java.util.regex.Matcher.matches|
|8864420|java.util.regex.Pattern.matcher|
So it might be that Method.isVarArgs() could include this
> 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)