[libcxx-commits] [PATCH] D128416: [libc++] Implement the deque functions of P1206R7

[Louis Dionne via Phabricator via libcxx-commits]

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

This is an awesome patch! I think there's a bunch of stuff that needs to be investigated per our live review just now, but we will all end up with a better understanding of how `deque` works so that's great.



================
Comment at: libcxx/include/__algorithm/iterator_operations.h:47
 
+  template <class _Iterator>
+  _LIBCPP_HIDE_FROM_ABI static _LIBCPP_CONSTEXPR_AFTER_CXX11
----------------
I think you'll find that you don't need this anymore once you rebase.


================
Comment at: libcxx/include/__ranges/from_range_t.h:18
+
+#if _LIBCPP_STD_VER > 20
+
----------------
This will soon not be useful anymore, but I think we should do it for consistency.


================
Comment at: libcxx/include/__split_buffer:121-126
+    template <class _InputIter, class _Sent>
     __enable_if_t<__is_exactly_cpp17_input_iterator<_InputIter>::value>
-        __construct_at_end(_InputIter __first, _InputIter __last);
-    template <class _ForwardIterator>
+        __construct_at_end(_InputIter __first, _Sent __last);
+    template <class _ForwardIterator, class _Sent>
     __enable_if_t<__is_cpp17_forward_iterator<_ForwardIterator>::value>
+        __construct_at_end(_ForwardIterator __first, _Sent __last);
----------------
I think we should make those `_LIBCPP_HIDE_FROM_ABI`. Especially since we're modifying them in a way that might not be backwards compatible if this had leaked into a user's ABI.


================
Comment at: libcxx/include/deque:1339
+
+#if _LIBCPP_STD_VER > 20
+
----------------
`&& _LIBCPP_HAS_NO_INCOMPLETE_RANGES`, here and elsewhere. We'll bulk-remove those in the future.


================
Comment at: libcxx/include/deque:1385-1394
+    auto __first1 = ranges::begin(__range);
+    auto __last1  = ranges::end(__range);
+    auto __first2 = begin();
+    auto __last2  = end();
+
+    while (__first1 != __last1 && __first2 != __last2) {
+      *__first2 = *__first1;
----------------
We could factor something out here, it seems like a potentially useful algorithm to me (in fact I thought the stdlib already had this algorithm):

```
template <class _Iter1, class _Sent1, class _Iter2, class _Sent2>
std::pair<_Iter1, _Iter2> __copy_shortest(_Iter1 __it1, _Sent1 __sent1, _Iter2 __it2, _Sent2 __sent2) {
    while (__first1 != __sent1 && __first2 != __sent2) {
      *__first2 = *__first1;
      ++__first1;
      ++__first2;
    }
    return {__first1, __first2};
}
```

And in fact, we can optimize the above for `sized_sentinel`s, which would get rid of the need for the overload of `assign_range` we have below. And that would not add as many new dependencies to `<deque>` (`views::take` and friends).

--------------------------------------------------------------

Alternatively, per our live discussion, if you want to keep only the overload below (with the removed `sized_range` constraint), I'd be OK with that, BUT:

1. We need to show that there is no significant compile-time impact to existing users of `<deque>`. In particular, if we end up pulling in a significant part of `<ranges>` just to implement this function, that doesn't work.

2. We need to show that the code generated is comparable to what I suggested above (and if we added an optimization for sized sentinels that basically lowered to `std::copy` aka `memmove`). If that's not the case, then we should use the lower-level version, and also file a bug against Clang so we can find ways to improve the codegen of ranges. Note that this exercise it useful on its own.

Yeah, sorry, I know I'm a pain but I think the bar for using higher-level and more complicated constructs from lower-level ones should be high, and that's what I'm enforcing here.


================
Comment at: libcxx/include/deque:1410
+      if constexpr (ranges::sized_range<_Range>) {
+        append_range(ranges::subrange(__it.in.base(), ranges::end(__range), ranges::distance(__range) - size()));
+      } else {
----------------
Any reason for not using this? IMO it's strictly clearer and it skips one level of indirection through `ranges::distance` if we already have a `sized_range`. And if we don't have a `sized_range`, we definitely don't want to be calling `ranges::distance` over it!


================
Comment at: libcxx/include/deque:1511
+    if constexpr (ranges::sized_range<_Range>) {
+      auto __n        = ranges::distance(__range);
+      auto __back_cap = __back_spare();
----------------



================
Comment at: libcxx/include/deque:1516-1517
+    }
+    for (auto&& __e : __range)
+      emplace_back(std::forward<decltype(__e)>(__e));
+  }
----------------
When you have a `sized_range`, we should be able to use `std::uninitialized_allocator_copy` here with `__deque_iterator`, and update the size at the end only. When we don't have a `sized_range`, the `for` loop with `emplace_back` seems like the best we can do.

Also, can you file a bug report to track creating a high-level libc++-internal abstraction for chunked iterators? Ideally, we should be able to generalize the overloads of `std::copy` & friends we have at the top of this file, and also use `std::uninitialized_allocator_copy` here, and have it do just the right thing. Then, once we use that abstraction in `uninitialized_allocator_copy`, we'd get improved perf for free.


================
Comment at: libcxx/include/deque:1524
+      auto __front_cap = __front_spare();
+      auto __dist      = ranges::distance(__range);
+      if (difference_type(__front_cap) < __dist)
----------------



================
Comment at: libcxx/include/deque:1529-1538
+    if constexpr (ranges::bidirectional_range<_Range>) {
+      for (auto&& __e : __range | views::reverse)
+        push_front(std::forward<decltype(__e)>(__e));
+    } else if constexpr (ranges::sized_range<_Range>) {
+      auto __dist = ranges::distance(__range);
+      std::__uninitialized_allocator_copy(
+          get_allocator(), ranges::begin(__range), ranges::end(__range), begin() - __dist);
----------------
I think you should be checking for a `sized_range` before you check for a `bidirectional_range`.


================
Comment at: libcxx/include/deque:1539
+    } else {
+      vector<value_type, allocator_type> __buf(get_allocator());
+      for (auto&& __e : __range)
----------------
You are missing a `<vector>` include here, and I must admit that makes me frown.

This also leads me to think that you should instead use `__split_buffer` here, that seems like the class used in both `vector` and `deque` to do these things. Furthermore, it should be possible in theory to instead do this:
1. Allocate a buffer and append the elements to it
2. Link that buffer to the beginning of the `deque` directly, to avoid having to reallocate and copy elements twice
3. Copy just the `n` elements at the end of that buffer to the original beginning of the `deque`.

As you go through this exercise, it might be useful to take some notes about how the data structure works and we can promote them to documentation in the future.


================
Comment at: libcxx/include/deque:1582
+    if constexpr (ranges::sized_range<_Range>)
+      __n = ranges::distance(__range);
+    __split_buffer<value_type, allocator_type&> __buf(__n, 0, get_allocator());
----------------



================
Comment at: libcxx/include/deque:1585
+    __buf.__construct_at_end(ranges::begin(__range), ranges::end(__range));
+    return __insert(__position, move_iterator(__buf.begin()), move_iterator(__buf.end()));
+  }
----------------
I have the same kind of hunch as above here -- why can't we reuse the buffer we just allocated, at least in part?


================
Comment at: libcxx/include/deque:1742
 
+#if _LIBCPP_STD_VER > 20
+
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128416