-
-
Notifications
You must be signed in to change notification settings - Fork 437
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
Contains and contains_subrange parallel algorithm implementation GSOC 2024 #6497
Contains and contains_subrange parallel algorithm implementation GSOC 2024 #6497
Conversation
Can one of the admins verify this patch? |
template <typename ExPolicy, typename Iterator, typename Sentinel, | ||
typename T, typename Proj> | ||
static util::detail::algorithm_result_t<ExPolicy, bool> parallel( | ||
ExPolicy&& policy, Iterator first, Sentinel last, const T& val, | ||
Proj&& proj) | ||
{ | ||
const auto distance = detail::distance(first, last); | ||
if (distance <= 0) | ||
{ | ||
return util::detail::algorithm_result<ExPolicy, bool>::get( | ||
false); | ||
} | ||
|
||
const auto itr = util::loop_pred< | ||
std::decay_t<ExPolicy>>(first, last, [&val, &proj] | ||
(const auto& cur) { return HPX_INVOKE(proj, *cur) == val; }); | ||
|
||
if (itr == last) | ||
{ | ||
return util::detail::algorithm_result<ExPolicy, bool>::get( | ||
false); | ||
} | ||
|
||
return util::detail::algorithm_result<ExPolicy, bool>::get(true); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably work-in-progress, but I just want to mention that util::loop_pred doesn't actually use the execution policy to run in parallel, so this will not run in multiple threads. You'll have to use a partitioner, which splits the work and launches it on multiple (the equal
algorithm is a a nice example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I was unaware at the time I wrote that, I should have been more mindful of what I was using. I checked out equal algorithm and I made some changes that now use the partitioner in the parallel overload. Also, I was hesitant to do this but I created a file in the parallel/algorithms/detail called contains.hpp, holding the implementation details for the algorithm.
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
1f8897c
to
f2496c9
Compare
@Zak-K-Abdi: Please take a look at the clang-format problems (https://gist.github.com/hkaiser/26bced03497017f481dbb9ee7d0df67c) and the cmake issues:
Also here is the list of inspect issues reported: /libs/core/algorithms/include/hpx/parallel/algorithms/contains.hpp: END file doesn't end with a newline, Endline Whitespace: 74, C, Lic, SPDX-Lic |
7b69479
to
711ca44
Compare
711ca44
to
38017ef
Compare
38017ef
to
54b3d7a
Compare
and tests for both algorithms.
rather than using .get()
for cleaner code
static hpx::future<bool> get(hpx::future<T>& itr, T& last) | ||
{ | ||
return itr.then( | ||
[&last](hpx::future<T> it) { return it.get() != last; }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pansysk75 Hi Panos, I'm not sure if this is correct, could you check this line of code I changed? I changed the helper function to return hpx::future, a continuation to the previous future, by using .then().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks right, i think you might want to capture last
by value and not by reference, as it is a local variable of the parallel
function. Otherwise looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pansysk75 Just to make sure, should I also pass last by value for the functions in my contains_subrange_helper_class or only for the lambda?
@@ -148,8 +137,7 @@ namespace hpx::parallel { namespace detail { | |||
HPX_FORWARD(Proj2, proj2)); | |||
|
|||
return util::detail::algorithm_result<ExPolicy, bool>::get( | |||
contains_subrange_helper<ExPolicy, FwdIter1>().get(itr) != | |||
last1); | |||
contains_subrange_helper<ExPolicy, FwdIter1>().get(itr, last1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think returning contains_subrange_helper<ExPolicy, FwdIter1>().get(itr, last1)
would suffice here.
The return types of your helper function already match with algorithm_result_t
. Calling that additional ::get
will call .get()
on your future before returning it, which undermines your latest change on your helper.
125867c
to
bdbd643
Compare
@Zak-K-Abdi Could you please address this: #6497 (comment) |
Apologies, I completely forgot about that comment. I will make that change soon. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Implementing parallel algorithms for contains and contains_subrange. GSOC 2024 proposal.
Will attempt to fix hpx::search so hpx::contains_subrange can make a call to hpx::search.