Conversation
compute_self_element_bbox_intersections runs one query per leaf in parallel. Each query only visits the boxes located after its leaf in the tree, so every pair is evaluated once. The kept pairs of each chunk of leaves are concatenated in leaf order: the result does not depend on the number of threads. BREAKING CHANGE: compute_self_element_bbox_intersections now takes a const thread-safe functor that returns true for the pairs to keep, and returns these pairs. The search can no longer be stopped early. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Cpp-Linter Report
|
BotellaA
left a comment
BotellaA
left a comment
There was a problem hiding this comment.
Maybe apply this comment? And see the perf impact
3. compute_other_element_bbox_intersections still uses the old contract (non-const, true = stop). Should it get the same treatment for consistency? It would also parallelize Inspector's surface-pair checks and Explicit's border search.
| for( const auto& pairs : chunk_pairs ) | ||
| { | ||
| return false; | ||
| result.insert( result.end(), pairs.begin(), pairs.end() ); |
There was a problem hiding this comment.
Use absl::c_move() avec un std::back_inserter
2 participants
Summary
AABBTree::compute_self_element_bbox_intersectionsnow runs in parallel and returns the kept pairs in a deterministic order. This moves the optimization from Geode-solutions/Geode-Explicit_private#314 (per-triangle queries in aparallel_forin client code) into the AABB, so every caller gets it with a plain call.This is a breaking change and deliberately goes against part of c385201 ("remove bad parallel design"). It needs discussion before merging, see Points to discuss.
Design
async::parallel_forover chunks of 128 leaves.element_end2 <= element_begin1), so every pair is evaluated exactly once, with the same orientation as before.Differences from the design removed in c385201:
parallel_forat the top.API change
Old call sites fail to compile, because of the non-
constoperator()and the[[nodiscard]]result, so no caller changes meaning silently. Porting means turning the action into aconsttest and then working on the returned pairs. Where an action had other side effects, those move to a sequential loop over the result.The search can no longer be stopped early. This only affected Inspector's "has self-intersection" checks, which now run the full (parallel) search.
Results
i7-12800H, Release build, tests run one at a time. "2 cores" means
taskset -c 0,2withLIBASYNC_NUM_THREADS=2, close to CI conditions.Explicit triangle search (
find_too_close_triangles, median of 3 runs):nextThis PR is a bit faster than #314 because each query skips the left part of the tree. #314 queried the whole tree for each triangle and dropped
t1 <= t0only at the leaves.Other callers get the speed-up without any client-side work:
nexttest-section(one large surface), 20 threadstest-anisotropic-frame-field)Validation
test-aabbchecks that two runs return the same pairs in the same order.next, every self-intersection call returned the same pairs in the same order with 20 threads (twice), 1 thread and 2 cores. The same holds for the 6 Inspector tests that call it. The 2 other Explicit tests,best-effortanddhi-daan, already vary between runs onnext.next: it is now leaf order instead of the old callback order. Explicit's element graph is therefore numbered differently, as with New functions in Geometry and Mesh #314's sort, and all tests still pass.Points to discuss
compute_other_element_bbox_intersectionsstill uses the old contract (non-const,true= stop). Should it get the same treatment for consistency? It would also parallelize Inspector's surface-pair checks and Explicit's border search.Downstream PRs (same branch name, so CI picks this build up)
Merge order: this PR, then OpenGeode-Inspector, then Geode-Hybrid and Geode-Explicit. Geode-Numerics also calls this method but is no longer active, so it isn't ported.
🤖 Generated with Claude Code