Skip to content

Commit

Permalink
Reimplement EnclosedElementsQuery to improve performance (#10237)
Browse files Browse the repository at this point in the history
  • Loading branch information
dstepanov authored Dec 11, 2023
1 parent 69c9029 commit c037ea1
Show file tree
Hide file tree
Showing 9 changed files with 372 additions and 115 deletions.
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) {
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;
}
}
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

Large diffs are not rendered by default.

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 true;
}
if (!methodNode.isAbstract() && classNode.isInterface()) {
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
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

cleanup:
context.close()
Expand Down
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
when:
def declaredMethods = classElement.getEnclosedElements(ElementQuery.ALL_METHODS.onlyDeclared())
then:
Expand Down
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<>(ee.size());
for (Element element : ee) {
Set<Modifier> modifiers = element.getModifiers();
if (elementKinds.contains(element.getKind()) && (includeAbstract || isNonAbstractMethod(modifiers))) {
list.add(element);
}
}
return list;
}

private boolean isNonAbstractMethod(Set<Modifier> modifiers) {
if (modifiers.contains(Modifier.DEFAULT)) {
return true;
}
if (modifiers.contains(Modifier.PRIVATE)) {
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) {

}

}
}
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
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

0 comments on commit c037ea1

Please sign in to comment.