[libcxx-commits] [PATCH] D142335: [libc++][ranges] Partially implement `ranges::to`.
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Apr 13 22:31:35 PDT 2023
var-const marked an inline comment as done.
var-const added a comment.
Note: I finished the tests (at least in the amount that I had planned originally). I still need to fix the CI.
================
Comment at: libcxx/include/__ranges/to.h:98
+ __iterator_traits_detail::__cpp17_input_iterator<iterator_t<_Range>> &&
+ constructible_from<_Container, iterator_t<_Range>, sentinel_t<_Range>, _Args...>
+ ) {
----------------
ldionne wrote:
> Can you add tests that would fail if this didn't short-circuit properly? TBH I think those tests would fail right now.
Thanks for pointing this out! Added tests.
================
Comment at: libcxx/include/deque:604
+ : __map_(__pointer_allocator(__a)), __start_(0), __size_(0, __a) {
+ if constexpr (ranges::sized_range<_Range>) {
+ __append_n(ranges::begin(__range), ranges::size(__range));
----------------
ldionne wrote:
> In the case of `deque`, it's not clear to me that we gain much from using `__append_n`. In both cases, we end up performing `N / CHUNK_SIZE` allocations and then copying the whole input range into these allocations. Maybe worth measuring? I think looking at how `__add_back_capacity(size_type __n)` and `__add_back_capacity()` differ will provide some additional insights here.
>
> Also, if we end up keeping this optimization, the `if constexpr` condition seems a bit off. What should we do for a `forward_range` that is not a `sized_range`? We'll have to determine whether we want to take the hit to compute `distance(rng)` and then call `__append_n`, or not.
I did a very quick benchmark:
```
template <class Container, class GenInputs>
void BM_ConstructFromRangeT(benchmark::State& st, Container, GenInputs gen) {
auto in = gen(st.range(0));
benchmark::DoNotOptimize(&in);
while (st.KeepRunning()) {
Container c(std::from_range, in);
DoNotOptimizeData(c);
}
}
BENCHMARK_CAPTURE(BM_ConstructFromRangeT,
deque_int,
std::deque<int>{},
getRandomIntegerInputs<int>)->Arg(5140480 * 10);
BENCHMARK_CAPTURE(BM_ConstructFromRangeT,
deque_string,
std::deque<std::string>{},
getRandomStringInputs)->Arg(1024 * 10);
BENCHMARK_MAIN();
```
I ran it with and without the optimization 5 times and took the average (going by the wall time but the difference with CPU time is negligible):
```
Test Time, ns
ints, with optimization 34045820
ints, no optimization 67872734
strings, with optimization 2057941
strings, no optimization 2000087
```
So for strings, the difference is negligible, presumably because the timings are dominated by copying (in fact, the optimized version is 3% slower, but I'd chalk it up to random fluctuations), but for ints the optimized version is 2x faster, making it worthwhile to keep.
================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp:48
+static_assert(!std::ranges::view<ContainerT>);
+static_assert(HasTo<ContainerT, InputRange>);
+static_assert(!HasTo<test_view<forward_iterator>, InputRange>);
----------------
var-const wrote:
> ldionne wrote:
> > I think this is technically ill-formed according to the spec. I wonder whether it makes sense to test the exact way in which it is ill-formed as we do here (probably not).
> >
> > Also note that if that's correct and this is ill-formed, then technically we could also be SFINAE friendly as a QOI matter. I don't think we should unless it's simple and other implementations are doing it as well.
> Sorry, but at this point I need a reminder on why this is ill-formed.
@ldionne Tagging this comment so that it doesn't get drowned out among the many other comments. :)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142335/new/
https://reviews.llvm.org/D142335
More information about the libcxx-commits
mailing list