Skip to content

Conversation

sunxiaoxia2022
Copy link
Contributor

@sunxiaoxia2022 sunxiaoxia2022 commented Apr 16, 2025

Details:

Tickets:

@sunxiaoxia2022 sunxiaoxia2022 requested review from a team as code owners April 16, 2025 03:43
@github-actions github-actions bot added category: Core OpenVINO Core (aka ngraph) category: CPU OpenVINO CPU plugin category: build OpenVINO cmake script / infra category: CPP API OpenVINO CPP API bindings labels Apr 16, 2025
@mlukasze mlukasze requested a review from ilya-lavrenov April 17, 2025 07:04
@maxnick maxnick self-assigned this Apr 22, 2025
@github-actions github-actions bot removed category: Core OpenVINO Core (aka ngraph) category: CPP API OpenVINO CPP API bindings labels Apr 30, 2025
Comment on lines 16 to 19
constexpr int default_multiplier = 32;
class ThreadPool;

class CpuParallel {
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT. Could you please move it inside the class as a static field?

class CpuParallel {
public:
    static constexpr int default_multiplier = 32;

public:
   CpuParallel() = delete;
    ....
 };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok moved it inside CpuParallel class.

Comment on lines +69 to +71
template <typename T0, typename R, typename F>
[[nodiscard]] R cpu_parallel_sum(const T0& D0, const R& input, const F& func) const {
#if OV_THREAD == OV_THREAD_TBB_ADAPTIVE
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move all the cpu_parallel_... methods into the private section? Because with this design, the cpu_parallel_... methods and the parallel_... methods of the same class may be used interchangeably, which would introduce a mess into the code base. Better to avoid such a situation and use the uniform name convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right, moved these functions into private section.

// cpu stream executor for current graph
ov::threading::CPUStreamsExecutor::Ptr m_cpuStreamExecutor;
std::shared_ptr<CpuParallel> m_cpuParallel = nullptr;
// std::shared_ptr<ThreadPool> m_threadPool = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Commented code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@maxnick maxnick left a comment

Choose a reason for hiding this comment

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

LGTM! Excellent work!

@maxnick
Copy link
Contributor

maxnick commented Oct 15, 2025

@sunxiaoxia2022 , could you please fix the CI errors?

@maxnick maxnick changed the title Setting parallel to tbb static partitioner or tbb auto partitoner by thread pool [CPU] Setting parallel to tbb static partitioner or tbb auto partitoner by thread pool Oct 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: build OpenVINO cmake script / infra category: Core OpenVINO Core (aka ngraph) category: CPP API OpenVINO CPP API bindings category: CPU OpenVINO CPU plugin category: inference OpenVINO Runtime library - Inference category: Python API OpenVINO Python bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants