[libcxx-commits] [PATCH] D99849: [pstl] Fix a number of bugs with parallel partial sort

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: pstl/include/pstl/internal/parallel_backend_omp.h:556-558
+    bool __empty() { return __smallest_k_items.empty(); }
 
+    auto __size() { return std::size(__smallest_k_items); }
----------------
As long as you're changing these anyway, might as well simplify.


================
Comment at: pstl/include/pstl/internal/parallel_backend_omp.h:569
 
-template <typename _RandomAccessIterator, typename _Compare>
-struct _MinKOp
-{
-    _MinKItems<_RandomAccessIterator>& __items;
+template <typename _RandomAccessIterator, typename _Compare> struct _MinKOp {
+    _MinKItems<_RandomAccessIterator> &__items;
----------------
New lines 577, 570, 573: seems like you should undo these whitespace diffs (manually if necessary).

I don't suppose it would make sense to use a heap instead of linearly scanning the vector in `__update_largest_item` every time one is inserted? I mean, that's the whole point of non-parallel `partial_sort`... but I guess the idea here is that rearranging the elements once would be less cache-friendly than linearly scanning them multiple times?


================
Comment at: pstl/include/pstl/internal/parallel_backend_omp.h:600-601
+            [this](const auto &l, const auto &r) { return __comp(*l, *r); });
         __items.__largest_item =
-            std::max_element(std::begin(__items.__smallest_k_items), std::end(__items.__smallest_k_items),
-                             [this](const auto& l, const auto& r) { return __comp(*l, *r); });
+            std::distance(std::begin(__items.__smallest_k_items), pos);
     }
----------------
if you wanted to keep this simple


================
Comment at: pstl/include/pstl/internal/parallel_backend_omp.h:619
+             __tracking_it != end(__items.__smallest_k_items);
+             ++__item_it, ++__tracking_it) {
             *__tracking_it = __item_it;
----------------
That this ever worked indicates that we could use some more "evil" test cases with overloaded/deleted `operator,` and so on. The iterators in `libcxx/test/support/test_iterators.h` are already "evil" in this way; we'd just need some new tests that use those iterators with parallel algorithms.

My ADL senses are tingling with those ADL calls to `begin` and `end`.

(Are there tests for parallel `partial_sort`? If it's this buggy, why aren't those tests failing now?)


================
Comment at: pstl/include/pstl/internal/parallel_backend_omp.h:630
+                         _MinKItems<_RandomAccessIterator> &__v2, _Compare __comp)
+    -> _MinKItems<_RandomAccessIterator> {
+        if (__v1.__empty()) {
----------------
Gratuitous whitespace diff here lost the indentation of the trailing return type.


================
Comment at: pstl/include/pstl/internal/parallel_backend_omp.h:760
                 auto __swap_item = std::next(__xs, __swap_index);
-                std::swap(__current_item, __swap_item);
+                std::swap(*__current_item, *__swap_item);
                 break;
----------------
I think this was meant to call `std::iter_swap(__current_item, __swap_item)`, actually. Calling `std::swap`  qualified seems like a bug, but calling `std::iter_swap` qualified would be normal practice.


================
Comment at: pstl/include/pstl/internal/parallel_backend_omp.h:859
     {
-        __parallel_stable_sort_body(__xs, __pivot, __comp);
+        __parallel_stable_sort_body(__xs, __part_end, __comp);
     }
----------------
Technically, all of these ADL calls are unsafe and should be qualified, like `__omp_backend::__parallel_stable_sort_body(...)`. You're changing so much in the PR currently that I feel like you should go ahead and ADL-proof it too; but I wouldn't make ADL the //main// point of the PR. ;)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99849/new/

https://reviews.llvm.org/D99849