-
Notifications
You must be signed in to change notification settings - Fork 115
[Dynamic Selection] Customization of Backends and Policies #2508
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
base: main
Are you sure you want to change the base?
Conversation
…t after, also removed submit from the sycl_backend. Now uses the base implementation
…t after, also removed submit from the sycl_backend. Now uses the base implementation
Signed-off-by: Dan Hoeflinger <[email protected]>
include/oneapi/dpl/internal/dynamic_selection_impl/policy_base.h
Outdated
Show resolved
Hide resolved
include/oneapi/dpl/internal/dynamic_selection_impl/policy_base.h
Outdated
Show resolved
Hide resolved
| { | ||
| s.report(execution_info::task_submission); | ||
| } | ||
| - Returns an object that can be used to wait for all active submissions. |
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 we need to add a topic on writing custom policies and another on writing custom backends. Maybe this information belongs somewhere one of those as a table. Its extraneous to actually using the policies so it may be confusing to retain in the individual policy topics.
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
|
|
||
| auto | ||
| get_submission_group() | ||
| auto_tune_policy(deferred_initialization_t) {} |
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.
Should we mark this constructor as explicit ?
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'm not sure what we benefit from this. I suppose we could, but basically we are preventing
auto_tune_policy p = oneapi::dpl::experimental::deferred_initialization;
Is there another advantage I'm missing here?
| if (index == use_best_resource) | ||
| { | ||
| return selection_type{*this, t->best_resource_, t}; | ||
| return std::make_shared<selection_type>(*this, t->best_resource_, t); |
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.
Should we call this make_shared under mutex?
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 you're right. I think for the sycl backend, where queues have reference semantics, it's a benign race on the value of best_resource_. But in general it could be an issue.
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 within a std::lock_guard<std::mutex> RAII scope. Is the question in the other direction for performance reasons (make_shared is too costly and might be able to outside the mutexed section)?
Otherwise, I'm not sure I understand the comment.
| { | ||
| auto r = state_->resources_with_index_[index]; | ||
| return selection_type{*this, r, t}; | ||
| return std::make_shared<selection_type>(*this, r, t); |
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.
The same
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.
see above
| static_assert(sizeof...(ReportReqs) == 0, "Default backend does not support reporting"); | ||
|
|
||
| for (const auto& e : u) | ||
| resources_.push_back(e); |
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.
Can we avoid this per-element copy?
For example, may be just move all u into resources_ ?
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 a reasonable question about dynamic selection in general. This pattern is how the code was originally written and accepted. Addressing these concerns of non-trivial resource types is something that may be worth doing, but I don't think we want to continue to expand the scope of this PR to do this across all of dynamic selection.
For now, to remain consistent with the way this works in dynamic selection, I'd suggest to keep it and possibly address this in a future standalone PR across all of this feature.
| } | ||
|
|
||
| auto | ||
| get_submission_group() |
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.
- Should this function be declared as
const? - This implementation means you copy
std::vectorinstance (so with memory allocation just forwait
Looks not quite correct...
Example:
s.get_submission_group().wait();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.
It only captures a reference to the resources, not a copy.
Also because it holds a non-const reference in the value returned, we shouldn't mark this const.
include/oneapi/dpl/internal/dynamic_selection_impl/default_backend.h
Outdated
Show resolved
Hide resolved
include/oneapi/dpl/internal/dynamic_selection_impl/default_backend.h
Outdated
Show resolved
Hide resolved
include/oneapi/dpl/internal/dynamic_selection_impl/default_backend.h
Outdated
Show resolved
Hide resolved
include/oneapi/dpl/internal/dynamic_selection_impl/default_backend.h
Outdated
Show resolved
Hide resolved
| using my_base = backend_base<ResourceType>; | ||
|
|
||
| template <typename... ReportReqs> | ||
| default_backend_impl(ReportReqs... reqs) : my_base(reqs...) |
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.
Should we use && and std::forward here?
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 don't think so. These are known to be empty tag structures indicating reporting requirements with no member fields. Forwarding doesn't save anything and just adds complexity.
|
|
||
| public: | ||
| template <typename... ReportReqs> | ||
| default_backend(ReportReqs... reqs) : base_t(reqs...) |
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.
the same
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.
see above
| using resource_type = typename backend_t::resource_type; | ||
| using wait_type = typename backend_t::wait_type; | ||
| using execution_resource_t = typename base_t::execution_resource_t; | ||
| using load_t = int; |
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.
Should we really have signed type here?
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 wouldn't really object to this, but I also dont think its really necessary.
We will never use that extra bit to represent load. If the load of something is ever greater than 2 billion, I'd be very surprised.
I dont think that using unsigned for values that cannot be negative is really good "documentation" either. It tends to be more likely to hide underflow bugs than fix them.
| get_submission_group() | ||
| dynamic_load_policy() { base_t::initialize(); } | ||
| dynamic_load_policy(deferred_initialization_t) {} | ||
| dynamic_load_policy(const std::vector<ResourceType>& u, ResourceAdapter adapter = {}) |
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.
What about to replace const std::vector<...>& -> std::vector<...>&& in class constructors and etc.
At this and in all other places like that.
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.
Similar to my response above, I think this is something we could address in a standalone PR rather than expanding the scope of this PR way beyond customization concerns.
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Dynamic Selection API: Backend and Policy Customization & Removal of Selection API
This PR refactors the Dynamic Selection API to introduce a flexible backend architecture and simplify the user-facing API by removing the
select()function. It provides better tools for customization of backend and policies to allow for easier customization and more flexibility for users.Implements RFCs #2220 and #2489 (without token policy, that will be a separate PR).
Key Changes
Backend Architecture
policy_base.handdefault_backend.hto provide common base classes for policies and backends respectivelyResourceAdapterfunction to support different flavors of resource with the same backend (e.g.sycl::queuevssycl::queue*)API Simplification
select()function - selections are now internal implementation detailstry_submit- always returns astd::optionalquickly, returns empty if unable to obtain a resourcetry_submit,submit()andsubmit_and_wait()functionstry_select_impl()(returnsstd::optional) instead of publicselect()Execution Info & Reporting
Summary of changes to look out for from individual components: