Setting parallel to tbb static partitioner or tbb auto partitoner by thread pool

[sunxiaoxia2022]

Details:

• Setting parallel to TBB STATIC partitioner or TBB AUTO partitioner through by thread pool
• oneDNN PR: [https://github.com/[FORK][FIX] Fix performance issues caused by using the thread pool oneDNN#291]

Tickets:

• CVS-165229

[sunxiaoxia2022]

Could you please create a oneDNN fork PR to make the review more convenient?

@maxnick Yes, created oneDNN PR291

[maxnick]

With this design you decoupled ThreadPool and CpuParallel, but in fact the ThreadPool should be a wrapper over CpuParallel object. I can only guess that this has been done to avoid circular dependency of storing CpuParallel as a shared pointer in ThreadPool . But CpuParallel can be stored as a reference or week_ptr, the latter is less preferable due to performance implications though.
Could you please store CpuParallel as a const reference inside ThreadPool and redirect all the calls to this object?

class ThreadPool : public dnnl::threadpool_interop::threadpool_iface {
public:
    ThreadPool() = delete;
    ThreadPool(ThreadPool&) = delete;
    ThreadPool& operator(ThreadPool&) = delete;
    
    ThreadPool(ThreadPool&&) = delete;
    ThreadPool& operator(ThreadPool&&) = delete;
    
    explicit ThreadPool(const CpuParallel& cpu_parallel) : m_cpu_parallel(cpu_parallel) {}
    
    [[nodiscard]] int get_num_threads() const override {
        return m_cpu_parallel.get_num_threads();
    }

    [[nodiscard]] bool get_in_parallel() const override {
        return false;
    }

    [[nodiscard]] uint64_t get_flags() const override {
        return 0;
    }

    void parallel_for(int n, const std::function<void(int, int)>& fn) override {
        m_cpu_parallel.parallel_simple(n, fn);
    }

private:
    const CpuParallel& cpu_parallel;
}

Also, just not to prolong the lifespan of the ThreadPool object more than the CpuParallel object, it does make sense to return a raw pointer instead of shared_ptr. What do you think?

[sunxiaoxia2022]

Yes, decoupling ThreadPool and CpuParalle is to avoid circular dependency. If store CpuParallel as a reference in ThreadPool, this problem is still exists. In order to solve this issue, I abstracted a ICpuParallel which has the methods ThreadPool needed. Please take a look.

[maxnick]

Could you please elaborate on why do we need ICpuParallel here? Why can't we simply store const CpuParallel&?
Suspect the reason might be in some dependency of the includes. But it looks like it may be resolved using forward declaration.
Also the virtual methods of the dnnl::threadpool_interop::threadpool_iface can be defined in the .cpp file as those are virtual functions anyway and aren't expected to be inline.

[sunxiaoxia2022]

Because there was a compilation problem before, it has been fixed now. ICpuParallel is no longer needed, it was removed now.

[maxnick]

Could you please elaborate on the reason behind removing the const qualifier?

[sunxiaoxia2022]

Added const again.