oneDPL icon indicating copy to clipboard operation
oneDPL copied to clipboard

Add workaround to parallel merge in TBB backend not to use input iterators default constructor

Open kboyarinov opened this issue 1 year ago • 6 comments

Add workaround to implementation of __parallel_merge function for the following use case:

#include <oneapi/dpl/algorithm>
#include <oneapi/dpl/execution>
#include <oneapi/dpl/iterator>
#include <vector>

int main() {
    std::vector<int> input1 = {1, 2, 3};
    std::vector<int> input2 = {1, 2, 3};
    std::vector<int> output = {0, 0, 0};

    auto no_transformation = [](int i) { return i; };

    oneapi::dpl::merge(oneapi::dpl::execution::par,
                       oneapi::dpl::make_transform_iterator(input1.begin(), no_transformation),
                       oneapi::dpl::make_transform_iterator(input1.end(), no_transformation),
                       input2.begin(),
                       input2.end(),
                       output.begin());
}

The first input iterator in merge algorithm is dpl::transform_iterator<vector_it, lambda>. Since lambda is not default constructible, this use case results in the compilation error while trying to default construct iterator inside TBB backend.

From the C++ Standard perspective, it is not an issue, because default constructability is part of ForwardIterator requirements, but after the short offline discussion, it was decided to workaround this place in parallel backend TBB to make things better for our fancy iterators.

There is still no 100% guarantee, that no issues would be present for transform iterator with lambda body, because some algorithms we use from the C++ Standard Library are still allowed to call default constructor on the input iterator, e.g. std::upper_bound). This PR was tested together with libstdc++ and no issues was found.

kboyarinov avatar Jan 15 '24 13:01 kboyarinov

I think we are able to fix this issue without changes in transform_iterator and it's more preferable: for that we should duplicate some code in if / else parts inside https://github.com/oneapi-src/oneDPL/pull/1346/files#diff-9908a4470cd112b8a7c7c87c5d3dfa425d873a5e5f9bf353268e6bdf756f5af8.

SergeyKopienko avatar Jan 15 '24 14:01 SergeyKopienko

What about the code

        // Using callable object instead of lambda here to ensure transform iterator would be
        // default constructible, that is part of the Forward Iterator requirements in the C++ standard.
        struct NoTransform
        {
            ValueType operator()(const ValueType& val) const
            {
                return val;
            }
        };

Should we use lambda now in https://github.com/oneapi-src/oneDPL/blob/5740f585386bed798af83e1c18ae1ee92c613637/test/parallel_api/iterator/permutation_iterator.h#L211 ?

SergeyKopienko avatar Jan 15 '24 14:01 SergeyKopienko

What about the code

        // Using callable object instead of lambda here to ensure transform iterator would be
        // default constructible, that is part of the Forward Iterator requirements in the C++ standard.
        struct NoTransform
        {
            ValueType operator()(const ValueType& val) const
            {
                return val;
            }
        };

Should we use lambda now in

https://github.com/oneapi-src/oneDPL/blob/5740f585386bed798af83e1c18ae1ee92c613637/test/parallel_api/iterator/permutation_iterator.h#L211

?

I would prefer to keep this test as-is because, as I mentioned this fix is not a 100% guarantee that merge with transform_iterator with lambda body would work. std::upper_bound and std::lower_bound implementations can still rely on default constructability of input iterators. For today, this code works, but formally such an iterator violates the ForwardIterator requirements from the Standard. This patch is intended to minimize the risk of issues, as we discussed offline.

kboyarinov avatar Jan 15 '24 15:01 kboyarinov

What about the code

        // Using callable object instead of lambda here to ensure transform iterator would be
        // default constructible, that is part of the Forward Iterator requirements in the C++ standard.
        struct NoTransform
        {
            ValueType operator()(const ValueType& val) const
            {
                return val;
            }
        };

Should we use lambda now in https://github.com/oneapi-src/oneDPL/blob/5740f585386bed798af83e1c18ae1ee92c613637/test/parallel_api/iterator/permutation_iterator.h#L211

?

I would prefer to keep this test as-is because, as I mentioned this fix is not a 100% guarantee that merge with transform_iterator with lambda body would work. std::upper_bound and std::lower_bound implementations can still rely on default constructability of input iterators. For today, this code works, but formally such an iterator violates the ForwardIterator requirements from the Standard. This patch is intended to minimize the risk of issues, as we discussed offline.

ok

SergeyKopienko avatar Jan 15 '24 15:01 SergeyKopienko

Should the patch also be done in PSTL upstream? @MikeDvorskiy @rarutyun

akukanov avatar Jan 26 '24 14:01 akukanov