-
Notifications
You must be signed in to change notification settings - Fork 1k
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
rfc: introducing threadpool support for Arm Compute Library #1293
Conversation
rfcs/20220303-Introducing-Threadpool-support-for-Arm-Compute-Library/README.md
Outdated
Show resolved
Hide resolved
rfcs/20220303-Introducing-Threadpool-support-for-Arm-Compute-Library/README.md
Show resolved
Hide resolved
rfcs/20220303-Introducing-Threadpool-support-for-Arm-Compute-Library/README.md
Outdated
Show resolved
Hide resolved
I thought that was the intent—setting the scheduler ensured that outcome—but “is set by” sounds like something is making it get set.
From: Crefeda Rodrigues ***@***.***>
Sent: Tuesday, March 8, 2022 8:18 AM
To: oneapi-src/oneDNN ***@***.***>
Cc: Dippold, Joel ***@***.***>; Comment ***@***.***>
Subject: Re: [oneapi-src/oneDNN] rfc: introducing threadpool support for Arm Compute Library (PR #1293)
@cfRod commented on this pull request.
________________________________
In rfcs/20220303-Introducing-Threadpool-support-for-Arm-Compute-Library/README.md<#1293 (comment)>:
+The `Scheduler::set()` method is used to instantiate the `ThreadpoolScheduler` object in oneDNN (in `src/cpu/aarch64/acl_utils.cpp`) via [`acl_set_custom_scheduler()`](https://github.com/cfRod/oneDNN/blob/caa172e5c86977edb398cbf16d26e443d51b2da4/src/cpu/aarch64/acl_utils.cpp#L123) as shown below:
+
+~~~c++
+void acl_set_custom_scheduler() {
+if DNNL_CPU_THREADING_RUNTIME == DNNL_RUNTIME_THREADPOOL
+ static std::once_flag flag_once;
+ // Create custom threadpool scheduler
+ std::shared_ptr<IScheduler> threadpool_scheduler
+ = std::make_unique<ThreadpoolScheduler>();
+ // Set custom scheduler in ACL
+ std::call_once(
+ flag_once, [&]() { arm_compute::Scheduler::set(threadpool_scheduler); });
+}
+~~~
+
+The creation of the ThreadpoolScheduler and setting is done during the primitive creation phase, i.e. at the [`init()` phase](https://github.com/cfRod/oneDNN/blob/caa172e5c86977edb398cbf16d26e443d51b2da4/src/cpu/aarch64/matmul/acl_matmul.hpp#L96). A single scheduler is set by ensuring that it is only called once and is used across different ACL-enabled primitives.
Hi,
I can sort out that sentence structure. I meant to say, the set scheduler is called once using std::call_once which would ensure that the scheduler is used across different ACL-enabled primitives. Hope that makes sense.
—
Reply to this email directly, view it on GitHub<#1293 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ANHT4PV6I25EOPDRVR754BTU654S3ANCNFSM5P3D2YBA>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you commented.Message ID: ***@***.******@***.***>>
|
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.
In general the RFC LGTM. This is very similar to what is done on x86 side.
I would recommend to move logic into a single place in aarch64 to avoid repeating the same few lines in each implementation. Also developers may forget to add all necessary calls in new implementations if they are not forced. So making this mechanism transparent to implementations will solve the issue.
rfcs/20220303-Introducing-Threadpool-support-for-Arm-Compute-Library/README.md
Show resolved
Hide resolved
rfcs/20220303-Introducing-Threadpool-support-for-Arm-Compute-Library/README.md
Outdated
Show resolved
Hide resolved
6c598a1
to
c3889e5
Compare
@jedippold and @igorsafo, thanks for your comments. I have updated the RFC incorporating your suggestions and my PoC-threadpool branch. If everything looks fine and this RFC is accepted, I will raise a separate PR based on the PoC-threadpool implementation linked. |
Hi @igorsafo and @jedippold , I have created a PR #1328 for this RFC. |
Thanks all! |
This RFC proposes to introduce threadpool runtime for Arm Compute Library.
Link to rendered document
Adding a diff link for the proof-of-concept implementation here.
RFC PR