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

Fix AABB tree performance. #1687

Merged
merged 3 commits into from Dec 20, 2023
Merged

Fix AABB tree performance. #1687

merged 3 commits into from Dec 20, 2023

Conversation

joaander
Copy link
Member

Description

Use the fast AABB-AABB overlap check when traversing AABB trees.

I implemented this by moving the overlap(AABB, AABB) function to a member function of AABB: aabb.overlaps(other_aabb). Explicitly calling the intended overlap test ensures that the correct function is called.

Motivation and context

Increase performance.

For reasons I don't fully understand, the compiler chose to automatically convert AABBs to OBBs, then call overlap(OBB, OBB) - which is many times slower than overlap(AABB, AABB).

How has this been tested?

This pull request increases performance by ~40-50% in HPMC benchmarks (glotzerlab/hoomd-benchmarks#68). It should also improve MD performance with nlist.Tree, though I haven't tested that.

Change log

Fixed:

* Increase HPMC performance a further ~50% on the CPU.

Checklist:

@joaander joaander requested review from a team as code owners December 19, 2023 19:01
@joaander joaander requested review from tcmoore3, tommy-waltmann and syjlee and removed request for a team December 19, 2023 19:01
I was testing whether Ball-AABB would be faster. Will investigate further in a later
PR: #1686
@@ -115,7 +115,7 @@ struct OBB
is_sphere = 1;
}

DEVICE OBB(const hoomd::detail::AABB& aabb)
DEVICE explicit OBB(const hoomd::detail::AABB& aabb)
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 change ensures that no future automatic conversions from AABB to OBB are allowed.

Callers must opt into the conversion explicitly: OBB obb(aabb) or OBB(aabb).

Copy link
Contributor

@tommy-waltmann tommy-waltmann left a comment

Choose a reason for hiding this comment

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

Have you tested to see if it was necessary to move the function call to a class member of AABB, or if just adding explicit to the OBB constructor was enough to do the trick?

Copy link
Contributor

@tommy-waltmann tommy-waltmann left a comment

Choose a reason for hiding this comment

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

Approving because I don't want to hold up the next release.

@joaander
Copy link
Member Author

Have you tested to see if it was necessary to move the function call to a class member of AABB, or if just adding explicit to the OBB constructor was enough to do the trick?

When I only added explicit, I got an error compile error that there was no overlap(AABB, AABB) to call. I don't know why because it is clearly in the header file - so I moved it to a member instead.

@tommy-waltmann
Copy link
Contributor

When I only added explicit, I got an error compile error that there was no overlap(AABB, AABB) to call. I don't know why because it is clearly in the header file - so I moved it to a member instead.

That seems like the reason the compiler was automatically converting the AABBs to OBBs, it couldn't find the overlap(AABB, AABB) declaration.

@joaander joaander added the validate Execute long running validation tests on pull requests label Dec 20, 2023
@joaander joaander merged commit 52b72ba into trunk-patch Dec 20, 2023
72 of 73 checks passed
@joaander joaander deleted the fix-aabb-performance branch December 20, 2023 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
validate Execute long running validation tests on pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants