Workaround for Stack performance #306
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dalg24
requested changes
May 18, 2020
dalg24
reviewed
May 18, 2020
| Node const **stack_ptr = stack; | ||
| *stack_ptr++ = NULL; | ||
| Node const *node = bvh_.getRoot(); | ||
| int count = 0; |
Can you confirm that this is what you tried? diff --git a/src/details/ArborX_DetailsTreeTraversal.hpp b/src/details/ArborX_DetailsTreeTraversal.hpp
index c664705..592380e 100644
--- a/src/details/ArborX_DetailsTreeTraversal.hpp
+++ b/src/details/ArborX_DetailsTreeTraversal.hpp
@@ -85,11 +85,9 @@ struct TreeTraversal<BVH, Predicates, Callback, SpatialPredicateTag>
{
auto const &predicate = Access::get(predicates_, queryIndex);
- Node const *stack[64];
- Node const **stack_ptr = stack;
- *stack_ptr++ = NULL;
+ Stack<Node const *> stack;
+ stack.push(nullptr);
Node const *node = bvh_.getRoot();
- int count = 0;
do
{
Node const *child_left = bvh_.getNodePtr(node->children.first);
@@ -105,7 +103,6 @@ struct TreeTraversal<BVH, Predicates, Callback, SpatialPredicateTag>
if (overlap_right && child_right->isLeaf())
{
callback_(queryIndex, child_right->getLeafPermutationIndex());
- count++;
}
bool traverse_left = (overlap_left && !child_left->isLeaf());
@@ -113,15 +110,16 @@ struct TreeTraversal<BVH, Predicates, Callback, SpatialPredicateTag>
if (!traverse_left && !traverse_right)
{
- node = *--stack_ptr;
+ node = stack.top();
+ stack.pop();
}
else
{
node = traverse_left ? child_left : child_right;
if (traverse_left && traverse_right)
- *stack_ptr++ = child_right;
+ stack.push(child_right);
}
- } while (node != NULL);
+ } while (node != nullptr);
}
}; |
|
Performance results are really too good (1d26860 vs 61b8435): The hollow mesh radius search for 1M with CUDA is 57% faster! |
I can't find the exact patch now, but from my memory yes, that is what I tried. |
I disagree with this. We should instead merge and then open an issue to clean it up. This way, users of ArborX can achieve these improvements immediately, without placing a burden on us to spend time on this now.
The changes are the correct fix. However, enhancing the tests, as you propose, should also be done, thought not necessarily as a part of this PR. |
|
Confirmed that it slows things down with the proposed patch (ran on my workstation, (how to read: in each |
|
retest this please |
|
I also remember looking through the |
|
Tested HACC example with this patch, and it's about 20% faster. |
|
Out of curiosity, I also updated the knn algorithm to follow the same idea as radius, and it produced similar results (dramatically improving Cuda, just minor improvement to host backends) I also wonder about |
aprokop
changed the title
|
For the record, @dalg24 tried the following Stack implementation: struct MyStack {
Node const *data_[64];
Node const **ptr_ = data;
KOKKOS_FORCEINLINE_FUNCTION Node const *pop() noexcept { return *--ptr_; }
KOKKOS_FORCEINLINE_FUNCTION void push(Node const *p) noexcept { *ptr_++ = p; }
};and it still exhibits pathological behavior. |
|
@dalg24 Done. |
dalg24
reviewed
May 20, 2020
| // immediately popped out of the stack and processed. | ||
| stack.emplace(bvh_.getRoot(), 0.); | ||
| Node const *stack_node[64]; | ||
| float stack_dist[64]; |
There was a problem hiding this comment.
Did you try
Kokkos::pair<Node const *, float> stack[64];There was a problem hiding this comment.
Yes. It complained about not having the default constructor, or something like that.
|
Removed knn from this PR to facilitate faster merging. |
dalg24
approved these changes
May 20, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Comparing the current implementation with the original Karras algorithm.
I have spent little time figuring why the new one is so much faster, and whether it could be reimplemented using the existing
Stackstructures (I tried doing a straightforward replacement, but that immediately slowed down).Update: our current understanding of the problem is that having a struct results in register spilling.