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

Variadic executor parameters #2204

Merged
merged 28 commits into from Jun 27, 2016

Conversation

mcopik
Copy link
Contributor

@mcopik mcopik commented Jun 7, 2016

This is my first attempt to provide a syntax for multiple executor parameters passed through a variadic template. I'm not completely happy with this solution, maybe there is a better way to implement unwrapping, I tried to find a way with the least amount of code duplication (which is already an unsolvable problem in execution policies).

The implementation creates a single parameter through variadic inheritance, which obviously prohibits duplication of parameter types; an additional static_assert in constructor provides a better error information than a compiler complaining about ambiguous choice between functions. Another possible option could be to use boost::hana (since 1.61) and implement it using compile-time algorithms on a tuple. I don't see any other way.

The biggest trick was to implement a support for parameters passed as a reference wrapper (Boost or C++11). For that purpose we need an unwrapper which API exposes only methods defined in the wrapped parameter; solution similar to executor_parameter_traits would cause another name clash between different parameters. Unfortunately, the problem is defining functions at type construction, not function overload, hence we can't use SFINAE directly. Just using a auto and decltype() with a fake template parameter is not sufficient.

My solution uses std::enable_if and creates different parameter types, where a tag is bound to specific methods; if executor parameters inherits from chunk tag, chunk size methods will be exposed etc. I couldn't use parameter traits directly, because it will always provide a default behaviour if a specific function doesn't exist -> duplication. The small benefit of this solution is clear ordering and documenting different types of parameters.

I have found one solution which doesn't use separate tags: for every method a checker similar to traits is implemented - design is the same, but instead of running either the parameter function or default case, a constexpr boolean is exposed (true iff a method is present in type). But then we would have two huge code bases doing almost the same thing. Merging it requires rewriting whole traits, removing helpers and dispatching functions with an enable_if. I could implement this solution, but it seems that it would be a huge change in existing code base.

@hkaiser hkaiser added this to the 0.9.12 milestone Jun 7, 2016
///
/// \returns The new sequential_task_execution_policy
///
template <typename Parameters>
template <typename... Parameters, typename ParametersType =
typename hpx::parallel::executor_parameters_join<Parameters...>::type>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the rationale for making the ParametersType a template argument? Would it be sufficient to directly use the hpx::parallel::executor_parameters_join<Parameters...> in the body of this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course we can, it's just how I've been using it all the time. I got used to this solution, because this allows me to use it easily in specifying type before function body or in inheritance. Hence I do not need to declare this type twice, one for return type of function and then again in the function body.

Is it considered a bad practice, bad style? Should I change it? I thought that it's better to have it as a default parameter rather than duplicate code, just like it happens in rebound_type.


namespace detail
{
/// \cond NOINTERNAL
template <typename T>
template <typename T, typename Tag>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be changed to: template <typename T, typename Tag = executor_parameters_tag> ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the name is confusing, I changed it to have a general scheme for all types. I'm not sure, should I change the name to emphasize how we use detailed implementation?

Yes, we can change it, but it's not necessary - is_executor_parameters inherits from detailed version using both template types.

mcopik and others added 8 commits June 11, 2016 22:44
- simplify static asserts
- changed unwrapper to enable_if functions on separate has_foo<> traits
- removed executor parameter tags
- unified formatting
- fixing compilation problems of test
- adding serialization support
Optimizing case handling just one parameters object
@hkaiser hkaiser merged commit dd374bf into STEllAR-GROUP:master Jun 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants