This is an automated email from the ASF dual-hosted git repository. emilles pushed a commit to branch GROOVY_4_0_X in repository https://gitbox.apache.org/repos/asf/groovy.git
commit ab5e1df0d8c141d88d923bff5994b15443c42649 Author: Eric Milles <[email protected]> AuthorDate: Thu May 5 09:50:06 2022 -0500 GROOVY-10535, GROOVY-10596: indy: cache [Bb]oolean cast for `null`, etc. --- .../v8/IndyGuardsFiltersAndSignatures.java | 52 +++++-------- .../org/codehaus/groovy/vmplugin/v8/Selector.java | 79 +++++++++----------- src/test/groovy/bugs/Groovy10535.groovy | 87 ++++++++++++++++++++++ 3 files changed, 142 insertions(+), 76 deletions(-) diff --git a/src/main/java/org/codehaus/groovy/vmplugin/v8/IndyGuardsFiltersAndSignatures.java b/src/main/java/org/codehaus/groovy/vmplugin/v8/IndyGuardsFiltersAndSignatures.java index ffdcc62d46..b358d6e4b9 100644 --- a/src/main/java/org/codehaus/groovy/vmplugin/v8/IndyGuardsFiltersAndSignatures.java +++ b/src/main/java/org/codehaus/groovy/vmplugin/v8/IndyGuardsFiltersAndSignatures.java @@ -54,61 +54,49 @@ import static org.codehaus.groovy.vmplugin.v8.IndyInterface.LOOKUP; public class IndyGuardsFiltersAndSignatures { private static final MethodType - ZERO_GUARD = MethodType.methodType(boolean.class), + OBJECT_FILTER = MethodType.methodType(Object.class, Object.class), OBJECT_GUARD = MethodType.methodType(boolean.class, Object.class), - CLASS1_GUARD = MethodType.methodType(boolean.class, Class.class, Object.class), - METACLASS1_GUARD = MethodType.methodType(boolean.class, MetaClass.class, Object.class), - - GRE_GUARD = MethodType.methodType(Object.class, GroovyRuntimeException.class), - - OBJECT_FILTER = MethodType.methodType(Object.class, Object.class), - - BOUND_INVOKER = MethodType.methodType(Object.class, Object[].class), - ANO_INVOKER = MethodType.methodType(Object.class, Object.class, Object[].class), - INVOKER = MethodType.methodType(Object.class, Object.class, String.class, Object[].class), - GET_INVOKER = MethodType.methodType(Object.class, String.class); + INVOKER = MethodType.methodType(Object.class, Object.class, String.class, Object[].class); protected static final MethodHandle - SAME_CLASS, UNWRAP_METHOD, - SAME_MC, IS_NULL, - UNWRAP_EXCEPTION, META_METHOD_INVOKER, - GROOVY_OBJECT_INVOKER, GROOVY_OBJECT_GET_PROPERTY, + SAME_CLASS, SAME_MC, IS_NULL, + UNWRAP_METHOD, UNWRAP_EXCEPTION, HAS_CATEGORY_IN_CURRENT_THREAD_GUARD, + META_METHOD_INVOKER, GROOVY_OBJECT_INVOKER, GROOVY_OBJECT_GET_PROPERTY, + META_CLASS_INVOKE_STATIC_METHOD, BEAN_CONSTRUCTOR_PROPERTY_SETTER, META_PROPERTY_GETTER, - SLOW_META_CLASS_FIND, META_CLASS_INVOKE_STATIC_METHOD, + SLOW_META_CLASS_FIND, MOP_GET, MOP_INVOKE_CONSTRUCTOR, MOP_INVOKE_METHOD, INTERCEPTABLE_INVOKER, - CLASS_FOR_NAME, BOOLEAN_IDENTITY, + BOOLEAN_IDENTITY, CLASS_FOR_NAME, DTT_CAST_TO_TYPE, SAM_CONVERSION, - HASHSET_CONSTRUCTOR, ARRAYLIST_CONSTRUCTOR, GROOVY_CAST_EXCEPTION, + HASHSET_CONSTRUCTOR, ARRAYLIST_CONSTRUCTOR, + GROOVY_CAST_EXCEPTION, EQUALS; static { try { - SAME_CLASS = LOOKUP.findStatic(IndyGuardsFiltersAndSignatures.class, "sameClass", CLASS1_GUARD); - UNWRAP_METHOD = LOOKUP.findStatic(IndyGuardsFiltersAndSignatures.class, "unwrap", OBJECT_FILTER); - SAME_MC = LOOKUP.findStatic(IndyGuardsFiltersAndSignatures.class, "isSameMetaClass", METACLASS1_GUARD); + SAME_CLASS = LOOKUP.findStatic(IndyGuardsFiltersAndSignatures.class, "sameClass", MethodType.methodType(boolean.class, Class.class, Object.class)); + SAME_MC = LOOKUP.findStatic(IndyGuardsFiltersAndSignatures.class, "isSameMetaClass", MethodType.methodType(boolean.class, MetaClass.class, Object.class)); IS_NULL = LOOKUP.findStatic(IndyGuardsFiltersAndSignatures.class, "isNull", OBJECT_GUARD); - UNWRAP_EXCEPTION = LOOKUP.findStatic(IndyGuardsFiltersAndSignatures.class, "unwrap", GRE_GUARD); + UNWRAP_METHOD = LOOKUP.findStatic(IndyGuardsFiltersAndSignatures.class, "unwrap", OBJECT_FILTER); + UNWRAP_EXCEPTION = LOOKUP.findStatic(IndyGuardsFiltersAndSignatures.class, "unwrap", MethodType.methodType(Object.class, GroovyRuntimeException.class)); + HAS_CATEGORY_IN_CURRENT_THREAD_GUARD = LOOKUP.findStatic(GroovyCategorySupport.class, "hasCategoryInCurrentThread", MethodType.methodType(boolean.class)); + META_METHOD_INVOKER = LOOKUP.findVirtual(MetaMethod.class, "doMethodInvoke", MethodType.methodType(Object.class, Object.class, Object[].class)); GROOVY_OBJECT_INVOKER = LOOKUP.findStatic(IndyGuardsFiltersAndSignatures.class, "invokeGroovyObjectInvoker", INVOKER.insertParameterTypes(0, MissingMethodException.class)); - - META_METHOD_INVOKER = LOOKUP.findVirtual(MetaMethod.class, "doMethodInvoke", ANO_INVOKER); - HAS_CATEGORY_IN_CURRENT_THREAD_GUARD = LOOKUP.findStatic(GroovyCategorySupport.class, "hasCategoryInCurrentThread", ZERO_GUARD); - GROOVY_OBJECT_GET_PROPERTY = LOOKUP.findVirtual(GroovyObject.class, "getProperty", GET_INVOKER); + GROOVY_OBJECT_GET_PROPERTY = LOOKUP.findVirtual(GroovyObject.class, "getProperty", MethodType.methodType(Object.class, String.class)); META_CLASS_INVOKE_STATIC_METHOD = LOOKUP.findVirtual(MetaObjectProtocol.class, "invokeStaticMethod", INVOKER); - BEAN_CONSTRUCTOR_PROPERTY_SETTER = LOOKUP.findStatic(IndyGuardsFiltersAndSignatures.class, "setBeanProperties", MethodType.methodType(Object.class, MetaClass.class, Object.class, Map.class)); META_PROPERTY_GETTER = LOOKUP.findVirtual(MetaProperty.class, "getProperty", OBJECT_FILTER); + SLOW_META_CLASS_FIND = LOOKUP.findStatic(InvokerHelper.class, "getMetaClass", MethodType.methodType(MetaClass.class, Object.class)); MOP_GET = LOOKUP.findVirtual(MetaObjectProtocol.class, "getProperty", MethodType.methodType(Object.class, Object.class, String.class)); - MOP_INVOKE_CONSTRUCTOR = LOOKUP.findVirtual(MetaObjectProtocol.class, "invokeConstructor", BOUND_INVOKER); + MOP_INVOKE_CONSTRUCTOR = LOOKUP.findVirtual(MetaObjectProtocol.class, "invokeConstructor", MethodType.methodType(Object.class, Object[].class)); MOP_INVOKE_METHOD = LOOKUP.findVirtual(MetaObjectProtocol.class, "invokeMethod", INVOKER); - SLOW_META_CLASS_FIND = LOOKUP.findStatic(InvokerHelper.class, "getMetaClass", MethodType.methodType(MetaClass.class, Object.class)); INTERCEPTABLE_INVOKER = LOOKUP.findVirtual(GroovyObject.class, "invokeMethod", MethodType.methodType(Object.class, String.class, Object.class)); - CLASS_FOR_NAME = LOOKUP.findStatic(Class.class, "forName", MethodType.methodType(Class.class, String.class, boolean.class, ClassLoader.class)); - BOOLEAN_IDENTITY = MethodHandles.identity(Boolean.class); + CLASS_FOR_NAME = LOOKUP.findStatic(Class.class, "forName", MethodType.methodType(Class.class, String.class, boolean.class, ClassLoader.class)); DTT_CAST_TO_TYPE = LOOKUP.findStatic(DefaultTypeTransformation.class, "castToType", MethodType.methodType(Object.class, Object.class, Class.class)); SAM_CONVERSION = LOOKUP.findStatic(CachedSAMClass.class, "coerceToSAM", MethodType.methodType(Object.class, Closure.class, Method.class, Class.class)); HASHSET_CONSTRUCTOR = LOOKUP.findConstructor(HashSet.class, MethodType.methodType(void.class, Collection.class)); diff --git a/src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java b/src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java index c294e94d30..5a0ccd516a 100644 --- a/src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java +++ b/src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java @@ -68,7 +68,6 @@ import java.util.HashSet; import static org.codehaus.groovy.vmplugin.v8.IndyGuardsFiltersAndSignatures.ARRAYLIST_CONSTRUCTOR; import static org.codehaus.groovy.vmplugin.v8.IndyGuardsFiltersAndSignatures.BEAN_CONSTRUCTOR_PROPERTY_SETTER; -import static org.codehaus.groovy.vmplugin.v8.IndyGuardsFiltersAndSignatures.BOOLEAN_IDENTITY; import static org.codehaus.groovy.vmplugin.v8.IndyGuardsFiltersAndSignatures.CLASS_FOR_NAME; import static org.codehaus.groovy.vmplugin.v8.IndyGuardsFiltersAndSignatures.DTT_CAST_TO_TYPE; import static org.codehaus.groovy.vmplugin.v8.IndyGuardsFiltersAndSignatures.EQUALS; @@ -162,16 +161,15 @@ public abstract class Selector { private final Class<?> staticSourceType, staticTargetType; public CastSelector(MutableCallSite callSite, Object[] arguments) { - super(callSite, Selector.class, "", CallType.CAST, false, false, false, arguments); + super(callSite, Selector.class, "", CallType.CAST, Boolean.FALSE, Boolean.FALSE, Boolean.FALSE, arguments); this.staticSourceType = callSite.type().parameterType(0); this.staticTargetType = callSite.type().returnType(); } @Override public void setCallSiteTarget() { - // targetTypes String, Enum and Class are handled - // by the compiler already - // Boolean / boolean + // target types String, Enum and Class are handled by the compiler + handleBoolean(); handleNullWithoutBoolean(); @@ -256,29 +254,25 @@ public abstract class Selector { private void handleBoolean() { if (handle != null) return; + boolean primitive = (staticTargetType == boolean.class); + if (!primitive && staticTargetType != Boolean.class) return; + // boolean->boolean, Boolean->boolean, boolean->Boolean are handled by the compiler + // which leaves (T)Z and (T)Boolean, where T is the static type but runtime type of T might be Boolean - // boolean->boolean, Boolean->boolean, boolean->Boolean - // is handled by compiler - // that leaves (T)Z and (T)Boolean, where T is the static type - // but runtime type of T might be Boolean + MethodHandle ifNull = IS_NULL.asType(MethodType.methodType(boolean.class, staticSourceType)); - boolean primitive = staticTargetType == boolean.class; - if (!primitive && staticTargetType != Boolean.class) return; - if (args[0] == null) { - if (primitive) { - handle = MethodHandles.constant(boolean.class, false); - handle = MethodHandles.dropArguments(handle, 0, staticSourceType); - } else { - handle = BOOLEAN_IDENTITY; - } - } else if (args[0] instanceof Boolean) { - // give value through or unbox - handle = BOOLEAN_IDENTITY; - } else { - //call asBoolean - name = "asBoolean"; - super.setCallSiteTarget(); + MethodHandle thenZero; + if (primitive) { // false + thenZero = MethodHandles.dropArguments(MethodHandles.constant(boolean.class, Boolean.FALSE), 0, staticSourceType); + } else { // (Boolean)null + thenZero = MethodHandles.identity(staticSourceType).asType(MethodType.methodType(Boolean.class, staticSourceType)); } + + name = "asBoolean"; + super.setCallSiteTarget(); + MethodHandle elseCallAsBoolean = handle; + + handle = MethodHandles.guardWithTest(ifNull, thenZero, elseCallAsBoolean); } } @@ -338,7 +332,7 @@ public abstract class Selector { if (Modifier.isStatic(f.getModifiers())) { // normally we would do the following // handle = MethodHandles.dropArguments(handle,0,Class.class); - // but because there is a bug in invokedynamic in all jdk7 versions + // but because there is a bug in invokedynamic in all jdk7 versions // maybe use Unsafe.ensureClassInitialized handle = META_PROPERTY_GETTER.bindTo(res); } @@ -403,8 +397,8 @@ public abstract class Selector { */ @Override public MetaClass getMetaClass() { - Object receiver = args[0]; - mc = GroovySystem.getMetaClassRegistry().getMetaClass((Class<?>) receiver); + mc = GroovySystem.getMetaClassRegistry().getMetaClass((Class<?>) args[0]); + if (LOG_ENABLED) LOG.info("meta class is " + mc); return mc; } @@ -447,7 +441,7 @@ public abstract class Selector { super.setHandleForMetaMethod(); } if (beanConstructor) { - // we have handle that takes no arguments to create the bean, + // we have handle that takes no arguments to create the bean, // we have to use its return value to call #setBeanProperties with it // and the meta class. @@ -508,8 +502,8 @@ public abstract class Selector { */ private static class MethodSelector extends Selector { private static final Object[] SINGLE_NULL_ARRAY = {null}; - protected MetaClass mc; private boolean isCategoryMethod; + protected MetaClass mc; public MethodSelector(MutableCallSite callSite, Class<?> sender, String methodName, CallType callType, Boolean safeNavigation, Boolean thisCall, Boolean spreadCall, Object[] arguments) { this.callType = callType; @@ -579,7 +573,7 @@ public abstract class Selector { this.cache &= !ClassInfo.getClassInfo(receiver.getClass()).hasPerInstanceMetaClasses(); } mc.initialize(); - + if (LOG_ENABLED) LOG.info("meta class is " + mc); return mc; } @@ -648,7 +642,7 @@ public abstract class Selector { handle = correctClassForNameAndUnReflectOtherwise(m); if (LOG_ENABLED) LOG.info("successfully unreflected method"); if (isStaticCategoryTypeMethod) { - handle = MethodHandles.insertArguments(handle, 0, new Object[]{null}); + handle = MethodHandles.insertArguments(handle, 0, SINGLE_NULL_ARRAY); handle = MethodHandles.dropArguments(handle, 0, targetType.parameterType(0)); } else if (!isCategoryTypeMethod && isStatic(m)) { // we drop the receiver, which might be a Class (invocation on Class) @@ -717,7 +711,7 @@ public abstract class Selector { if (receiver instanceof GroovyObject) { // if the meta class call fails we may still want to fall back to call // GroovyObject#invokeMethod if the receiver is a GroovyObject - if (LOG_ENABLED) LOG.info("add MissingMethod handler for GrooObject#invokeMethod fallback path"); + if (LOG_ENABLED) LOG.info("add MissingMethod handler for GroovyObject#invokeMethod fallback path"); handle = MethodHandles.catchException(handle, MissingMethodException.class, GROOVY_OBJECT_INVOKER); } } @@ -775,14 +769,14 @@ public abstract class Selector { // so we really need to rewrap handle = handle.asCollector(lastParam, 1); } else if (params.length > args.length) { - // we depend on the method selection having done a good + // we depend on the method selection having done a good // job before already, so the only case for this here is, that // we have no argument for the array, meaning params.length is // args.length+1. In that case we have to fill in an empty array handle = MethodHandles.insertArguments(handle, params.length - 1, Array.newInstance(lastParam.getComponentType(), 0)); if (LOG_ENABLED) LOG.info("added empty array for missing vargs part"); } else { //params.length < args.length - // we depend on the method selection having done a good + // we depend on the method selection having done a good // job before already, so the only case for this here is, that // all trailing arguments belong into the vargs array handle = handle.asCollector( @@ -815,8 +809,8 @@ public abstract class Selector { // the argument is an instance of the parameter type. We also // exclude boxing, since the MethodHandles will do that part // already for us. Another case is the conversion of a primitive - // to another primitive or of the wrappers, or a combination of - // these. This is also handled already. What is left is the + // to another primitive or of the wrappers, or a combination of + // these. This is also handled already. What is left is the // GString conversion and the number conversions. if (arg == null) continue; @@ -830,7 +824,7 @@ public abstract class Selector { if (wrappedPara == TypeHelper.getWrapperClass(got)) continue; // equal in terms of an assignment in Java. That means according to Java widening rules, or - // a subclass, interface, superclass relation, this case then handles also + // a subclass, interface, superclass relation, this case then handles also // primitive to primitive conversion. Those case are also solved by explicitCastArguments. if (parameters[i].isAssignableFrom(got)) continue; @@ -863,7 +857,7 @@ public abstract class Selector { */ public void addExceptionHandler() { //TODO: if we would know exactly which paths require the exceptions - // and which paths not, we can sometimes save this guard + // and which paths not, we can sometimes save this guard if (handle == null || !catchException) return; Class<?> returnType = handle.type().returnType(); if (returnType != Object.class) { @@ -973,8 +967,6 @@ public abstract class Selector { public void setSelectionBase() { if (thisCall) { selectionBase = sender; - } else if (args[0] == null) { - selectionBase = NullObject.class; } else { selectionBase = mc.getTheClass(); } @@ -985,8 +977,8 @@ public abstract class Selector { * Sets a handle to call {@link GroovyInterceptable#invokeMethod(String, Object)} */ public boolean setInterceptor() { - if (!(this.args[0] instanceof GroovyInterceptable)) return false; - handle = MethodHandles.insertArguments(INTERCEPTABLE_INVOKER, 1, this.name); + if (!(args[0] instanceof GroovyInterceptable)) return false; + handle = MethodHandles.insertArguments(INTERCEPTABLE_INVOKER, 1, name); handle = handle.asCollector(Object[].class, targetType.parameterCount() - 1); handle = handle.asType(targetType); return true; @@ -1004,7 +996,6 @@ public abstract class Selector { public void setCallSiteTarget() { if (!setNullForSafeNavigation() && !setInterceptor()) { getMetaClass(); - if (LOG_ENABLED) LOG.info("meta class is " + mc); setSelectionBase(); MetaClassImpl mci = getMetaClassImpl(mc, callType != CallType.GET); chooseMeta(mci); diff --git a/src/test/groovy/bugs/Groovy10535.groovy b/src/test/groovy/bugs/Groovy10535.groovy new file mode 100644 index 0000000000..0e85c00f8b --- /dev/null +++ b/src/test/groovy/bugs/Groovy10535.groovy @@ -0,0 +1,87 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package groovy.bugs + +import org.junit.Test + +import static groovy.test.GroovyAssert.assertScript + +final class Groovy10535 { + + @Test + void testBooleanTypecast_invokeDynamicOptimization1() { + assertScript ''' + @groovy.transform.CompileStatic + class C { + static main(args) { + Collection<String> strings = null + for (int i = 0; i <= 200_000; i += 1) { // vs groovy.indy.optimize.threshold + assert test(strings) === null + } + strings = ['x'] + assert test(strings) !== null + } + static test(Collection<String> values) { + if (values) return 'thing' + } + } + ''' + } + + @Test + void testBooleanTypecast_invokeDynamicOptimization2() { + assertScript ''' + @groovy.transform.CompileStatic + class C { + static main(args) { + Collection<String> strings = ['x'] + for (int i = 0; i <= 200_000; i += 1) { + assert test(strings) !== null + } + strings = null + assert test(strings) === null + } + static test(Collection<String> values) { + if (values) return 'thing' + } + } + ''' + } + + @Test + void testBooleanTypecast_invokeDynamicOptimization3() { + assertScript ''' + @groovy.transform.CompileStatic + class C { + static main(args) { + Collection<String> strings + for (int i = 0; i <= 200_000; i += 1) { + strings = [i as String] + assert test(strings) !== null + } + strings = null + assert test(strings) === null + } + static test(Collection<String> values) { + if (values) return 'thing' + } + } + ''' + } +}
