Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove owning_view #1757

Open
wants to merge 3 commits into
base: distributed-ranges
Choose a base branch
from

Conversation

akukanov
Copy link
Contributor

@akukanov akukanov commented Aug 2, 2024

@BenBrock @lslusarczyk - this patch proposes to completely remove the custom implementation of owning_view.

For the context, that implementation is used as the return type for views::zip::segments() - it wraps a vector of views paired with a rank that the function builds. No other use of owning_view exists in the proposed oneDPL patch, but in the standalone DR repo it is also used for the same purpose by distributed_dense_matrix::segments().

A comment in detail/owning_view.hpp says that the custom implementation of owning_view there is needed only for ranges-v3. However, when I tried to replace it to std::ranges::owning_view, it resulted in compilation errors, for example (many details omitted):

/home/runner/work/oneDPL/oneDPL/test/distributed_ranges/sp/../include/common_tests.hpp:214:15:
error: no matching function for call to object of type 'const _Join'
  214 |   auto flat = stdrng::views::join(segments);
      |               ^~~~~~~~~~~~~~~~~~~
...
/usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/ranges:3224:2:
note: candidate template ignored: constraints not satisfied
[with _Range = stdrng::owning_view<vector<zip_view<span<unsigned int, _Iterator>, remote_span<int, device_ptr<int>>>>> &]
 3224 |         operator() [[nodiscard]] (_Range&& __r) const
      |         ^
/usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/ranges:3221:16:
note: because 'std::ranges::owning_view< ... > &' does not satisfy 'viewable_range'
 3221 |       template<viewable_range _Range>
      |                ^
/usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/ranges_base.h:809:41:
note: because 'constructible_from<remove_cvref_t<owning_view< ... > &>, std::ranges::owning_view< ... > &>'
evaluated to false
  809 |       && ((view<remove_cvref_t<_Tp>> && constructible_from<remove_cvref_t<_Tp>, _Tp>)
      |                                         ^
...

The error happens because std::ranges::owning_view is not copyable: see https://en.cppreference.com/w/cpp/ranges/owning_view#ctor. Therefore an lvalue of owning_view is not a viewable_range (see the above log) and cannot be used as an argument to construct std::views::all as well as other views (as far as I can see, most views rely on all at least for CTAD).

The custom implementation of owning_view in DR does not delete the copy constructor and therefore has no such problem, but consequently it is not compliant to the standard definition of owning_view.

So, if the custom owning_view in DR is replaced with the standard one, then storing the result of ranges::segments() into a variable and then building a view over that variable (as in the test code below) does not work:

  auto segments = dr::ranges::segments(r);
  auto flat = stdrng::views::join(segments); // the compilation error shown in the log above

However it is not quite clear to me why views::zip::segments() wrap the vector it builds into owning_view before returning. I guess the idea could have been that ranges::segments() applied to a view must return a view as well. or maybe you wanted ranges::segments() to be directly used in a view pipe. As far as I can tell, however, the method segments() of your distributed containers returns a copy of their vector of segments. So, unless I miss something, returning a vector of segments without wrapping it into owning_view should seemingly be just fine.

@akukanov akukanov requested review from lslusarczyk and removed request for lslusarczyk August 2, 2024 22:22
@akukanov akukanov force-pushed the dev/dr-remove-owning-view-akukanov branch from 91789d4 to 84aa166 Compare August 5, 2024 12:50
@akukanov akukanov changed the title Rework the use of ranges-v3 and remove owning_view Remove owning_view Aug 5, 2024
@akukanov akukanov force-pushed the dev/dr-remove-owning-view-akukanov branch from 23ce634 to e732a08 Compare August 5, 2024 23:35
@akukanov akukanov marked this pull request as ready for review August 6, 2024 01:00
@lslusarczyk
Copy link
Contributor

lslusarczyk commented Aug 6, 2024

Both DR tests timed out on "Zip/0.Drop" on this change. It needs to be checked if code does not hang now in this case.

@akukanov
Copy link
Contributor Author

akukanov commented Aug 6, 2024

I will take a look at the test. Yet, I would like to have some pros & cons discussion as well, as I might easily miss some reasons for owning_view to really be the proper way to return the segments.

@lslusarczyk
Copy link
Contributor

I will take a look at the test. Yet, I would like to have some pros & cons discussion as well, as I might easily miss some reasons for owning_view to really be the proper way to return the segments.

Sure. I have to look and @BenBrock too. It would be great to simplify the code and remove not needed parts. I had no enough time to rereview owning-view so I only managed to spot this failing test.

@BenBrock
Copy link
Contributor

BenBrock commented Aug 6, 2024

The issue here is that we have some functions that create views of segments. They take in a range of segments and return a modified view of those segments (e.g. take_segments on line 33 of segments_tools.hpp).

These functions are implemented, naturally, using standard range adaptors like enumerate, take, and transform. Those range adaptors must store a view, which they create using views::all (the stored view being of type views::all_t). If you pass in an lvalue to an owning range (like a vector of device_spans), it will create a ref_view, assuming that the owning range will outlive the lifetime of the newly created ref_view.

I assume this is what's causing problems in the PR---we assume that segments(my_zip_view) returns a view and use an lvalue reference to it to create more views. This can be fixed by either ensuring segments() always returns a view (my preference), or by modifying the code to never use an lvalue reference to the return of segments() (somewhat trickier, and the user could potentially always violate this).

My preference would probably be to simplify using std::views::owning_view. I don't think we have significant uses of copying segment views. If I remember correctly I primarily added a copy constructor because mhp needed it.

@akukanov
Copy link
Contributor Author

akukanov commented Aug 7, 2024

Ben's response gave me the clue for where to look. Apparently, the implementation of take_ & drop_segments does not properly handle rvalue arguments. Simply adding std::forward fixed the tests.

The semantical questions remain though - what should users expect from ranges::segments(dr)? Does it return a view, a range of views, or a range of ranges (assuming that a view is a range, but not vice versa)? Is the returned type copyable, movable but not copyable, or it depends on the range?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants