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.

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

Conversation

dstepanov
Copy link
Contributor

  • Querieng now avoids processing abstract interface methods for non-abstract classes
  • Added caching for query requests
  • Fixed hides method

Should fix #10145 and also improve KSP performance.

@dstepanov
Copy link
Contributor Author

@Spikhalskiy Can you please try your project with this branch?

@dstepanov dstepanov added the type: improvement A minor improvement to an existing feature label Dec 8, 2023
@dstepanov dstepanov added this to the 4.3.0 milestone Dec 8, 2023
@@ -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

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

@@ -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 (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

@dstepanov dstepanov requested a review from yawkat December 11, 2023 08:18
iterator.remove();
}
}
elements.removeIf(element -> !filter.test(element));
Copy link
Member

Choose a reason for hiding this comment

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

you can use filter.negate()

Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

88.4% 88.4% Coverage
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: improvement A minor improvement to an existing feature
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants