Add option to specify stack size to pthread_create #2073
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Working reduced stack sizes. Adjusted default stack size to 64kb, moved Offer/Answer messages to be on heap instead of stack * Custom stack sizes in Common.c * CMake flag working and added to README * Adjust connection listener stack size for datachannels * Change stack size for media related threads and connection listener again. Add README notes to highlight this potential problem threads for other applications * Unnecessary memset * nit picks * Update CMakeLists.txt fix for ZLIB since it is not found on Windows, that way we can verify the stack side change on Windows platform, previously it was not running at all due to build fail. * Update CMake dependency back to develop since the other PRs have merged * README update --------- Co-authored-by: Hassan Sahibzada <[email protected]> Co-authored-by: Divya Sampath Kumar <[email protected]>
stefankiesz
reviewed
Nov 8, 2024
| @@ -52,6 +52,15 @@ execute_process( | |||
| add_definitions(-DSDK_VERSION=\"${GIT_COMMIT_HASH}\") | |||
| add_definitions(-DVERSION_STRING=\"${PROJECT_VERSION}\") | |||
| add_definitions(-DDETECTED_GIT_HASH) | |||
| if(NOT KVS_STACK_SIZE OR KVS_STACK_SIZE STREQUAL "") | |||
There was a problem hiding this comment.
Rather than checking for an empty string, can we validate that it is a numeric value with something like:
if(KVS_STACK_SIZE MATCHES "^[+-]?[0-9]+$")
There was a problem hiding this comment.
I'm thinking of removing this section actually. Because when I ran Producer-C with the thread stacksize changes, I was able to pass it in via producer-c's CMake command:
Producer-C/build $ cmake .. -DBUILD_DEPENDENCIES=OFF -DKVS_DEFAULT_STACK_SIZE= 524288 -DCONSTRAINED_DEVICE=ON
...
CMake Error at dependency/libkvspic/kvspic-src/CMakeLists.txt:62 (message):
Conflicting parameters: KVS_DEFAULT_STACK_SIZE and CONSTRAINED_DEVICE
cannot both be set.
-- Configuring incomplete, errors occurred!
Which would indicate this section is redundant.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was changed?
KVS_STACK_SIZEto specify the thread stack size of threads. By default, THREAD_CREATE (defined in PIC, which calls pthread_create), will use the system default settings (ulimit -s). These changes allow the user to set the stack sizes for threads created by the SDK without needing to change system defaults.THREAD_CREATE_WITH_PARAMSAPI instead ofTHREAD_CREATE. (Added in PIC#243)Why was it changed?
How was it changed?
KVS_STACK_SIZEoption will get passed to kvsCommonLws (Producer-C), who forwards it to PIC, which contains the thread creation definitions.What testing was done for the changes?
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.