Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reimplement EnclosedElementsQuery to improve performance #10237

Merged
merged 4 commits into from
Dec 11, 2023
Merged

Reimplement EnclosedElementsQuery to improve performance #10237

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
57fffb9
Reimplement `EnclosedElementsQuery` to improve performance
dstepanov Dec 7, 2023
95d88e2
Address feedback
dstepanov Dec 11, 2023
1c4b6e9
Correct Groovy element
dstepanov Dec 11, 2023
ff38d98
Correct Java class element
dstepanov Dec 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions core-processor/src/main/java/io/micronaut/inject/ast/DefaultElementQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ final class DefaultElementQuery<T extends Element> implements ElementQuery<T>, E
private final boolean includeOverriddenMethods;
private final boolean includeHiddenElements;
private final boolean excludePropertyElements;
private final int hashcode;

DefaultElementQuery(Class<T> elementType) {
this(elementType, null, false, false, false, false, false, false, false, false, false, false, null, null, null, null, null);
Expand Down Expand Up @@ -89,6 +90,7 @@ final class DefaultElementQuery<T extends Element> implements ElementQuery<T>, E
this.includeHiddenElements = includeHiddenElements;
this.excludePropertyElements = excludePropertyElements;
this.typePredicates = typePredicates;
this.hashcode = Objects.hash(elementType, onlyAccessibleType, onlyDeclared, onlyAbstract, onlyConcrete, onlyInjected, namePredicates, annotationPredicates, modifiersPredicates, elementPredicates, typePredicates, onlyInstance, onlyStatic, includeEnumConstants, includeOverriddenMethods, includeHiddenElements, excludePropertyElements);
}

@Override
Expand Down Expand Up @@ -561,4 +563,21 @@ public ElementQuery<T> filter(@NonNull Predicate<T> predicate) {
public Result<T> result() {
return this;
}

@Override
public boolean equals(Object o) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could also check the hashcode field for short-circuiting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it will be used without a previous hash code check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea probably not

if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
DefaultElementQuery<?> that = (DefaultElementQuery<?>) o;
return onlyDeclared == that.onlyDeclared && onlyAbstract == that.onlyAbstract && onlyConcrete == that.onlyConcrete && onlyInjected == that.onlyInjected && onlyInstance == that.onlyInstance && onlyStatic == that.onlyStatic && includeEnumConstants == that.includeEnumConstants && includeOverriddenMethods == that.includeOverriddenMethods && includeHiddenElements == that.includeHiddenElements && excludePropertyElements == that.excludePropertyElements && Objects.equals(elementType, that.elementType) && Objects.equals(onlyAccessibleType, that.onlyAccessibleType) && Objects.equals(namePredicates, that.namePredicates) && Objects.equals(annotationPredicates, that.annotationPredicates) && Objects.equals(modifiersPredicates, that.modifiersPredicates) && Objects.equals(elementPredicates, that.elementPredicates) && Objects.equals(typePredicates, that.typePredicates);
}

@Override
public int hashCode() {
return hashcode;
}
}
6 changes: 5 additions & 1 deletion core-processor/src/main/java/io/micronaut/inject/ast/MethodElement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,10 @@ default boolean hides(@NonNull MemberElement memberElement) {
return false;
}
}
if (!getDeclaringType().isAssignable(memberElement.getDeclaringType())) {
// not a parent class
return false;
}
ClassElement existingReturnType = hidden.getReturnType().getGenericType();
ClassElement newTypeReturn = newMethod.getReturnType().getGenericType();
if (!newTypeReturn.isAssignable(existingReturnType)) {
Expand All @@ -337,7 +341,7 @@ default boolean hides(@NonNull MemberElement memberElement) {
if (hidden.isPackagePrivate()) {
return newMethod.getDeclaringType().getPackageName().equals(hidden.getDeclaringType().getPackageName());
}
return true;
return memberElement.isAccessible(getDeclaringType());
}
return false;
}
Expand Down
322 changes: 227 additions & 95 deletions core-processor/src/main/java/io/micronaut/inject/ast/utils/EnclosedElementsQuery.java
View file
Open in desktop

Large diffs are not rendered by default.

47 changes: 41 additions & 6 deletions inject-groovy/src/main/groovy/io/micronaut/ast/groovy/visitor/GroovyClassElement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import io.micronaut.inject.ast.utils.AstBeanPropertiesUtils;
import io.micronaut.inject.ast.utils.EnclosedElementsQuery;
import org.codehaus.groovy.ast.AnnotatedNode;
import org.codehaus.groovy.ast.AnnotationNode;
import org.codehaus.groovy.ast.ClassHelper;
import org.codehaus.groovy.ast.ClassNode;
import org.codehaus.groovy.ast.ConstructorNode;
Expand Down Expand Up @@ -720,21 +721,25 @@ protected Collection<ClassNode> getInterfaces(ClassNode classNode) {

@Override
protected List<AnnotatedNode> getEnclosedElements(ClassNode classNode,
ElementQuery.Result<?> result) {
ElementQuery.Result<?> result,
boolean includeAbstract) {
Class<?> elementType = result.getElementType();
return getEnclosedElements(classNode, result, elementType);
return getEnclosedElements(classNode, result, elementType, includeAbstract);
}

private List<AnnotatedNode> getEnclosedElements(ClassNode classNode, ElementQuery.Result<?> result, Class<?> elementType) {
private List<AnnotatedNode> getEnclosedElements(ClassNode classNode,
ElementQuery.Result<?> result,
Class<?> elementType,
boolean includeAbstract) {
if (elementType == MemberElement.class) {
return Stream.concat(
getEnclosedElements(classNode, result, FieldElement.class).stream(),
getEnclosedElements(classNode, result, MethodElement.class).stream()
getEnclosedElements(classNode, result, FieldElement.class, includeAbstract).stream(),
getEnclosedElements(classNode, result, MethodElement.class, includeAbstract).stream()
).toList();
} else if (elementType == MethodElement.class) {
return classNode.getMethods()
.stream()
.filter(methodNode -> !JUNK_METHOD_FILTER.test(methodNode) && (methodNode.getModifiers() & Opcodes.ACC_SYNTHETIC) == 0)
.filter(methodNode -> !JUNK_METHOD_FILTER.test(methodNode) && (methodNode.getModifiers() & Opcodes.ACC_SYNTHETIC) == 0 && (includeAbstract || isNonAbstract(classNode, methodNode)))
.<AnnotatedNode>map(m -> m)
.toList();
} else if (elementType == FieldElement.class) {
Expand All @@ -761,6 +766,19 @@ private List<AnnotatedNode> getEnclosedElements(ClassNode classNode, ElementQuer
}
}

private boolean isNonAbstract(ClassNode classNode, MethodNode methodNode) {
if (methodNode.isDefault()) {
return false;
}
if (methodNode.isPrivate() && classNode.isInterface()) {
return false;
}
if (!methodNode.isAbstract() && classNode.isInterface()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont understand this method. private & interface -> abstract? !abstract & interface -> abstract?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private methods are never abstract, and the other one is the default method

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code says that private interface methods are abstract though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, fixed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can drop the interface check too i guess

return false;
}
return !methodNode.isAbstract();
}

@Override
protected boolean excludeClass(ClassNode classNode) {
String packageName = Objects.requireNonNullElse(classNode.getPackageName(), "");
Expand All @@ -775,6 +793,23 @@ protected boolean excludeClass(ClassNode classNode) {
|| Script.class.getName().equals(className);
}

@Override
protected boolean isAbstractClass(ClassNode classNode) {
return classNode.isAbstract();
}

@Override
protected boolean isInterface(ClassNode classNode) {
if (classNode.isInterface()) {
return true;
}
List<AnnotationNode> annotations = classNode.getAnnotations();
if (annotations != null) {
return annotations.stream().anyMatch(a -> a.getClassNode().getName().equals(groovy.transform.Trait.class.getName()));
}
return false;
}

@Override
protected Element toAstElement(AnnotatedNode nativeType, Class<?> elementType) {
final GroovyElementFactory elementFactory = visitorContext.getElementFactory();
Expand Down
3 changes: 1 addition & 2 deletions ...c/test/groovy/io/micronaut/aop/introduction/IntroductionAdviceWithNewInterfaceSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ package io.micronaut.aop.introduction

import io.micronaut.ast.transform.test.AbstractBeanDefinitionSpec
import io.micronaut.context.ApplicationContext
import io.micronaut.context.DefaultBeanContext
import io.micronaut.context.event.ApplicationEventListener
import io.micronaut.inject.BeanDefinition
import io.micronaut.inject.InstantiatableBeanDefinition
Expand Down Expand Up @@ -193,7 +192,7 @@ interface SpecificInterface {
then:
//I ended up going this route because actually calling the methods here would be relying on
//having the target interface in the bytecode of the test
instance.$proxyMethods.length == 1
instance.$proxyMethods.length == 2
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Groovy test is now the same as the one for Java


cleanup:
context.close()
Expand Down
2 changes: 1 addition & 1 deletion inject-groovy/src/test/groovy/io/micronaut/inject/visitor/ClassElementSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -815,7 +815,7 @@ interface MyBean extends GenericInterface, SpecificInterface {
when:
def allMethods = classElement.getEnclosedElements(ElementQuery.ALL_METHODS)
then:
allMethods.size() == 1
allMethods.size() == 2
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Groovy test is now the same as the one for Java

when:
def declaredMethods = classElement.getEnclosedElements(ElementQuery.ALL_METHODS.onlyDeclared())
then:
Expand Down
40 changes: 36 additions & 4 deletions inject-java/src/main/java/io/micronaut/annotation/processing/visitor/JavaClassElement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ public class JavaClassElement extends AbstractJavaElement implements ArrayableCl
private Map<String, Map<String, ClassElement>> resolvedAllTypeArguments;
@Nullable
private ClassElement resolvedSuperType;
@Nullable
private List<ClassElement> resolvedInterfaces;
private final JavaEnclosedElementsQuery enclosedElementsQuery = new JavaEnclosedElementsQuery(false);
private final JavaEnclosedElementsQuery sourceEnclosedElementsQuery = new JavaEnclosedElementsQuery(true);
@Nullable
Expand Down Expand Up @@ -256,7 +258,10 @@ public boolean isPrimitive() {

@Override
public Collection<ClassElement> getInterfaces() {
return classElement.getInterfaces().stream().map(mirror -> newClassElement(mirror, getTypeArguments())).toList();
if (resolvedInterfaces == null) {
resolvedInterfaces = classElement.getInterfaces().stream().map(mirror -> newClassElement(mirror, getTypeArguments())).toList();
}
return resolvedInterfaces;
}

@Override
Expand Down Expand Up @@ -796,15 +801,32 @@ protected Collection<TypeElement> getInterfaces(TypeElement classNode) {
}

@Override
protected List<Element> getEnclosedElements(TypeElement classNode, ElementQuery.Result<?> result) {
protected List<Element> getEnclosedElements(TypeElement classNode, ElementQuery.Result<?> result, boolean includeAbstract) {
List<? extends Element> ee;
if (classNode == classElement) {
ee = getEnclosedElements();
} else {
ee = classNode.getEnclosedElements();
}
EnumSet<ElementKind> elementKinds = getElementKind(result);
return ee.stream().filter(element -> elementKinds.contains(element.getKind())).collect(Collectors.toList());
List<Element> list = new ArrayList<>(elementKinds.size());
yawkat marked this conversation as resolved.
Show resolved Hide resolved
for (Element element : ee) {
Set<Modifier> modifiers = element.getModifiers();
if (elementKinds.contains(element.getKind()) && (includeAbstract || isNonAbstractMethod(modifiers, classNode))) {
list.add(element);
}
}
return list;
}

private boolean isNonAbstractMethod(Set<Modifier> modifiers, TypeElement classNode) {
if (modifiers.contains(Modifier.DEFAULT)) {
return true;
}
if (modifiers.contains(Modifier.PRIVATE) && isInterface(classNode)) {
yawkat marked this conversation as resolved.
Show resolved Hide resolved
return true;
}
return !modifiers.contains(Modifier.ABSTRACT);
}

@Override
Expand All @@ -813,6 +835,16 @@ protected boolean excludeClass(TypeElement classNode) {
|| classNode.getQualifiedName().toString().equals(Enum.class.getName());
}

@Override
protected boolean isAbstractClass(TypeElement classNode) {
return classNode.getModifiers().contains(Modifier.ABSTRACT);
}

@Override
protected boolean isInterface(TypeElement classNode) {
return classNode.getKind() == ElementKind.INTERFACE;
}

@Override
protected io.micronaut.inject.ast.Element toAstElement(Element nativeType, Class<?> elementType) {
final JavaElementFactory elementFactory = visitorContext.getElementFactory();
Expand Down Expand Up @@ -892,4 +924,4 @@ private EnumSet<ElementKind> getElementKind(ElementQuery.Result<?> result) {

}

}
}
36 changes: 36 additions & 0 deletions inject-java/src/test/groovy/io/micronaut/visitors/ClassElementSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3319,6 +3319,42 @@ interface MyInterface {
methods.size() == 4
}

void "test unrecognized default method"() {
given:
ClassElement classElement = buildClassElement('''
package elementquery;

interface MyBean extends GenericInterface, SpecificInterface {

default Specific getObject() {
return null;
}

}

class Generic {
}
class Specific extends Generic {
}
interface GenericInterface {
Generic getObject();
}
interface SpecificInterface {
Specific getObject();
}

''')
when:
def allMethods = classElement.getEnclosedElements(ElementQuery.ALL_METHODS)
then:
allMethods.size() == 2
when:
def declaredMethods = classElement.getEnclosedElements(ElementQuery.ALL_METHODS.onlyDeclared())
then:
declaredMethods.size() == 1
declaredMethods.get(0).isDefault() == true
}

private void assertListGenericArgument(ClassElement type, Closure cl) {
def arg1 = type.getAllTypeArguments().get(List.class.name).get("E")
def arg2 = type.getAllTypeArguments().get(Collection.class.name).get("E")
Expand Down
18 changes: 12 additions & 6 deletions inject-kotlin/src/main/kotlin/io/micronaut/kotlin/processing/visitor/KotlinClassElement.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,22 +700,24 @@ internal open class KotlinClassElement(

override fun getEnclosedElements(
classNode: KSClassDeclaration,
result: ElementQuery.Result<*>
result: ElementQuery.Result<*>,
includeAbstract: Boolean
): List<KSNode> {
val elementType: Class<*> = result.elementType
return getEnclosedElements(classNode, result, elementType)
return getEnclosedElements(classNode, result, elementType, includeAbstract)
}

private fun getEnclosedElements(
classNode: KSClassDeclaration,
result: ElementQuery.Result<*>,
elementType: Class<*>
elementType: Class<*>,
includeAbstract: Boolean
): List<KSNode> {
return when (elementType) {
MemberElement::class.java -> {
Stream.concat(
getEnclosedElements(classNode, result, FieldElement::class.java).stream(),
getEnclosedElements(classNode, result, MethodElement::class.java).stream()
getEnclosedElements(classNode, result, FieldElement::class.java, includeAbstract).stream(),
getEnclosedElements(classNode, result, MethodElement::class.java, includeAbstract).stream()
).toList()
}

Expand All @@ -729,7 +731,7 @@ internal open class KotlinClassElement(
"hashCode",
"toString",
"equals"
).contains(func.simpleName.asString())
).contains(func.simpleName.asString()) && (includeAbstract || !func.isAbstract || !classNode.isAbstract())
}
.toList()
}
Expand Down Expand Up @@ -772,6 +774,10 @@ internal open class KotlinClassElement(
classNode.qualifiedName.toString() == Enum::class.java.name
}

override fun isAbstractClass(classNode: KSClassDeclaration) = classNode.isAbstract()

override fun isInterface(classNode: KSClassDeclaration) = classNode.classKind == ClassKind.INTERFACE

override fun toAstElement(
nativeType: KSNode,
elementType: Class<*>
Expand Down
Loading