Skip to content
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

Custom Thread StackSize API #264

Merged
merged 6 commits into from
Nov 7, 2024
Merged

Custom Thread StackSize API #264

merged 6 commits into from
Nov 7, 2024

Conversation

sirknightj
Copy link
Contributor

@sirknightj sirknightj commented Nov 5, 2024

Issue #, if available:

  • N/A

What was changed?

  • See Add API for specifying thread stack size #243 for the original PR with details.
  • Add new CMake flag (KVS_DEFAULT_STACK_SIZE) that allows users to configure the default stack size of threads created with THREAD_CREATE.
  • Add new CMake flag (THREAD_CREATE_WITH_PARAMS) that allows users to create threads with custom stack sizes.
  • For non-windows, there was a CMake flag available: -DCONSTRAINED_DEVICE that sets the default stack size to 0.5 MiB. For backwards compatibility, this flag was not changed.
  • Make KVS_DEFAULT_STACK_SIZE and CONSTRAINED_DEVICE incompatible with each other. The build will fatally exit if both of these are set.
  • Added more unit tests for the new API, including a unit test that checks that validates the stack size was actually changed.
  • Add a script to run various CMake flag combinations to check that that it correctly fails the build if those 2 exclusive CMake flags are set. Generate a markdown test report for that as well.

Why was it changed?

How was it changed?

What testing was done for the changes?

image

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

jdelapla and others added 4 commits November 4, 2024 14:59
* Add API for specifying thread stack size

* Add compile time default stack size

* compile time definitions in cmake

* add local variable for stack size in API

* Adding temporary CMake Debug message

* Removed debug message in CMakeLists.txt, added CMAKE flag to readme

* Reset global variable before running new thread test

* Remove duplicate code and unused variables

* explicit cast

* missing )

* enforce pthread min stack size

* Change name of variable to have *_BYTES for readability

* Update variable name to include 'bytes' by request

* Addressing nit picks

* bound rand stack size value to not exceed max

* Remove rand() test on an OS wrapper API

* Wake up github

* Comment

* Remove lower bound checking for pthread

* Clang
…or the thread create API; Add new CI job and script to check CMake flag compatibility
tst/utils/Thread.cpp Outdated Show resolved Hide resolved
@sirknightj sirknightj merged commit 007f19c into develop Nov 7, 2024
26 checks passed
@sirknightj sirknightj deleted the thread-stacksize-api branch November 7, 2024 19:21
Copy link
Contributor

@MushMal MushMal left a comment

Choose a reason for hiding this comment

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

This is a good change. My only concern is the name of the new API and the choice of the parameter itself. What if in the future you might want to add another parameter? I would suggest adding a struct that can be versioned and new params could be added later for back compat. The struct initially can have only a couple of fields - like the version, stack size, thread name. But later can include more flags such as start suspended, priority, etc..

@unicornss
Copy link

unicornss commented Nov 7, 2024

Something like this ...

#define THREAD_PARAMS_VERSION_1 1
typedef struct ThreadParams {
    int version;
    SIZE_T stackSize;
    :
} ThreadParams;

sirknightj added a commit that referenced this pull request Nov 18, 2024
* CI enhancements, linter job rework

* PR description linter allowing * characters in the markdown format

* Apply linting

* Fix unit test when log level env variable is not set

* Use master branch, not main branch

* Prettify test configuration

* Correct missing defined in include file (#265)

* Custom Thread StackSize API (#264)

* Add API for specifying thread stack size (#243)

* Add API for specifying thread stack size

* Add compile time default stack size

* compile time definitions in cmake

* add local variable for stack size in API

* Adding temporary CMake Debug message

* Removed debug message in CMakeLists.txt, added CMAKE flag to readme

* Reset global variable before running new thread test

* Remove duplicate code and unused variables

* explicit cast

* missing )

* enforce pthread min stack size

* Change name of variable to have *_BYTES for readability

* Update variable name to include 'bytes' by request

* Addressing nit picks

* bound rand stack size value to not exceed max

* Remove rand() test on an OS wrapper API

* Wake up github

* Comment

* Remove lower bound checking for pthread

* Clang

* StackSize and ConstrainedDevice incompatible; Additional unit tests for the thread create API; Add new CI job and script to check CMake flag compatibility

* Add second case

* Fix typo

* Rename variables in the test

* Address comments and add log line in case of conflict again

---------

Co-authored-by: jdelapla <[email protected]>

* Change createThreadWithParams API from size_t to versioned struct (#268)

* Change createThreadWithParams from size_t to versioned struct and add new negative test scenarios

* Remove designated initializers

* Fix capitalization

* Add message to clarify that stack size is not set

* Adjust status name and strengthen equality checks in the test

* Adjust name in the windows path as well

* Move the statuscode to the other location, adjust tooLargeThreadStack test for windows and linux

* Fix missing parenthesis

* Move the sleep into the guarded section

* Rename variable in the test

---------

Co-authored-by: Rob Baily <[email protected]>
Co-authored-by: jdelapla <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants