-
Notifications
You must be signed in to change notification settings - Fork 51
feat: Implemented GlobalThreadPool for async functions #603
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
Conversation
ca24036 to
18ea813
Compare
packages/react-native-executorch/common/rnexecutorch/threads/GlobalThreadPool.h
Outdated
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/GlobalThreadPool.h
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/HighPerformanceThreadPool.h
Outdated
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/HighPerformanceThreadPool.h
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/HighPerformanceThreadPool.h
Outdated
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/HighPerformanceThreadPool.h
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/HighPerformanceThreadPool.h
Outdated
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/HighPerformanceThreadPool.h
Outdated
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/HighPerformanceThreadPool.h
Outdated
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/ThreadUtils.h
Outdated
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/HighPerformanceThreadPool.h
Outdated
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/HighPerformanceThreadPool.h
Outdated
Show resolved
Hide resolved
| WorkItem item; | ||
| item.task = std::move(task); | ||
| item.priority = priority; | ||
| item.enqueueTime = std::chrono::steady_clock::now(); |
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.
This is completely up to you, but I would probably define constructor for this struct, it's not classic POD because you have a non-copyable std::unique_ptr here. What are the benefits? More concise initialization, priority and enqueueTime might be marked private.
packages/react-native-executorch/common/rnexecutorch/RnExecutorchInstaller.cpp
Outdated
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/GlobalThreadPool.h
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/HighPerformanceThreadPool.h
Outdated
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/HighPerformanceThreadPool.h
Outdated
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/RnExecutorchInstaller.cpp
Outdated
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/ThreadUtils.h
Outdated
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/GlobalThreadPool.h
Outdated
Show resolved
Hide resolved
| namespace rnexecutorch { | ||
| namespace threads { | ||
|
|
||
| class GlobalThreadPool { |
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.
Consider removing this class and leaving all functions inside the threads::global_thread_pool namespace
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.
HighPerformanceThreadPool should be probably implemented similarly to this one:
class Singleton {
public:
Singleton(const Singleton&) = delete;
Singleton& operator=(const Singleton&) = delete;
Singleton(Singleton&&) = delete;
Singleton& operator=(Singleton&&) = delete;
static Singleton& getInstance() {
static std::once_flag initFlag;
if (!instance) {
throw std::logic_error("Instance not initialized.");
}
return *instance;
}
static void initialize(int arg) {
static std::once_flag initFlag;
std::call_once(initFlag, [arg]() {
instance = std::make_unique<Singleton>(arg);
});
}
private:
int value;
Singleton(int val) : value(val) {}
static std::unique_ptr<Singleton> instance;
};
std::unique_ptr<Singleton> Singleton::instance = nullptr;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.
I think you meant GlobalThreadPool, right? HighPerformanceThreadPool was never supposed to be a singleton
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.
Currently, HighPerformanceThreadPool is used only in the context of GlobalThreadPool. I guess Jakub meant that GlobalThreadPool is only a wrapper on a class that might be a singleton itself. If in the future there might be any other way to use HighPerformanceThreadPool the current design will make more sense to me then.
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.
That was my idea when creating this, HighPerformanceThreadPool is not meant to be a singleton, but GlobalThreadPool is. Also semantically it makes more sense to me to keep GlobalThreadPool as a class instead of a namespace even though it doesn't have any nonstatic members.
|
Add conditional closing of this issue: #565 in the PR |
packages/react-native-executorch/react-native-executorch.podspec
Outdated
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/HighPerformanceThreadPool.h
Outdated
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/HighPerformanceThreadPool.h
Outdated
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/HighPerformanceThreadPool.h
Outdated
Show resolved
Hide resolved
msluszniak
left a comment
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.
Very small NITs
packages/react-native-executorch/common/rnexecutorch/threads/HighPerformanceThreadPool.h
Outdated
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/HighPerformanceThreadPool.h
Outdated
Show resolved
Hide resolved
packages/react-native-executorch/common/rnexecutorch/threads/HighPerformanceThreadPool.h
Outdated
Show resolved
Hide resolved
msluszniak
left a comment
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.
🚀
|
|
||
| void setCPUAffinity() { | ||
| // AFAIK it is not possible on iOS | ||
| #ifdef __ANDROID__ |
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.
You should wrap the whole function implementation with #ifdef __ANDROID__ to not introduce overhead of translating empty function body on iOS. Check other applicable places
| void setThreadPriority() { | ||
| // pthread_setschedparam doesn't work on android because permissions reasons | ||
| // and in general does not provide visible improvements on iOS | ||
|
|
||
| // Set nice value as fallback or additional priority boost | ||
| constexpr int nice_value = 0; | ||
| if (setpriority(PRIO_PROCESS, 0, nice_value) != 0) { | ||
| log(LOG_LEVEL::Debug, "Failed to set nice value"); | ||
| } else { | ||
| log(LOG_LEVEL::Debug, "Set nice value", nice_value); | ||
| } | ||
| } |
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.
Some functions like here or getCurrentThreadId might be marked static or moved outside of the class
chmjkb
left a comment
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.
besides the single comment lgtm
| '"$(PODS_TARGET_SRCROOT)/third-party/executorch/backends/xnnpack/third-party/pthreadpool/include" '+ | ||
| '"$(PODS_TARGET_SRCROOT)/third-party/executorch/backends/xnnpack/third-party/cpuinfo/include" ', |
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.
Shouldn't this be third-party/include/executorch/...? If this is the case then it is already included in the line above
159141f to
6912d47
Compare
6912d47 to
5d4e588
Compare
## Description Added singleton class GlobalThreadPool for single threadpool management so that we don't have to spawn new threads for each async function and instead we can delegate functions to the threadpool. Also added pthreadpool and cpuinfo binaries for iOS to allow for XNNPack threadpool configuration just like on Android ### Introduces a breaking change? - [ ] Yes - [x] No ### Type of change - [ ] Bug fix (change which fixes an issue) - [x] New feature (change which adds functionality) - [ ] Documentation update (improves or adds clarity to existing documentation) - [ ] Other (chores, tests, code style improvements etc.) ### Tested on - [x] iOS - [x] Android ### Testing instructions <!-- Provide step-by-step instructions on how to test your changes. Include setup details if necessary. --> ### Screenshots <!-- Add screenshots here, if applicable --> ### Related issues <!-- Link related issues here using #issue-number --> ### Checklist - [ ] I have performed a self-review of my code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have updated the documentation accordingly - [ ] My changes generate no new warnings ### Additional notes <!-- Include any additional information, assumptions, or context that reviewers might need to understand this PR. --> --------- Co-authored-by: Mateusz Kopciński <mateusz.kopcinski@swmansnion.com>
Added singleton class GlobalThreadPool for single threadpool management so that we don't have to spawn new threads for each async function and instead we can delegate functions to the threadpool. Also added pthreadpool and cpuinfo binaries for iOS to allow for XNNPack threadpool configuration just like on Android - [ ] Yes - [x] No - [ ] Bug fix (change which fixes an issue) - [x] New feature (change which adds functionality) - [ ] Documentation update (improves or adds clarity to existing documentation) - [ ] Other (chores, tests, code style improvements etc.) - [x] iOS - [x] Android <!-- Provide step-by-step instructions on how to test your changes. Include setup details if necessary. --> <!-- Add screenshots here, if applicable --> <!-- Link related issues here using #issue-number --> - [ ] I have performed a self-review of my code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have updated the documentation accordingly - [ ] My changes generate no new warnings <!-- Include any additional information, assumptions, or context that reviewers might need to understand this PR. --> --------- Co-authored-by: Mateusz Kopciński <mateusz.kopcinski@swmansnion.com>
…ion#603) ## Description Added singleton class GlobalThreadPool for single threadpool management so that we don't have to spawn new threads for each async function and instead we can delegate functions to the threadpool. Also added pthreadpool and cpuinfo binaries for iOS to allow for XNNPack threadpool configuration just like on Android ### Introduces a breaking change? - [ ] Yes - [x] No ### Type of change - [ ] Bug fix (change which fixes an issue) - [x] New feature (change which adds functionality) - [ ] Documentation update (improves or adds clarity to existing documentation) - [ ] Other (chores, tests, code style improvements etc.) ### Tested on - [x] iOS - [x] Android ### Testing instructions <!-- Provide step-by-step instructions on how to test your changes. Include setup details if necessary. --> ### Screenshots <!-- Add screenshots here, if applicable --> ### Related issues <!-- Link related issues here using #issue-number --> ### Checklist - [ ] I have performed a self-review of my code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have updated the documentation accordingly - [ ] My changes generate no new warnings ### Additional notes <!-- Include any additional information, assumptions, or context that reviewers might need to understand this PR. --> --------- Co-authored-by: Mateusz Kopciński <mateusz.kopcinski@swmansnion.com>
Description
Added singleton class GlobalThreadPool for single threadpool management so that we don't have to spawn new threads for each async function and instead we can delegate functions to the threadpool.
Also added pthreadpool and cpuinfo binaries for iOS to allow for XNNPack threadpool configuration just like on Android
Introduces a breaking change?
Type of change
Tested on
Testing instructions
Screenshots
Related issues
Checklist
Additional notes