-
Notifications
You must be signed in to change notification settings - Fork 211
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
Add system related parameters to performance related algorithms' parameters #2724
Conversation
…l perf-related parameters classes from system_parameters. 3. Add tests for system_parameters
Pull the changes from main branch
@keeranroth and @rakshithgb-fujitsu, |
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.
Overall looks good. Just some cosmetic comments
@@ -80,6 +81,9 @@ void cpu_info_impl::print_any(const std::any& value, std::stringstream& ss) cons | |||
else if (ti == typeid(cpu_vendor)) { | |||
print<cpu_vendor>(value, ss); | |||
} | |||
else { | |||
throw unimplemented{ dal::detail::error_messages::unsupported_data_type() }; |
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 you pass the typeid of ti
so that this message is a little more specific? It would be nice to know what the unknown type is
system_parameters::system_parameters() | ||
: impl_(detail::pimpl<system_parameters_impl>(new system_parameters_impl())) {} |
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.
Avoid the use of bare new. Use std::make_unique here. pimpl
is a shared pointer, but going from unique to shared pointer is easy. It should probably be looked at in the future, whether impl_
should be a unique pointer in here. It is a private field and the constructor creates the object, so it seems like ownership is clear here.
This might be opening a can of worms, but the ownership of objects is a little lax in the rest of the code. I'd like to see more unique pointers, especially in new code. Maybe that's just me, though.
system_parameters::system_parameters() | |
: impl_(detail::pimpl<system_parameters_impl>(new system_parameters_impl())) {} | |
system_parameters::system_parameters() | |
: impl_(std::make_unique<system_parameters_impl>()) {} |
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.
Thank you for this valid observation. I have replaced new
with make_unique
.
Regarding making impl_
a unique pointer.
I'd prefer not to do it in the scope of this PR because:
- it would lead to the inconsistency with the rest of oneDAL's code
- modifying
detail::pimpl
's definition will require extensive testing, including performance testing aspimpl
is used widely in the code. For me it looks like a separate task with unknown outcome.
I've forwarded your observations to my team. We'll think on how to prioritize this.
/// Stores system-related parameters that affect the performance of the algorithms. | ||
/// Those parameters can differ from the `get_global_context().get_cpu_info()`. | ||
/// | ||
/// `cpu_info` reports the parameters available in hardware, when `system_parameters` |
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.
nit:
/// `cpu_info` reports the parameters available in hardware, when `system_parameters` | |
/// `cpu_info` reports the parameters available in hardware, where `system_parameters` |
/// are the software-enabled parameters that can differ from `cpu_info`. | ||
class system_parameters : public base { | ||
public: | ||
/// Creates a new default ``system_parameters`` instance. |
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.
Use of double backticks here, where single backticks are used in other places in this file. Keep the usage consistent
/// Creates a new default ``system_parameters`` instance. | |
/// Creates a new default `system_parameters` instance. |
#endif | ||
|
||
private: | ||
detail::pimpl<system_parameters_impl> impl_; |
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 this be a unique pointer?
detail::pimpl<system_parameters_impl> impl_; | |
std::unique_ptr<system_parameters_impl> impl_; |
ss << std::any_cast<std::uint32_t>(value); | ||
} | ||
else { | ||
throw unimplemented{ dal::detail::error_messages::unsupported_data_type() }; |
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 information about ti
be printed in the exception as well?
Update: Looking at the code, this is not a simple ask, so ignore me. I've raised issue #2742
|
||
std::string system_parameters_impl::dump() const { | ||
std::ostringstream ss; | ||
for (auto it = sys_info_.begin(); it != sys_info_.end(); ++it) { |
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.
for (auto it = sys_info_.begin(); it != sys_info_.end(); ++it) { | |
for (auto const& it : sys_info_) { |
or if we can assume C++17:
for (auto const &[name, val] : sys_info_) {
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.
C++17 variant was implemented here and in cpu_info
std::string dump(sycl::queue& queue) const; | ||
#endif | ||
|
||
protected: |
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.
nit: I don't think that this is ever used as a base class, so this should be private
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.
Overall looks good to me with previous comments
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.
All looks good to me. Thanks for the updates
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.
Please fix conflicts and wait private CI checks
/intelci: run |
/intelci: run |
/intelci: run |
Merge changes from main branch
/intelci: run |
/intelci: run |
/intelci: run |
Following changed were added:
system_parameters
class holding system-related performance parameters implemented.covariance::detail::compute_parameters
are now derived fromsystem_parameters
class because in the future those system-related parameters might be chosen on per-algorithm basis.