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

Fix a strange thing in parallel::detail::handle_exception. (Fix #2834.) #3089

Merged
merged 4 commits into from Jan 8, 2018

Conversation

Projects
None yet
2 participants
@taeguk
Copy link
Member

commented Jan 1, 2018

Fixes #2834 .
flyby: Fixes #3088 .

taeguk added some commits Jan 1, 2018

Fix a strange thing in parallel::detail::handle_exception.
There are some changes in parallel::sort for fixing #2834.
And this commit also fixes #3088.
@hkaiser

This comment has been minimized.

Copy link
Member

commented Jan 1, 2018

@taeguk Thanks for looking into this! It's overdue that you get access to the repository - I've sent you an invite.

@@ -201,19 +202,22 @@ namespace hpx { namespace parallel { inline namespace v1
result = execution::async_execute(policy.executor(),
&sort_thread<ExPolicy, RandomIt, Compare>,
std::ref(policy), first, last, comp);

return result;

This comment has been minimized.

Copy link
@hkaiser

hkaiser Jan 2, 2018

Member

Wouldn't a simple

return execution::async_execute(policy.executor(),  
    &sort_thread<ExPolicy, RandomIt, Compare>,  
    std::ref(policy), first, last, comp);

do the trick here?

This comment has been minimized.

Copy link
@taeguk

taeguk Jan 2, 2018

Author Member

@hkaiser You're right. That code was derived from just moving return result; into inside of try block. I'll fix that like you said.

catch (...) {
return hpx::make_exceptional_future<RandomIt>(
std::current_exception());
}

This comment has been minimized.

Copy link
@hkaiser

hkaiser Jan 2, 2018

Member

Hmmm, do we really have to use nested try/catch blocks here? handle_local_exceptions only rethrows the current exception as an exception list. Couldn't we directly call make_exceptional_future with a exception list constructed from the current exception?

This comment has been minimized.

Copy link
@taeguk

taeguk Jan 2, 2018

Author Member

@hkaiser handle_local_exceptions() handles an exception differently when std::current_exception() means std::bad_alloc. In that case, handle_local_exceptions() just rethrows std::bad_alloc without wrapping into exception_list.
We need to handle the exception differently according to what is the exception which we handles. And for that, I used handle_local_exception().
And nested catch (Line216-219) is needed for wrapping an exception which is threw properly from handle_local_exception().

This comment has been minimized.

Copy link
@hkaiser

hkaiser Jan 2, 2018

Member

@taeguk yes, I understand. Still, there might be a better way.

This comment has been minimized.

Copy link
@taeguk

taeguk Jan 2, 2018

Author Member

@hkaiser Aha, okay i got it.
https://github.com/taeguk/hpx/blob/9d72e4de54495987597bc2e6bbe609bc8125a01c/hpx/parallel/algorithms/sort.hpp#L249-L260
If we move that exception handling from Line208 to above lines, we can avoid nested try/catch blocks with just calling detail::handle_exception().

catch (hpx::exception_list const& el) {
throw el;
catch (hpx::exception_list const&) {
throw;
}
catch (...) {
throw hpx::exception_list(std::current_exception());

This comment has been minimized.

Copy link
@hkaiser

hkaiser Jan 2, 2018

Member

Hmm, this looks really strange (it probably was strange all along). I'm sure this can be done with one level of try/catch blocks.

This comment has been minimized.

Copy link
@taeguk

taeguk Jan 2, 2018

Author Member

@hkaiser Yes, we can do that with only one level of try/catch blocks.

taeguk added some commits Jan 7, 2018

Avoid nested try/catch in hpx::parallel::detail::handle_exception.
flyby: Consistent usage between exception_list and hpx::exception_list.
@hkaiser

hkaiser approved these changes Jan 7, 2018

Copy link
Member

left a comment

Very nice, thanks a lot!

@hkaiser hkaiser merged commit 092422f into STEllAR-GROUP:master Jan 8, 2018

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
pycicle daint-6.0.0 Build errors 0
Details
pycicle daint-6.0.0 Config errors 0
Details
pycicle daint-6.0.0 Test errors 0
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.