This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Enable segmented iterator optimizations for join_view::iterator
ClosedPublic

Authored by philnik on Nov 21 2022, 3:44 AM.

Diff Detail

Event Timeline

philnik created this revision.Nov 21 2022, 3:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 21 2022, 3:44 AM
philnik requested review of this revision.Nov 21 2022, 3:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 21 2022, 3:44 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Here are the benchmark results:

------------------------------------------------------------
Benchmark                                old             new
------------------------------------------------------------
BM_join_view_in_vectors/0           0.701 ns         3.47 ns
BM_join_view_in_vectors/1            16.6 ns         3.75 ns
BM_join_view_in_vectors/2            33.7 ns         5.66 ns
BM_join_view_in_vectors/64           1052 ns          172 ns
BM_join_view_in_vectors/512          8255 ns         1400 ns
BM_join_view_in_vectors/1024        16603 ns         2858 ns
BM_join_view_in_vectors/4000        64662 ns        12452 ns
BM_join_view_in_vectors/4096        66348 ns        12795 ns
BM_join_view_in_vectors/5500        89765 ns        17645 ns
BM_join_view_in_vectors/64000     1234386 ns       617878 ns
BM_join_view_in_vectors/65536     1269340 ns       600301 ns
BM_join_view_in_vectors/70000     1343142 ns       684041 ns
BM_join_view_out_vectors/0          0.699 ns        0.736 ns
BM_join_view_out_vectors/1           16.1 ns         3.79 ns
BM_join_view_out_vectors/2           31.6 ns         5.93 ns
BM_join_view_out_vectors/64           999 ns          161 ns
BM_join_view_out_vectors/512         7932 ns         1457 ns
BM_join_view_out_vectors/1024       15976 ns         2920 ns
BM_join_view_out_vectors/4000       62218 ns        13179 ns
BM_join_view_out_vectors/4096       63710 ns        13419 ns
BM_join_view_out_vectors/5500       85663 ns        18292 ns
BM_join_view_out_vectors/64000    1207589 ns       559316 ns
BM_join_view_out_vectors/65536    1242416 ns       590786 ns
BM_join_view_out_vectors/70000    1257713 ns       683171 ns
BM_join_view_deques/0                3.08 ns         16.1 ns
BM_join_view_deques/1                92.9 ns         19.4 ns
BM_join_view_deques/2                 195 ns         26.2 ns
BM_join_view_deques/64               5703 ns          251 ns
BM_join_view_deques/512             43109 ns         2188 ns
BM_join_view_deques/1024            88938 ns         5401 ns
BM_join_view_deques/4000           360749 ns        27811 ns
BM_join_view_deques/4096           371800 ns        28080 ns
BM_join_view_deques/5500           501057 ns        38059 ns
BM_join_view_deques/64000        10414050 ns      2155094 ns
BM_join_view_deques/65536        10813164 ns      2132998 ns
BM_join_view_deques/70000        11620544 ns      2395421 ns
ldionne requested changes to this revision.Nov 21 2022, 9:59 AM

This is a neat optimization! I do have some requests for changes, but once those are addressed I think we're on the right track.

libcxx/include/__iterator/iterator_with_data.h
21

This is definitely more complicated, but we need to make this an actual iterator.

libcxx/include/__ranges/join_view.h
72–73

The reason why you're making this change is that otherwise, you can't specialize

template <class ???>
struct __segmented_iterator_traits<???>

Naively, one would want to write

template <class _View, bool _Const>
struct __segmented_iterator_traits<ranges::join_view<_View>::__iterator<_Const>>

but obviously that doesn't work because one cannot specialize on a dependent type, which __iterator is. Technically, it would be possible to use a enable_if that checks for a nested member of the type (e.g. __is_join_view_iterator), but that's not great and it introduces a SFINAE path whenever we instantiate these traits, which is bad for compile-times.

The other option is what you've done here: define the iterator and sentinel types outside of the class. I think that's much better, however it's an ABI break. Fortunately we don't care about this for now because we're still shipping ranges experimentally, but I think we should make this change for all of our existing (and upcoming) views right now to future-proof them.

CC @var-const @huixie90 @Mordante for awareness

272–273

Can this constructor be private? We don't want to expose non-standard APIs, and this iterator type is public, even though its name isn't public.

403–404

This would avoid forming a __segmented_iterator_traits<It> specialization when it would be nonsensical.

405

There's too much iterator-y stuff going on, let's be explicit about this.

428–430

Suggestion? I was thrown off by the *__iter.__inner_;, I thought you were dereferencing the iterator instead of the optional.

libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp
124

I am worried about this test file becoming a mess if we start putting tests for all the segmented iterators here. Could we perhaps extract them to test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.segmented.pass.cpp instead, along with the deque tests?

This revision now requires changes to proceed.Nov 21 2022, 9:59 AM
philnik updated this revision to Diff 479049.Nov 30 2022, 12:32 PM
philnik marked 6 inline comments as done.

Address comments

libcxx/include/__ranges/join_view.h
428–430

Unfortunately optional::or_else only available in C++23.

huixie90 added inline comments.Dec 3 2022, 10:11 AM
libcxx/include/__iterator/iterator_with_data.h
44

this can be const?

46

do you need to && qualify this member? otherwise how can we know it can be only called once

79

this is a bit non-standard. Could you please briefly explain why we have a non-const operator*

81–82

questions: do we need iter_move and iter_swap?

philnik updated this revision to Diff 489179.Jan 13 2023, 6:22 PM
philnik marked 4 inline comments as done.

Address comments

libcxx/include/__iterator/iterator_with_data.h
79

I'm not sure. Removed it.

81–82

I'm not sure. Added them for completeness.

philnik updated this revision to Diff 489663.Jan 16 2023, 6:14 PM

Try to fix CI

ldionne accepted this revision.Jan 19 2023, 8:29 AM
ldionne added inline comments.
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp
126

Can you please make sure that we have a test that uses e.g. std::copy with a join_view_iterator that is *not* a segmented iterator?

This revision is now accepted and ready to land.Jan 19 2023, 8:29 AM
ldionne added inline comments.Jan 19 2023, 8:30 AM
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp
126

Specifically, please mess up the constraint on purpose and see what fails. If nothing fails, we're missing coverage and we need to add it.

philnik updated this revision to Diff 490633.Jan 19 2023, 12:39 PM
philnik marked 2 inline comments as done.

Try to fix CI

This revision was landed with ongoing or failed builds.Jan 19 2023, 10:56 PM
This revision was automatically updated to reflect the committed changes.