Skip to content

Require a minimum number of volumes to build a BIH tree #1775

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

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

elliottbiondo
Copy link
Contributor

Analysis of the tilecal problem confirmed that the BIH is causing a 3x slowdown. Profiling revealed that this is due to increased branching (fewer active threads per warp) which leads to worse memory performance. The tilecal problem itself has a few universes with more than 100 volumes, but many more with fewer than 20. This MR adds a bih_min_size parameter that causes the BIH to only be built for universes with a sufficiently large number of volumes. If a universe has too few volumes, all volumes will be placed into "non-tree volumes," which was previously reserved for volumes with infinite bounding boxes. This results in the following performance on Wildstyle:

Commit Tilecal runtime on V100 (s)
before BIH was merged: 09cbf5b 49.5
after BIH was merged: acf5007 146
develop: a4c469f 146
this MR: c37d4d3 64.6

Likewise, this MR provides an 88% increase in active threads per warp w.r.t. develop, as well as the following improved memory behavior (also w.r.t. develop):

Screenshot 2025-05-08 at 6 07 53 PM

This MR is a draft for two reasons:

  1. bih_min_size is a tuning parameter. We need to come up with a better system for incorporating these in the code.
  2. It is not clear what to tune this parameter to. Obviously a single test on a V100 is not sufficient. Would the entire suite on an MI250X be sufficient? Can we define and overall FOM for the suite, in the event that performance gains for one problem are accompanied by performance losses in another?

Once we resolve these issues I will update this MR accordingly.

@elliottbiondo elliottbiondo marked this pull request as draft May 8, 2025 22:31
@elliottbiondo elliottbiondo requested a review from sethrj May 8, 2025 22:31
@elliottbiondo elliottbiondo self-assigned this May 8, 2025
@elliottbiondo elliottbiondo added orange Work on ORANGE geometry engine performance Changes for performance optimization geometry Geometry-related features labels May 8, 2025
@elliottbiondo elliottbiondo added this to the v1.0.0 milestone May 8, 2025
Copy link

github-actions bot commented May 8, 2025

Test summary

  466 files    727 suites   23s ⏱️
1 169 tests 1 150 ✅ 14 💤  5 ❌
2 358 runs  2 340 ✅  8 💤 10 ❌

For more details on these failures, see this check.

Results for commit c37d4d3.

@elliottbiondo
Copy link
Contributor Author

Draft until this is refactor to instead set a maximum leaf size

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
geometry Geometry-related features orange Work on ORANGE geometry engine performance Changes for performance optimization
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

1 participant