This is an automated email from the ASF dual-hosted git repository. paulk pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/groovy.git
commit 7b58e57941ddf587feaba0d80470c2a7a9c45476 Author: Paul King <[email protected]> AuthorDate: Wed May 3 15:29:58 2023 +1000 GROOVY-11040: Optimisations for bytecode for record generated methods --- .../java/groovy/transform/EqualsAndHashCode.java | 7 ++++ src/main/java/groovy/transform/ToString.java | 7 ++++ .../codehaus/groovy/ast/tools/GeneralUtils.java | 4 +- .../EqualsAndHashCodeASTTransformation.java | 46 ++++++++++++++------- .../transform/RecordTypeASTTransformation.java | 6 +-- .../transform/ToStringASTTransformation.java | 15 ++++--- .../CanonicalComponentsTransformTest.groovy | 48 +++++++++++++++++++++- 7 files changed, 106 insertions(+), 27 deletions(-) diff --git a/src/main/java/groovy/transform/EqualsAndHashCode.java b/src/main/java/groovy/transform/EqualsAndHashCode.java index 5a0fe83307..ca672b18a1 100644 --- a/src/main/java/groovy/transform/EqualsAndHashCode.java +++ b/src/main/java/groovy/transform/EqualsAndHashCode.java @@ -296,4 +296,11 @@ public @interface EqualsAndHashCode { * @since 4.0.0 */ boolean pojo() default false; + + /** + * Whether to access properties directly by their fields (faster) or via their getters. + * + * @since 4.0.12 + */ + boolean useGetters() default true; } diff --git a/src/main/java/groovy/transform/ToString.java b/src/main/java/groovy/transform/ToString.java index 53ecb3f0b5..81f2160341 100644 --- a/src/main/java/groovy/transform/ToString.java +++ b/src/main/java/groovy/transform/ToString.java @@ -391,4 +391,11 @@ public @interface ToString { * @since 4.0.0 */ String fieldSeparator() default ", "; + + /** + * Whether to access properties directly by their fields (faster) or via their getters. + * + * @since 4.0.12 + */ + boolean useGetters() default true; } diff --git a/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java b/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java index f253744e95..c5439e899f 100644 --- a/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java +++ b/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java @@ -607,8 +607,8 @@ public class GeneralUtils { return eqX(varX(fNode), propX(other, fNode.getName())); } - public static BinaryExpression hasEqualPropertyX(final ClassNode annotatedNode, final PropertyNode pNode, final VariableExpression other) { - return eqX(getterThisX(annotatedNode, pNode), getterX(other.getOriginType(), other, pNode)); + public static BinaryExpression hasEqualPropertyX(final ClassNode cNode, final PropertyNode pNode, final VariableExpression other) { + return eqX(getterThisX(cNode, pNode), getterX(other.getOriginType(), other, pNode)); } @Deprecated diff --git a/src/main/java/org/codehaus/groovy/transform/EqualsAndHashCodeASTTransformation.java b/src/main/java/org/codehaus/groovy/transform/EqualsAndHashCodeASTTransformation.java index dfa4193ecb..0d5bd80d67 100644 --- a/src/main/java/org/codehaus/groovy/transform/EqualsAndHashCodeASTTransformation.java +++ b/src/main/java/org/codehaus/groovy/transform/EqualsAndHashCodeASTTransformation.java @@ -43,6 +43,7 @@ import org.codehaus.groovy.transform.stc.StaticTypeCheckingSupport; import org.codehaus.groovy.util.HashCodeHelper; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashSet; import java.util.List; import java.util.Objects; @@ -99,6 +100,7 @@ public class EqualsAndHashCodeASTTransformation extends AbstractASTTransformatio private static final ClassNode HASHUTIL_TYPE = make(HashCodeHelper.class); private static final ClassNode POJO_TYPE = make(POJO.class); private static final ClassNode OBJECTS_TYPE = make(Objects.class); + private static final ClassNode ARRAYS_TYPE = make(Arrays.class); private static final ClassNode OBJECT_TYPE = makeClassSafe(Object.class); private static final String HASH_CODE = "hashCode"; private static final String UNDER_HASH_CODE = "_hashCode"; @@ -133,6 +135,7 @@ public class EqualsAndHashCodeASTTransformation extends AbstractASTTransformatio addError("Error during " + MY_TYPE_NAME + " processing: callSuper=true but '" + cNode.getName() + "' has no super class.", anno); } boolean includeFields = memberHasValue(anno, "includeFields", true); + boolean useGetter = !memberHasValue(anno, "useGetters", false); List<String> excludes = getMemberStringList(anno, "excludes"); List<String> includes = getMemberStringList(anno, "includes"); final boolean allNames = memberHasValue(anno, "allNames", true); @@ -140,8 +143,8 @@ public class EqualsAndHashCodeASTTransformation extends AbstractASTTransformatio if (!checkIncludeExcludeUndefinedAware(anno, excludes, includes, MY_TYPE_NAME)) return; if (!checkPropertyList(cNode, includes, "includes", anno, MY_TYPE_NAME, includeFields)) return; if (!checkPropertyList(cNode, excludes, "excludes", anno, MY_TYPE_NAME, includeFields)) return; - createHashCode(cNode, cacheHashCode, includeFields, callSuper, excludes, includes, allNames, allProperties, pojo); - createEquals(cNode, includeFields, callSuper, useCanEqual, excludes, includes, allNames, allProperties, pojo); + createHashCode(cNode, cacheHashCode, includeFields, callSuper, excludes, includes, allNames, allProperties, pojo, useGetter); + createEquals(cNode, includeFields, callSuper, useCanEqual, excludes, includes, allNames, allProperties, pojo, useGetter); } } @@ -158,6 +161,10 @@ public class EqualsAndHashCodeASTTransformation extends AbstractASTTransformatio } public static void createHashCode(ClassNode cNode, boolean cacheResult, boolean includeFields, boolean callSuper, List<String> excludes, List<String> includes, boolean allNames, boolean allProperties, boolean pojo) { + createHashCode(cNode, cacheResult, includeFields, callSuper, excludes, includes, allNames, allProperties, pojo, false); + } + + public static void createHashCode(ClassNode cNode, boolean cacheResult, boolean includeFields, boolean callSuper, List<String> excludes, List<String> includes, boolean allNames, boolean allProperties, boolean pojo, boolean useGetter) { // make a public method if none exists otherwise try a private method with leading underscore boolean hasExistingHashCode = hasDeclaredMethod(cNode, HASH_CODE, 0); if (hasExistingHashCode) { @@ -175,11 +182,11 @@ public class EqualsAndHashCodeASTTransformation extends AbstractASTTransformatio final Expression hash = varX(hashField); body.addStatement(ifS( isZeroX(hash), - calculateHashStatements(cNode, hash, includeFields, callSuper, excludes, includes, allNames, allProperties, pojo) + calculateHashStatements(cNode, hash, includeFields, callSuper, excludes, includes, allNames, allProperties, pojo, useGetter) )); body.addStatement(returnS(hash)); } else { - body.addStatement(calculateHashStatements(cNode, null, includeFields, callSuper, excludes, includes, allNames, allProperties, pojo)); + body.addStatement(calculateHashStatements(cNode, null, includeFields, callSuper, excludes, includes, allNames, allProperties, pojo, useGetter)); } addGeneratedMethod(cNode, @@ -191,14 +198,14 @@ public class EqualsAndHashCodeASTTransformation extends AbstractASTTransformatio body); } - private static Statement calculateHashStatements(ClassNode cNode, Expression hash, boolean includeFields, boolean callSuper, List<String> excludes, List<String> includes, boolean allNames, boolean allProperties, boolean pojo) { + private static Statement calculateHashStatements(ClassNode cNode, Expression hash, boolean includeFields, boolean callSuper, List<String> excludes, List<String> includes, boolean allNames, boolean allProperties, boolean pojo, boolean useGetter) { if (pojo) { - return calculateHashStatementsPOJO(cNode, hash, includeFields, callSuper, excludes, includes, allNames, allProperties); + return calculateHashStatementsPOJO(cNode, hash, includeFields, callSuper, excludes, includes, allNames, allProperties, useGetter); } - return calculateHashStatementsDefault(cNode, hash, includeFields, callSuper, excludes, includes, allNames, allProperties); + return calculateHashStatementsDefault(cNode, hash, includeFields, callSuper, excludes, includes, allNames, allProperties, useGetter); } - private static Statement calculateHashStatementsDefault(ClassNode cNode, Expression hash, boolean includeFields, boolean callSuper, List<String> excludes, List<String> includes, boolean allNames, boolean allProperties) { + private static Statement calculateHashStatementsDefault(ClassNode cNode, Expression hash, boolean includeFields, boolean callSuper, List<String> excludes, List<String> includes, boolean allNames, boolean allProperties, boolean useGetter) { final Set<String> names = new HashSet<>(); final List<PropertyNode> pList = getAllProperties(names, cNode, true, false, allProperties, false, false, false); final List<FieldNode> fList = new ArrayList<>(); @@ -213,10 +220,10 @@ public class EqualsAndHashCodeASTTransformation extends AbstractASTTransformatio for (PropertyNode pNode : pList) { if (shouldSkipUndefinedAware(pNode.getName(), excludes, includes, allNames)) continue; // _result = HashCodeHelper.updateHash(_result, getProperty()) // plus self-reference checking - Expression getter = getterThisX(cNode, pNode); - final Expression current = callX(HASHUTIL_TYPE, UPDATE_HASH, args(result, getter)); + Expression prop = useGetter ? getterThisX(cNode, pNode) : propX(varX("this"), pNode.getName()); + final Expression current = callX(HASHUTIL_TYPE, UPDATE_HASH, args(result, prop)); body.addStatement(ifS( - notIdenticalX(getter, varX("this")), + notIdenticalX(prop, varX("this")), assignS(result, current))); } @@ -243,7 +250,7 @@ public class EqualsAndHashCodeASTTransformation extends AbstractASTTransformatio return body; } - private static Statement calculateHashStatementsPOJO(ClassNode cNode, Expression hash, boolean includeFields, boolean callSuper, List<String> excludes, List<String> includes, boolean allNames, boolean allProperties) { + private static Statement calculateHashStatementsPOJO(ClassNode cNode, Expression hash, boolean includeFields, boolean callSuper, List<String> excludes, List<String> includes, boolean allNames, boolean allProperties, boolean useGetter) { final Set<String> names = new HashSet<>(); final List<PropertyNode> pList = getAllProperties(names, cNode, true, false, allProperties, false, false, false); final List<FieldNode> fList = new ArrayList<>(); @@ -254,7 +261,11 @@ public class EqualsAndHashCodeASTTransformation extends AbstractASTTransformatio final ArgumentListExpression args = new ArgumentListExpression(); for (PropertyNode pNode : pList) { if (shouldSkipUndefinedAware(pNode.getName(), excludes, includes, allNames)) continue; - args.addExpression(getterThisX(cNode, pNode)); + if (useGetter) { + args.addExpression(getterThisX(cNode, pNode)); + } else { + args.addExpression(propX(varX("this"), pNode.getName())); + } } for (FieldNode fNode : fList) { if (shouldSkipUndefinedAware(fNode.getName(), excludes, includes, allNames)) continue; @@ -263,7 +274,7 @@ public class EqualsAndHashCodeASTTransformation extends AbstractASTTransformatio if (callSuper) { args.addExpression(varX("super")); } - Expression calcHash = callX(OBJECTS_TYPE, "hash", args); + Expression calcHash = callX(ARRAYS_TYPE, HASH_CODE, args); if (hash != null) { body.addStatement(assignS(hash, calcHash)); } else { @@ -303,6 +314,9 @@ public class EqualsAndHashCodeASTTransformation extends AbstractASTTransformatio } public static void createEquals(ClassNode cNode, boolean includeFields, boolean callSuper, boolean useCanEqual, List<String> excludes, List<String> includes, boolean allNames, boolean allProperties, boolean pojo) { + createEquals(cNode, includeFields, callSuper, useCanEqual, excludes, includes, allNames, allProperties, pojo, false); + } + public static void createEquals(ClassNode cNode, boolean includeFields, boolean callSuper, boolean useCanEqual, List<String> excludes, List<String> includes, boolean allNames, boolean allProperties, boolean pojo, boolean useGetter) { if (useCanEqual) createCanEqual(cNode); // make a public method if none exists otherwise try a private method with leading underscore boolean hasExistingEquals = hasDeclaredMethod(cNode, EQUALS, 1); @@ -348,8 +362,10 @@ public class EqualsAndHashCodeASTTransformation extends AbstractASTTransformatio boolean canBeSelf = StaticTypeCheckingSupport.implementsInterfaceOrIsSubclassOf( pNode.getOriginType(), cNode ); + Expression thisX = useGetter ? getterThisX(originType, pNode) : propX(varX("this"), pNode.getName()); + Expression otherX = useGetter ? getterX(originType, otherTyped, pNode) : propX(otherTyped, pNode.getName()); Expression propsEqual = pojo - ? callX(OBJECTS_TYPE, EQUALS, args(getterThisX(originType, pNode), getterX(originType, otherTyped, pNode))) + ? callX(OBJECTS_TYPE, EQUALS, args(thisX, otherX)) : hasEqualPropertyX(originType, pNode, otherTyped); if (!canBeSelf) { body.addStatement(ifS(notX(propsEqual), returnS(constX(Boolean.FALSE, true)))); diff --git a/src/main/java/org/codehaus/groovy/transform/RecordTypeASTTransformation.java b/src/main/java/org/codehaus/groovy/transform/RecordTypeASTTransformation.java index 0dd3597387..821b1f81ee 100644 --- a/src/main/java/org/codehaus/groovy/transform/RecordTypeASTTransformation.java +++ b/src/main/java/org/codehaus/groovy/transform/RecordTypeASTTransformation.java @@ -243,7 +243,7 @@ public class RecordTypeASTTransformation extends AbstractASTTransformation imple ToStringASTTransformation.createToString(cNode, false, false, null, null, true, false, false, false, false, false, false, false, true, - new String[]{"[", "]", "=", ", "}); + new String[]{"[", "]", "=", ", "}, false); } } @@ -252,8 +252,8 @@ public class RecordTypeASTTransformation extends AbstractASTTransformation imple createRecordEquals(cNode); createRecordHashCode(cNode); } else { - EqualsAndHashCodeASTTransformation.createEquals(cNode, false, false, false, null, null, false, false, true); - EqualsAndHashCodeASTTransformation.createHashCode(cNode, false, false, false, null, null, false, false, true); + EqualsAndHashCodeASTTransformation.createEquals(cNode, false, false, false, null, null, false, false, true, false); + EqualsAndHashCodeASTTransformation.createHashCode(cNode, true, false, false, null, null, false, false, true, false); } } diff --git a/src/main/java/org/codehaus/groovy/transform/ToStringASTTransformation.java b/src/main/java/org/codehaus/groovy/transform/ToStringASTTransformation.java index 1228666d7b..9f060445bd 100644 --- a/src/main/java/org/codehaus/groovy/transform/ToStringASTTransformation.java +++ b/src/main/java/org/codehaus/groovy/transform/ToStringASTTransformation.java @@ -66,6 +66,7 @@ import static org.codehaus.groovy.ast.tools.GeneralUtils.ifElseS; import static org.codehaus.groovy.ast.tools.GeneralUtils.ifS; import static org.codehaus.groovy.ast.tools.GeneralUtils.localVarX; import static org.codehaus.groovy.ast.tools.GeneralUtils.notNullX; +import static org.codehaus.groovy.ast.tools.GeneralUtils.propX; import static org.codehaus.groovy.ast.tools.GeneralUtils.returnS; import static org.codehaus.groovy.ast.tools.GeneralUtils.sameX; import static org.codehaus.groovy.ast.tools.GeneralUtils.varX; @@ -108,6 +109,7 @@ public class ToStringASTTransformation extends AbstractASTTransformation { String rightDelim = getMemberStringValue(anno, "rightDelimiter", ")"); String nameValueSep = getMemberStringValue(anno, "nameValueSeparator", ":"); String fieldSep = getMemberStringValue(anno, "fieldSeparator", ", "); + boolean useGetter = !memberHasValue(anno, "useGetters", false); if (includes != null && includes.contains("super")) { includeSuper = true; } @@ -133,7 +135,7 @@ public class ToStringASTTransformation extends AbstractASTTransformation { if (!checkPropertyList(cNode, includes != null ? DefaultGroovyMethods.minus(includes, "super") : null, "includes", anno, MY_TYPE_NAME, includeFields, includeSuperProperties, allProperties)) return; if (!checkPropertyList(cNode, excludes, "excludes", anno, MY_TYPE_NAME, includeFields, includeSuperProperties, allProperties)) return; String[] delims = new String[]{leftDelim, rightDelim, nameValueSep, fieldSep}; - createToString(cNode, includeSuper, includeFields, excludes, includes, includeNames, ignoreNulls, includePackage, cacheToString, includeSuperProperties, allProperties, allNames, includeSuperFields, pojo, delims); + createToString(cNode, includeSuper, includeFields, excludes, includes, includeNames, ignoreNulls, includePackage, cacheToString, includeSuperProperties, allProperties, allNames, includeSuperFields, pojo, delims, useGetter); } } @@ -166,6 +168,9 @@ public class ToStringASTTransformation extends AbstractASTTransformation { } public static void createToString(ClassNode cNode, boolean includeSuper, boolean includeFields, List<String> excludes, List<String> includes, boolean includeNames, boolean ignoreNulls, boolean includePackage, boolean cache, boolean includeSuperProperties, boolean allProperties, boolean allNames, boolean includeSuperFields, boolean pojo, String[] delims) { + createToString(cNode, includeSuper, includeFields, excludes, includes, includeNames, ignoreNulls, includePackage, cache, includeSuperProperties, allProperties, allNames, includeSuperFields, pojo, delims, false); + } + public static void createToString(ClassNode cNode, boolean includeSuper, boolean includeFields, List<String> excludes, List<String> includes, boolean includeNames, boolean ignoreNulls, boolean includePackage, boolean cache, boolean includeSuperProperties, boolean allProperties, boolean allNames, boolean includeSuperFields, boolean pojo, String[] delims, boolean useGetter) { if (delims == null || delims.length != 4) { delims = new String[]{"(", ")", ":", ", "}; } @@ -186,11 +191,11 @@ public class ToStringASTTransformation extends AbstractASTTransformation { final Expression savedToString = varX(cacheField); body.addStatement(ifS( equalsNullX(savedToString), - assignS(savedToString, calculateToStringStatements(cNode, includeSuper, includeFields, includeSuperFields, excludes, includes, includeNames, ignoreNulls, includePackage, includeSuperProperties, allProperties, body, allNames, pojo, delims)) + assignS(savedToString, calculateToStringStatements(cNode, includeSuper, includeFields, includeSuperFields, excludes, includes, includeNames, ignoreNulls, includePackage, includeSuperProperties, allProperties, body, allNames, pojo, delims, useGetter)) )); tempToString = savedToString; } else { - tempToString = calculateToStringStatements(cNode, includeSuper, includeFields, includeSuperFields, excludes, includes, includeNames, ignoreNulls, includePackage, includeSuperProperties, allProperties, body, allNames, pojo, delims); + tempToString = calculateToStringStatements(cNode, includeSuper, includeFields, includeSuperFields, excludes, includes, includeNames, ignoreNulls, includePackage, includeSuperProperties, allProperties, body, allNames, pojo, delims, useGetter); } body.addStatement(returnS(tempToString)); @@ -210,7 +215,7 @@ public class ToStringASTTransformation extends AbstractASTTransformation { boolean canBeSelf; } - private static Expression calculateToStringStatements(ClassNode cNode, boolean includeSuper, boolean includeFields, boolean includeSuperFields, List<String> excludes, final List<String> includes, boolean includeNames, boolean ignoreNulls, boolean includePackage, boolean includeSuperProperties, boolean allProperties, BlockStatement body, boolean allNames, boolean pojo, String[] delims) { + private static Expression calculateToStringStatements(ClassNode cNode, boolean includeSuper, boolean includeFields, boolean includeSuperFields, List<String> excludes, final List<String> includes, boolean includeNames, boolean ignoreNulls, boolean includePackage, boolean includeSuperProperties, boolean allProperties, BlockStatement body, boolean allNames, boolean pojo, String[] delims, boolean useGetter) { // def _result = new StringBuilder() final Expression result = localVarX("_result"); body.addStatement(declS(result, ctorX(STRINGBUILDER_TYPE))); @@ -239,7 +244,7 @@ public class ToStringASTTransformation extends AbstractASTTransformation { // it's really just a field elements.add(new ToStringElement(varX(fNode), name, canBeSelf(cNode, fNode.getType()))); } else { - Expression getter = getterThisX(cNode, pNode); + Expression getter = useGetter ? getterThisX(cNode, pNode) : propX(varX("this"), pNode.getName()); elements.add(new ToStringElement(getter, name, canBeSelf(cNode, pNode.getType()))); } } diff --git a/src/test/org/codehaus/groovy/transform/CanonicalComponentsTransformTest.groovy b/src/test/org/codehaus/groovy/transform/CanonicalComponentsTransformTest.groovy index 68ebe5f821..f06958c0c7 100644 --- a/src/test/org/codehaus/groovy/transform/CanonicalComponentsTransformTest.groovy +++ b/src/test/org/codehaus/groovy/transform/CanonicalComponentsTransformTest.groovy @@ -641,6 +641,50 @@ class CanonicalComponentsTransformTest extends GroovyShellTestCase { """ } + void testToStringUseGettersAttribute() { + new GroovyShell().evaluate """ + import groovy.transform.* + + @ToString(useGetters=true) + record Foo0(String x0, String x1) { + String x0() { x0.toUpperCase() } + } + + @ToString(useGetters=false) + record Foo1(String x0, String x1) { + String x1() { x1.toUpperCase() } + } + + assert new Foo0('cat', 'dog').toString() == 'Foo0(CAT, dog)' + assert new Foo1('cat', 'dog').toString() == 'Foo1(cat, dog)' + """ + } + + void testEqualsAndHashCodeUseGettersAttribute() { + new GroovyShell().evaluate """ + import groovy.transform.* + + @EqualsAndHashCode(useGetters=true) + record Foo0(String x0, String x1) { + String x0() { x0.toUpperCase() } + } + + @EqualsAndHashCode(useGetters=false) + record Foo1(String x0, String x1) { + String x1() { x1.toUpperCase() } + } + + def foo0a = new Foo0('cat', 'dog') + def foo0b = new Foo0('CAT', 'dog') + def foo1a = new Foo1('cat', 'dog') + def foo1b = new Foo1('cat', 'DOG') + assert foo0a == foo0b + assert foo0a.hashCode() == foo0b.hashCode() + assert foo1a != foo1b + assert foo1a.hashCode() != foo1b.hashCode() + """ + } + void testincludeSuperFieldsAndroperties_GROOVY8013() { new GroovyShell().evaluate """ import groovy.transform.* @@ -767,7 +811,7 @@ class CanonicalComponentsTransformTest extends GroovyShellTestCase { new GroovyShell().evaluate """ import groovy.transform.* @EqualsAndHashCode(includeFields = true) - class FieldAndPropertyIncludedInHashCode { + class FieldAndPropertyIncludedInHashCode { private String field String property } @@ -779,7 +823,7 @@ class CanonicalComponentsTransformTest extends GroovyShellTestCase { new GroovyShell().evaluate ''' import groovy.transform.* @EqualsAndHashCode(allProperties = true) - class FieldAndPropertyIncludedInHashCode { + class FieldAndPropertyIncludedInHashCode { String property String getField() { null } }
