Round robin updates #1056
Round robin updates #1056
Conversation
There was a problem hiding this comment.
The old file - round_robin_policy_impl still exists and should be removed.
This file name should be mentioned in include/oneapi/dpl/internal/dynamic_selection.h
Maybe changes need to be made the the tests too
|
|
||
| template<typename ...Args> | ||
| selection_t select(Args&&...) { | ||
| size_t i=unit_->offset_; |
There was a problem hiding this comment.
The tests will have to change with the introduction of offset, or we would need some more tests
There was a problem hiding this comment.
Yes, we'll need more tests for offset and deferred. However, since they have defaults, the current tests should be fine.
| unit_->deferred_ = deferred; | ||
| } | ||
|
|
||
| round_robin_policy(resource_container_t u, int offset=0, bool deferred=false) : backend_{std::make_shared<Backend>()}, unit_{std::make_shared<unit_t>()} { |
There was a problem hiding this comment.
If deferred=true,the backend should not be initialized
There was a problem hiding this comment.
maybe check if (deferred==true) backend_->initialize();
There was a problem hiding this comment.
The thinking was to have the backend decide on what to do and not the policy making the decision.
| return backend_->get_resources(); | ||
| } | ||
|
|
||
| void initialize(int offset=0, bool deferred=false) { |
There was a problem hiding this comment.
Does initialize need deferred?
There was a problem hiding this comment.
Calling initialize to set the offset could cause the backend to return the default resources or not. deferred here will let the backend know what to do.
There was a problem hiding this comment.
I don't think it makes sense to call initialize with deferred. It is a choice to silently ignored initialization if not deferred, but definitely should not reinitialize. So need to check if already initialized.
|
|
||
| std::shared_ptr<unit_t> unit_; | ||
| public: | ||
| round_robin_policy(int offset=0, bool deferred=false) : backend_{std::make_shared<Backend>()}, unit_{std::make_shared<unit_t>()} { |
There was a problem hiding this comment.
Should be a pre-defined deferred value for the offset, not an additional argument. And if deferred nothing should be initialized in policy. Since backends do require an initialize function, it should be ok to assume they can also be constructed as deferred. So it would be ok to create backend in deferred state.
| private: | ||
| std::shared_ptr<Backend> backend_; | ||
|
|
||
| struct unit_t{ |
There was a problem hiding this comment.
Let's use state_t and state_ instead of unit_t and unit_.
| resource_container_size_t num_contexts_; | ||
| std::atomic<resource_container_size_t> next_context_; | ||
| int offset_; | ||
| bool deferred_; |
There was a problem hiding this comment.
probably can be removed if offset_ == deferred means the same thing.
| return backend_->get_resources(); | ||
| } | ||
|
|
||
| void initialize(int offset=0, bool deferred=false) { |
There was a problem hiding this comment.
Only a single argument that is offset.
| return backend_->get_resources(); | ||
| } | ||
|
|
||
| void initialize(int offset=0, bool deferred=false) { |
There was a problem hiding this comment.
I don't think it makes sense to call initialize with deferred. It is a choice to silently ignored initialization if not deferred, but definitely should not reinitialize. So need to check if already initialized.
|
|
||
| void initialize(int offset=0, bool deferred=false) { | ||
| unit_->offset_ = offset; | ||
| unit_->deferred_ = deferred; |
There was a problem hiding this comment.
If you want to use this to track state of initialization, maybe just call it initialized instead. So, if deferred, it starts out false.
This is to ensure that the round robin changes are on the right track.
Notable updates are the new names (without expanding out the _type yet), use of a boolean passed to the backend for deferred initialization.