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

Optimize FieldLocation #1

Conversation

schlosna
Copy link

@schlosna schlosna commented Jan 26, 2024

FieldLocation maintains set of paths to use in rules ordered from leaf to parent. The set provides O(1) hierarchyMatches operation.

This addresses the following trace from JFR running some test cases using assertj 3.25.2 using usingRecursiveComparison().isEqualTo

image

Check List:

@@ -33,7 +36,7 @@ public final class FieldLocation implements Comparable<FieldLocation> {

private final String pathToUseInRules;
private final List<String> decomposedPath;
private final List<String> pathsHierarchyToUseInRules;
private final Set<String> pathsHierarchyToUseInRules;
Copy link
Author

Choose a reason for hiding this comment

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

provide O(1) contains instead of O(N) for List#contains used by hierarchyMatches(String)

@@ -163,7 +166,10 @@ public boolean equals(Object obj) {

@Override
public int hashCode() {
return Objects.hash(pathToUseInRules, decomposedPath, pathsHierarchyToUseInRules);
Copy link
Author

Choose a reason for hiding this comment

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

minor improvement to avoid implicit Object[] allocation from Objects.hash

Comment on lines -272 to -274
List<String> parentPath = new ArrayList<>(decomposedPath);
parentPath.remove(decomposedPath.size() - 1);
return new FieldLocation(parentPath);
Copy link
Author

Choose a reason for hiding this comment

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

This was allocating a lot of ArrayList and FieldLocation. I had tried return new FieldLocation(decomposedPath.subList(0, decomposedPath.size() - 1)); but that still allocates O(N) FieldLocation as pathsHierarchyToUseInRules traverses the node's path to root.

Copy link

@joel-costigliola joel-costigliola Jan 26, 2024

Choose a reason for hiding this comment

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

we could simplify the hash code by only using pathToUseInRules since the fields are more or less variations of that information

Copy link
Author

Choose a reason for hiding this comment

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

good call, String#hashCode() is memoized and much less expensive than also including List<String>#hashCode() and Set<String>#hashCode() which are each O(N)

Copy link
Author

Choose a reason for hiding this comment

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

updated

Copy link
Author

Choose a reason for hiding this comment

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

Going to adjust this to satisfy equals verifier error

-> Significant fields: equals relies on decomposedPath, but hashCode does not.

FieldLocation maintains set of paths to use in rules ordered from leaf
to parent. The set provides O(1) hierarchyMatches operation.
@schlosna schlosna force-pushed the aash/usingRecursiveComparison-slowness-repro branch from 159ad26 to f9b759a Compare January 26, 2024 17:38
@ash211 ash211 merged commit eaca0f2 into ash211:aash/usingRecursiveComparison-slowness-repro Jan 28, 2024
@schlosna schlosna deleted the aash/usingRecursiveComparison-slowness-repro branch January 29, 2024 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants