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

Jump to bottom

Improve Refaster matching algorithm #104

Closed
wants to merge 6 commits into from
Closed

Improve Refaster matching algorithm #104

wants to merge 6 commits into from

Conversation

rickie
Copy link
Member

@rickie rickie commented May 20, 2022 •
edited
Loading

(Will add a more detailed explanation later)

(The PR cannot succeed because of the required Error Prone changes, see branch: sschroevers/refaster-tree-tweaks

@rickie rickie requested a review from Stephan202 May 21, 2022 10:35
rickie
rickie commented May 21, 2022
View reviewed changes
Copy link
Member Author

@rickie rickie left a comment

Choose a reason for hiding this comment

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

Some context.

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterCheck.java Outdated
refasterTemplatesTree.collectCandidateTemplates(
sourceIdentifiers.asList(), candidateRules::add);

// XXX: Remove these debug lines
Copy link
Member Author

Choose a reason for hiding this comment

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

Left it here, such that you can still easily verify the changes.

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RefasterCheckTest.java Outdated
@@ -2,7 +2,6 @@

import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap;
import static java.util.function.Function.identity;
import static java.util.function.Predicate.not;
Copy link
Member Author

Choose a reason for hiding this comment

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

The changes in this file are to do a quick and easy verification of the changes. It only runs the tests and templates of the LongStream template collection. These changes can be reverted once we are done with the PR.

pom.xml Show resolved Hide resolved
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterCheck.java Outdated
@@ -161,6 +381,7 @@ private static Stream<Replacement> getReplacements(
return description.fixes.stream().flatMap(fix -> fix.getReplacements(endPositions).stream());
}

// XXX: Instead create an `ImmutableList<RefasterRule>`
Copy link
Member Author

Choose a reason for hiding this comment

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

This is (likely) easier to pick up when we also merge #43.

@rickie rickie force-pushed the rossendrijver/refaster-speed-up branch from 18a783e to 87dfe9c Compare May 21, 2022 11:01
@Stephan202 Stephan202 force-pushed the rossendrijver/refaster-speed-up branch from 87dfe9c to 8285bd6 Compare May 21, 2022 19:58
Stephan202
Stephan202 reviewed May 21, 2022
View reviewed changes
Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

Did a first pass; rebased and added a commit.

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BuildNode.java Outdated
import java.util.TreeMap;

@AutoValue
abstract class BuildNode<T> {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can make this an inner class of Node.

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BuildNode.java Outdated
return new AutoValue_BuildNode<>(new TreeMap<>(), new HashSet<>());
}

static <T> BuildNode<T> create(SortedMap<String, BuildNode<T>> children, Set<T> candidateRules) {
Copy link
Member

Choose a reason for hiding this comment

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

This method seems unused.

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BuildNode.java Outdated
}

@SuppressWarnings("AutoValueImmutableFields" /* The BuildNode is used to construct the tree. */)
public abstract SortedMap<String, BuildNode<T>> children();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public abstract SortedMap<String, BuildNode<T>> children();
abstract SortedMap<String, BuildNode<T>> children();

(We should check why Checkstyle or Error Prone didn't flag this.)

Copy link
Member

Choose a reason for hiding this comment

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

This map doesn't need to be sorted; using a regular HashMap will be more efficient.

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BuildNode.java Outdated
public abstract SortedMap<String, BuildNode<T>> children();

@SuppressWarnings("AutoValueImmutableFields" /* The BuildNode is used to construct the tree. */)
public abstract Set<T> candidateRules();
Copy link
Member

Choose a reason for hiding this comment

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

Node should not talk about rules; it's generic and be used for other purposes.

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BuildNode.java Outdated
@AutoValue
abstract class BuildNode<T> {
static <T> BuildNode<T> create() {
return new AutoValue_BuildNode<>(new TreeMap<>(), new HashSet<>());
Copy link
Member

Choose a reason for hiding this comment

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

We might want to use Collections.newSetFromMap(new IdentityHashMap<>()) instead of a HashSet, or even just ArrayList. TBD how much time we spend on the equality checks.

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Util.java Outdated
Comment on lines 29 to 34
case POSTFIX_INCREMENT:
case PREFIX_INCREMENT:
return "++";
case POSTFIX_DECREMENT:
case PREFIX_DECREMENT:
return "--";
Copy link
Member

Choose a reason for hiding this comment

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

We can easily deduplicate these, e.g. like so:

Suggested change
case POSTFIX_INCREMENT:
case PREFIX_INCREMENT:
return "++";
case POSTFIX_DECREMENT:
case PREFIX_DECREMENT:
return "--";
case POSTFIX_INCREMENT:
return "x++";
case PREFIX_INCREMENT:
return "++x";
case POSTFIX_DECREMENT:
return "x--";
case PREFIX_DECREMENT:
return "--x";

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Util.java Outdated
Comment on lines 26 to 29
// XXX: List needs to be extended, as it currently only supports `BinaryTree`s and `UnaryTree`s.
static String treeKindToString(Tree.Kind kind) {
Copy link
Member

Choose a reason for hiding this comment

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

Arguably this code should live in RefasterCheck; especially if we make sure that each kind has a unique string representation, then there are currently no other uses for it.

pom.xml Show resolved Hide resolved
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterCheck.java Outdated
return Description.NO_MATCH;
for (RefasterRule<?, ?> rule : candidateRules) {
try {
rule.apply(state.getPath(), new SubContext(state.context), matches::add);
Copy link
Member

Choose a reason for hiding this comment

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

I think that the new SubContext(state.context) can be reused.

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterCheck.java Outdated
Comment on lines 264 to 265
String operator = Util.treeKindToString(node.getKind());
identifierCombinations.forEach(ids -> ids.add(operator));
Copy link
Member

Choose a reason for hiding this comment

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

Here we can inline operator, like below. (That, said, it seems we can deduplicate some logic here.)

@rickie rickie force-pushed the rossendrijver/refaster-speed-up branch from 8285bd6 to e914f9d Compare June 2, 2022 07:47
@rickie rickie force-pushed the rossendrijver/refaster-speed-up branch from e914f9d to dac45fb Compare August 25, 2022 22:17
@rickie
Copy link
Member Author

rickie commented Sep 26, 2022

We can close this in favor of: #261.

The important changes are ported to that PR.

@rickie rickie closed this Sep 26, 2022
@Stephan202 Stephan202 deleted the rossendrijver/refaster-speed-up branch September 26, 2022 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stephan202 Stephan202 Stephan202 left review comments

Assignees
No one assigned
Labels
new feature
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rickie @Stephan202