Add system related parameters to performance related algorithms' parameters #2724

Vika-F · 2024-04-15T12:17:30Z

Following changed were added:

system_parameters class holding system-related performance parameters implemented.
all the performance-related hyperparameters classes like covariance::detail::compute_parameters are now derived from system_parameters class because in the future those system-related parameters might be chosen on per-algorithm basis.

…l perf-related parameters classes from system_parameters. 3. Add tests for system_parameters

Pull the changes from main branch

Vika-F · 2024-04-24T10:17:11Z

@keeranroth and @rakshithgb-fujitsu,
Can you please take a look on this PR?
It adds new API that deals with the enabled CPU instruction set and other system-related parameters in oneDAL.

keeranroth

Overall looks good. Just some cosmetic comments

keeranroth · 2024-04-24T11:00:25Z

cpp/oneapi/dal/detail/cpu_info_impl.cpp

@@ -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() };


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

keeranroth · 2024-04-24T11:10:15Z

cpp/oneapi/dal/detail/system_parameters.cpp

+system_parameters::system_parameters()
+        : impl_(detail::pimpl<system_parameters_impl>(new system_parameters_impl())) {}


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.

Suggested change

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>()) {}

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 as pimpl 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.

keeranroth · 2024-04-24T11:11:59Z

cpp/oneapi/dal/detail/system_parameters.hpp

+/// 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`


nit:

Suggested change

/// `cpu_info` reports the parameters available in hardware, when `system_parameters`

/// `cpu_info` reports the parameters available in hardware, where `system_parameters`

keeranroth · 2024-04-24T11:12:55Z

cpp/oneapi/dal/detail/system_parameters.hpp

+/// are the software-enabled parameters that can differ from `cpu_info`.
+class system_parameters : public base {
+public:
+    /// Creates a new default ``system_parameters`` instance.


Use of double backticks here, where single backticks are used in other places in this file. Keep the usage consistent

Suggested change

/// Creates a new default ``system_parameters`` instance.

/// Creates a new default `system_parameters` instance.

keeranroth · 2024-04-24T11:14:42Z

cpp/oneapi/dal/detail/system_parameters.hpp

+#endif
+
+private:
+    detail::pimpl<system_parameters_impl> impl_;


Can this be a unique pointer?

Suggested change

detail::pimpl<system_parameters_impl> impl_;

std::unique_ptr<system_parameters_impl> impl_;

keeranroth · 2024-04-24T11:16:31Z

cpp/oneapi/dal/detail/system_parameters_impl.cpp

+        ss << std::any_cast<std::uint32_t>(value);
+    }
+    else {
+        throw unimplemented{ dal::detail::error_messages::unsupported_data_type() };


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

keeranroth · 2024-04-24T11:41:48Z

cpp/oneapi/dal/detail/system_parameters_impl.cpp

+
+std::string system_parameters_impl::dump() const {
+    std::ostringstream ss;
+    for (auto it = sys_info_.begin(); it != sys_info_.end(); ++it) {


Suggested change

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_) {

C++17 variant was implemented here and in cpu_info

keeranroth · 2024-04-24T11:45:34Z

cpp/oneapi/dal/detail/system_parameters_impl.hpp

+    std::string dump(sycl::queue& queue) const;
+#endif
+
+protected:


nit: I don't think that this is ever used as a base class, so this should be private

Alexandr-Solovev

Overall looks good to me with previous comments

keeranroth

All looks good to me. Thanks for the updates

Alexandr-Solovev

Please fix conflicts and wait private CI checks

Vika-F · 2024-04-29T10:52:15Z

/intelci: run

Vika-F · 2024-04-30T11:02:03Z

/intelci: run

Vika-F · 2024-05-08T10:31:45Z

/intelci: run

Merge changes from main branch

Vika-F · 2024-05-10T17:00:51Z

/intelci: run

Vika-F · 2024-05-13T11:28:38Z

/intelci: run

Vika-F · 2024-05-15T10:58:17Z

Re-started CI: http://intel-ci.intel.com/ef12a9dd-06d4-f1a9-94c3-a4bf010d0e2e

Vika-F · 2024-05-16T11:24:47Z

/intelci: run

Vika-F · 2024-05-16T11:26:55Z

Re-started CI: http://intel-ci.intel.com/ef137714-0989-f1e2-b2cf-a4bf010d0e2e

Vika-F added 3 commits April 15, 2024 05:05

Initial implementation of system_parameters class

03ff287

Add a test for system-related parameters

a5ee87d

1. Fix system_parameters::get_top_enabled_cpu_extension. 2. Derive al…

2084a26

…l perf-related parameters classes from system_parameters. 3. Add tests for system_parameters

Vika-F added the enhancement label Apr 17, 2024

Vika-F added 5 commits April 17, 2024 03:59

clang-format; Add missing includes

474604b

Fix include name in cpu.hpp

c3776be

Return incorrectly deleted include back

51f2649

Fix build error in oneAPI examples

9a7eb9b

Add EOL in the end of the files

849beb6

Vika-F marked this pull request as ready for review April 24, 2024 09:30

Vika-F requested review from Alexsandruss, samir-nasibli and Alexandr-Solovev as code owners April 24, 2024 09:30

Vika-F requested review from avolkov-intel and removed request for samir-nasibli and Alexsandruss April 24, 2024 09:31

Vika-F added 2 commits April 24, 2024 03:04

Add comments to system_parameters class

c936722

Merge pull request #23 from oneapi-src/main

1911009

Pull the changes from main branch

keeranroth reviewed Apr 24, 2024

View reviewed changes

Alexandr-Solovev reviewed Apr 25, 2024

View reviewed changes

Implement the changes requested in code review

a2fa159

keeranroth approved these changes Apr 25, 2024

View reviewed changes

clang-format

18497e8

Alexandr-Solovev approved these changes Apr 29, 2024

View reviewed changes

Merge changes from main branch

20f5680

Fix DPC++ tests failures and Coverity failures

72ed679

Add system_parameters related symbols into onedal_parameters libs

81359a7

Fix bazel build

e38d51e

Merge pull request #24 from oneapi-src/main

63e5e3d

Merge changes from main branch

Fix parameters library build in makefile

511f814

Vika-F merged commit 5c3a22c into oneapi-src:main May 17, 2024
16 of 17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add system related parameters to performance related algorithms' parameters #2724

Add system related parameters to performance related algorithms' parameters #2724

Vika-F commented Apr 15, 2024 •

edited

Vika-F commented Apr 24, 2024

keeranroth left a comment

keeranroth Apr 24, 2024

keeranroth Apr 24, 2024

Vika-F Apr 25, 2024

keeranroth Apr 24, 2024

keeranroth Apr 24, 2024

keeranroth Apr 24, 2024

keeranroth Apr 24, 2024

keeranroth Apr 24, 2024

Vika-F Apr 25, 2024

keeranroth Apr 24, 2024

Alexandr-Solovev left a comment

keeranroth left a comment

Alexandr-Solovev left a comment

Vika-F commented Apr 29, 2024

Vika-F commented Apr 30, 2024

Vika-F commented May 8, 2024

Vika-F commented May 10, 2024

Vika-F commented May 13, 2024

Vika-F commented May 15, 2024

Vika-F commented May 16, 2024

Vika-F commented May 16, 2024

		system_parameters::system_parameters()
		: impl_(detail::pimpl<system_parameters_impl>(new system_parameters_impl())) {}

	/// `cpu_info` reports the parameters available in hardware, when `system_parameters`
	/// `cpu_info` reports the parameters available in hardware, where `system_parameters`

	/// Creates a new default ``system_parameters`` instance.
	/// Creates a new default `system_parameters` instance.

	detail::pimpl<system_parameters_impl> impl_;
	std::unique_ptr<system_parameters_impl> impl_;

	for (auto it = sys_info_.begin(); it != sys_info_.end(); ++it) {
	for (auto const& it : sys_info_) {

Add system related parameters to performance related algorithms' parameters #2724

Add system related parameters to performance related algorithms' parameters #2724

Conversation

Vika-F commented Apr 15, 2024 • edited

Vika-F commented Apr 24, 2024

keeranroth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alexandr-Solovev left a comment

Choose a reason for hiding this comment

keeranroth left a comment

Choose a reason for hiding this comment

Alexandr-Solovev left a comment

Choose a reason for hiding this comment

Vika-F commented Apr 29, 2024

Vika-F commented Apr 30, 2024

Vika-F commented May 8, 2024

Vika-F commented May 10, 2024

Vika-F commented May 13, 2024

Vika-F commented May 15, 2024

Vika-F commented May 16, 2024

Vika-F commented May 16, 2024

Vika-F commented Apr 15, 2024 •

edited