[libcxx-commits] [PATCH] D132505: [libc++] Refactor deque::iterator algorithm optimizations

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I think this is a really really nice generalization. It's super elegant how the segmented iterator traits bridge perfectly with our existing deque iterator.

The speedup this provides for ranges is extremely valuable too, and it opens the door to doing the same for other segmented iterators (maybe `join_view`)?



================
Comment at: libcxx/benchmarks/CMakeLists.txt:171
     algorithms/ranges_push_heap.bench.cpp
     algorithms/ranges_sort.bench.cpp
     algorithms/ranges_sort_heap.bench.cpp
----------------
Not tied to this file: Let's add a release note. This is an interesting optimization and it will yield large performance benefits for all segmented iterators, so it's worth advertising.


================
Comment at: libcxx/benchmarks/deque_iterator.bench.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
I would be curious to try adding `_LIBCPP_ALWAYS_INLINE` to `__unwrap_and_dispatch` (and possibly other implementation details in that code path) to see what impact it has on the benchmarks. Could you try that out and report? I would assume that for raw pointers, that should all end up being inlined away.


================
Comment at: libcxx/include/__algorithm/copy_backward.h:49
+    using _Traits        = __segmented_iterator_traits<_InIter>;
+    auto __first_segment = _Traits::__segment(__first);
+    auto __last_segment  = _Traits::__segment(__last);
----------------
Just a matter of personal preference, but I find these names quite long (`__first_segment`, `__last_segment`). IMO it would be a bit easier to read with shorter names because of how the lines would be wrapped. I'd suggest `__sfirst` and `__slast`, which I think is idiomatic to segmented iterators based on the paper we looked at.


================
Comment at: libcxx/include/__algorithm/copy_backward.h:91
+      auto __local_last  = _Traits::__local(__result);
+      auto __size        = std::min<typename common_type<__iter_diff_t<_InIter>, __iter_diff_t<_OutIter> >::type>(
+          __local_last - __local_first, __last - __first);
----------------
Here and in a few other places, a `using _DiffT = typename common_type<__iter_diff_t<_InIter>, __iter_diff_t<_OutIter> >::type;` would increase readability.


================
Comment at: libcxx/include/__algorithm/copy_backward.h:93
+          __local_last - __local_first, __last - __first);
+      auto __iters = std::__copy_backward<_AlgPolicy>(__last - __size, __last, __local_last);
+      __last -= __size;
----------------



================
Comment at: libcxx/include/__algorithm/copy_backward.h:103-115
+    while (true) {
+      auto __local_first = _Traits::__begin(__segment_iterator);
+      auto __local_last  = _Traits::__end(__segment_iterator);
+      auto __size        = std::min<typename common_type<__iter_diff_t<_InIter>, __iter_diff_t<_OutIter> >::type>(
+          __local_last - __local_first, __last - __first);
+      auto __iters = std::__copy_backward<_AlgPolicy>(__last - __size, __last, __local_last);
+      __last -= __size;
----------------
I think you can pretty easily remove the special casing for the last segment by doing something like this. I'm not saying it's the best way to write it, but it should be possible to remove the special case.

Also, let's use `{}` on the `if` since it spans multiple lines.


================
Comment at: libcxx/include/__iterator/segmented_iterator.h:30
+// - static __segment_iterator Traits::__segment(It __it)
+//   Returns an iterator to the segment that the provided iterator is in.
+//
----------------
Indentation here and below doesn't match above.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp:60
+  {// simple test
+   {std::array in{1, 2, 3, 4};
+  std::array<int, 4> out;
----------------
The indentation is crazy here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132505