[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.

I really like this patch in spirit! It should be rebased onto `main`, though.



================
Comment at: libcxx/include/__algorithm/copy.h:73
+    if (__range_size < __segment_size) {
+      auto __iters = std::__copy(__first, __last, __segment.__begin_);
+      __result += __range_size;
----------------
It seems to me that what you really need here is some kind of `__copy_up_to` primitive, where we would copy `[__first, as-far-as-we-can-go)` to `[__segment.__begin_, segment-end)`. Then you would not need to know the size of the input segment, and this optimization would apply to non-random-access iterators as well.

It might not be worth trying to optimize for non random-access input iterators anyway though, since we will end up having to loop over each element regardless inside `__copy_up_to`, which is not much better than what we'd do without applying any segmented optimization at all.


================
Comment at: libcxx/include/__iterator/segmented_iterator.h:12-26
+// segmented iterators are iterators which are made up of multiple other iterators.
+// For example, std::deque uses segments to store a few elements in a contiguous block and then have a new block
+// after that. To allow algorithms to optimize on this an iterator can specialize __segmented_iterator_traits.
+// If __segmented_iterator_traits::__is_segmented_iterator::value is true, the following functions have to be
+// implemented:
+// - `static <undefined> __get_forward_range(_Iterator __first, _Iterator __last);`
+//   where <undefined> is a range which contains the segments to iterate over. The segments are iterated front to back.
----------------
I think your definition of segmented iterators is an excellent starting point, however we could make the following changes based on prior art (http://lafstern.org/matt/segmented.pdf):

```
// Segmented iterators are iterators over (not necessarily contiguous) sub-ranges.
//
// For example, std::deque stores its data into multiple blocks of contiguous memory,
// which are not stored contiguously themselves. The concept of segmented iterators
// allows algorithms to operate over these multi-level iterators natively, opening the
// door to various optimizations. See http://lafstern.org/matt/segmented.pdf for details.
//
// Given Traits = __segmented_iterator_traits<It>, if Traits::__is_segmented_iterator::value
// is true, the following functions and associated types must be provided:
// - Traits::__local_iterator
//    The type of iterators used to iterate inside a segment.
//
// - Traits::__segment_iterator
//    The type of iterators used to iterate over segments.
//    Segment iterators can be forward iterators or bidirectional iterators, depending on the
//    underlying data structure.
//
// - static __segment_iterator Traits::__segment(It __it)
//   Returns an iterator to the segment that the provided iterator is in.
//
// - static __local_iterator Traits::__local(It __it)
//   Returns the local iterator pointing to the element that the provided iterator points to.
//
// - static __local_iterator Traits::__begin(__segment_iterator __it)
//   Returns the local iterator to the beginning of the segment that the provided iterator is pointing into.
//
// - static __local_iterator Traits::__end(__segment_iterator __it)
//   Returns the one-past-the-end local iterator to the segment that the provided iterator is pointing into.
```

Then, you can use it as:

```
void f(__deque_iterator<T> __first, __deque_iterator<T> __last) {
    using _Traits = __segmented_iterator_traits<__deque_iterator<T> >;

    Traits::__segment_iterator __sfirst = _Traits::__segment(__first);
    Traits::__segment_iterator __slast = _Traits::__segment(__last);
    use(Traits::__local(__first), Traits::__end(__sfirst)); // special case the first block
    for (++__sfirst; __sfirst != __slast; ++__sfirst) {
      use(Traits::__begin(__sfirst), Traits::__end(__sfirst));
    }
    use(Traits::__begin(__slast), Traits::__local(__last));
}
```

At a glance, I think we would implement the traits for `deque` something like:

```
template <class T>
struct __segmented_iterator_traits<__deque_iterator<T> > {
  struct __segment_iterator {
    // fun stuff
  };

  using __local_iterator = T*;

  static __segment_iterator __segment(__deque_iterator<T> __it) {
    return __segment_iterator(__it.__m_iter_);
  }

  static __local_iterator __local(__deque_iterator<T> __it) {
    return __it.__ptr_;
  }

  static __local_iterator __begin(__segment_iterator __it) {
    return *__it.__m_iter_;
  }

  static __local_iterator __end(__segment_iterator __it) {
    return *__it.__m_iter_ + BLOCK_SIZE;
  }
};
```


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move.pass.cpp:82
 
+template <class InContainer, class OutContainer, class In, class Out, class Sent = In>
+constexpr void test_containers() {
----------------
> Also fix `ranges::move` with contiguous iterators and non-iterator sentinels as a drive-by.

I would really prefer doing this separately. If @var-const 's patch does that already, can you simply double-check that your added tests pass now? Basically I want to make sure that you don't lose test coverage when you rebase on top of his change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132505