[
https://issues.apache.org/jira/browse/GROOVY-11609?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17943802#comment-17943802
]
ASF GitHub Bot commented on GROOVY-11609:
-----------------------------------------
Copilot commented on code in PR #2186:
URL: https://github.com/apache/groovy/pull/2186#discussion_r2040641694
##########
src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java:
##########
@@ -192,113 +195,130 @@ private void declare(final Variable variable, final
ASTNode context) {
currentScope.putDeclaredVariable(variable);
}
+ private final Map<ClassMemberCacheKey, VariableWrapper> classMemberCache =
new HashMap<>(64);
private Variable findClassMember(final ClassNode node, final String name) {
- final boolean abstractType = node.isAbstract();
-
- for (ClassNode cn = node; cn != null && !ClassHelper.isObjectType(cn);
cn = cn.getSuperClass()) {
- for (FieldNode fn : cn.getFields()) {
- if (name.equals(fn.getName())) return fn;
- }
+ VariableWrapper variableWrapper = classMemberCache.computeIfAbsent(new
ClassMemberCacheKey(name, node), k -> {
+ final ClassNode classNode = k.node;
+ final String memberName = k.name;
+ final boolean abstractType = classNode.isAbstract();
+
+ for (ClassNode cn = classNode; cn != null &&
!ClassHelper.isObjectType(cn); cn = cn.getSuperClass()) {
+ for (FieldNode fn : cn.getFields()) {
+ if (memberName.equals(fn.getName())) return new
VariableWrapper(fn);
+ }
- for (PropertyNode pn : cn.getProperties()) {
- if (name.equals(pn.getName())) return pn;
- }
+ for (PropertyNode pn : cn.getProperties()) {
+ if (memberName.equals(pn.getName())) return new
VariableWrapper(pn);
+ }
- for (MethodNode mn : cn.getMethods()) {
- if ((abstractType || !mn.isAbstract()) &&
name.equals(getPropertyName(mn))) {
- // check for super property before returning a
pseudo-property
- for (PropertyNode pn :
getAllProperties(cn.getSuperClass())) {
- if (name.equals(pn.getName())) return pn;
+ for (MethodNode mn : cn.getMethods()) {
+ if ((abstractType || !mn.isAbstract()) &&
memberName.equals(getPropertyName(mn))) {
+ // check for super property before returning a
pseudo-property
+ for (PropertyNode pn :
getAllProperties(cn.getSuperClass())) {
+ if (memberName.equals(pn.getName())) return new
VariableWrapper(pn);
+ }
+
+ FieldNode fn = new FieldNode(memberName,
mn.getModifiers() & 0xF, ClassHelper.dynamicType(), cn, null);
+ fn.setHasNoRealSourcePosition(true);
+ fn.setDeclaringClass(cn);
+ fn.setSynthetic(true);
+
+ PropertyNode pn = new PropertyNode(fn,
fn.getModifiers(), null, null);
+ pn.putNodeMetaData("access.method", mn);
+ pn.setDeclaringClass(cn);
+ return new VariableWrapper(pn);
}
+ }
- FieldNode fn = new FieldNode(name, mn.getModifiers() &
0xF, ClassHelper.dynamicType(), cn, null);
- fn.setHasNoRealSourcePosition(true);
- fn.setDeclaringClass(cn);
- fn.setSynthetic(true);
-
- PropertyNode pn = new PropertyNode(fn, fn.getModifiers(),
null, null);
- pn.putNodeMetaData("access.method", mn);
- pn.setDeclaringClass(cn);
- return pn;
+ for (ClassNode in : cn.getAllInterfaces()) {
+ FieldNode fn = in.getDeclaredField(memberName);
+ if (fn != null) return new VariableWrapper(fn);
+ PropertyNode pn = in.getProperty(memberName);
+ if (pn != null) return new VariableWrapper(pn);
}
}
- for (ClassNode in : cn.getAllInterfaces()) {
- FieldNode fn = in.getDeclaredField(name);
- if (fn != null) return fn;
- PropertyNode pn = in.getProperty(name);
- if (pn != null) return pn;
- }
- }
+ return new VariableWrapper(null);
Review Comment:
Returning a VariableWrapper wrapping null may lead to ambiguity in cache
lookups. Consider handling cache misses explicitly so that the absence of a
variable is distinguishable without wrapping a null value.
```suggestion
return VariableWrapper.EMPTY;
```
##########
src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java:
##########
@@ -760,4 +780,66 @@ public void visitVariableExpression(final
VariableExpression expression) {
checkVariableContextAccess(variable, expression);
}
}
+
+ private static class VariableWrapper {
+ private final Variable variable;
+ private int accessedCount;
+
+ VariableWrapper(final Variable variable) {
+ this.variable = variable;
+ }
+ }
+
+ private static class ClassMemberCacheKey {
+ private static final int DEFAULT_HASH = -1;
+ private final String name;
+ private final ClassNode node;
+ private int hash = DEFAULT_HASH;
+
+ ClassMemberCacheKey(final String name, final ClassNode node) {
+ this.name = name;
+ this.node = node;
+ }
+
+ @Override
+ public boolean equals(final Object obj) {
+ if (this == obj) return true;
+ if (!(obj instanceof ClassMemberCacheKey)) return false;
+ ClassMemberCacheKey that = (ClassMemberCacheKey) obj;
+ return name.equals(that.name) && node.equals(that.node);
+ }
+
+ @Override
+ public int hashCode() {
+ return DEFAULT_HASH != hash ? hash : (hash = Objects.hash(name,
node));
Review Comment:
Using -1 as DEFAULT_HASH may cause an issue if Objects.hash returns -1,
leading to an incorrect cached hash. Consider choosing a different sentinel
value to avoid potential collisions.
> Avoid finding same variable/class member repeatedly
> ---------------------------------------------------
>
> Key: GROOVY-11609
> URL: https://issues.apache.org/jira/browse/GROOVY-11609
> Project: Groovy
> Issue Type: Improvement
> Reporter: Daniel Sun
> Priority: Major
>
> When I debugged GROOVY-4721, I found {{VariableScopeVisitor}} will try to
> find same variable/class member again and again, the finding logic is quite
> complex. Unfortunately, {{VariableScopeVisitor}} is widely used, e.g.
> {{ResolveVisitor}}, {{JavaStubCompilationUnit}} and
> {{TraitASTTransformation}}. So it's better to avoid finding same
> variable/class member repeatedly to gain better performance.
> For example, in the following code, {{a}}, {{i}}, {{j}} will be tried to find
> many times:
> {code}
> class BubbleSort {
> public static void bubbleSort(int[] a) {
> for (int i = 0, n = a.length; i < n - 1; i++) {
> for (int j = 0; j < n - i - 1; j++) {
> if (a[j] > a[j + 1]) {
> int temp = a[j]
> a[j] = a[j + 1]
> a[j + 1] = temp
> }
> }
> }
> }
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
