Remove fancy iterators #852
Conversation
|
@pca006132 Before I go too much further, do you have a sense of how much conflict this will generate with #833? Perhaps we should merge that first if it's ready soon. |
|
Okay, I've removed zip iterators completely and done some other cleanup, like removing |
|
Nice, will review later. |
There was a problem hiding this comment.
Not many comments here, I just had a glimpse over it. The new code is easier to read, which is always a nice bonus when we refactor our code :)
Will work on implementing the transform iterator, as well as maybe give some simple benchmark result to see if this is having unexpected impact on performance considering we change those sort on zip iterators to sort + permute
| @@ -154,15 +154,13 @@ struct CreateRadixTree { | |||
|
|
|||
| template <typename T, const bool selfCollision, typename Recorder> | |||
| struct FindCollisions { | |||
| VecView<const T> queries; | |||
| VecView<const Box> nodeBBox_; | |||
| VecView<const thrust::pair<int, int>> internalChildren_; | |||
There was a problem hiding this comment.
Thinking if we can remove this thrust::pair as well, maybe with std::pair?
Not really related to fancy iterator, but I feel that the reason for using thrust::pair is mostly because of zip.
| if (reverse) | ||
| transform(policy, w03.begin(), w03.end(), w03.begin(), | ||
| thrust::negate<int>()); | ||
| for_each_n(policy, countAt(0), s02.size(), |
| stable_sort( | ||
| policy, ids.begin(), ids.end(), | ||
| [&edge](const int& a, const int& b) { return edge[a] < edge[b]; }); | ||
|
|
There was a problem hiding this comment.
I am curious if we can skip the edge tmp buffer completely, and compute the edge information on-the-fly. Maybe we can try a benchmark later, not critical.
There was a problem hiding this comment.
Yeah, I never liked that either; I couldn't find another way before, but if you have an idea, that would be great.
There was a problem hiding this comment.
FYI: I tried this, it is slightly slower.
|
And a general comment: maybe we should use |
|
FYI: there is basically no performance different. |
pca006132
force-pushed
the
banishIterators
branch
from
ed26cf7 to
680f233
Compare
|
It seems that thrust is doing something non-trivial in their copy function, causing issues with our simple custom iterator implementations. I don't want to dig into the details of that as we will be getting rid of them later anyway, so I just switch to the sequential implementation for now if there is no pstl support. |
|
Thank you, this looks excellent! Let's go ahead and merge this and remove thrust::pair and such in the follow-up. |
|
actually that was done in another PR :) |
This refactor will aid #823 - by removing zip iterators all we'll need is a counting iterator, which will reduce the amount of Thrust we rely on. Frankly, I also feel like this cleans up the code a little anyway - I always hated those tuples.
I've only done a few files so far, but it's going pretty quickly. Should be no big deal - just wanted to show my progress.