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

Add task destroying to task processing. #129

Closed
wants to merge 1 commit into from

Conversation

risa2000
Copy link

@risa2000 risa2000 commented Nov 23, 2023

Fixes a single issue when the task queued to the pool contains a capture of shared ref. In the original code the processed task was held indefinitely, until the worker thread exited or processed another task, which was the only way to release the shared ref. (Would probably be the same for a functor.)

Style

Have you formatted your code using the .clang-format file attached to this project? Yes.

Testing

Have you tested the new code using the provided automated test program BS_thread_pool_test.cpp (preferably with the provided multi-compiler test script BS_thread_pool_test.ps1) and/or performed any other tests to ensure that the new code works correctly? Used BS_thread_pool_test.ps1 with clang++ and msvc.

If so, please provide information about the test system(s):

  • CPU model, architecture, # of cores and threads: i9-9900K (8 cores/16 threads)
  • Operating system: Windows 10 Pro
  • Name and version of C++ compiler:
clang version 18.0.0 (https://github.com/llvm/llvm-project.git d65feccb126226e6f7805a01d759ca1c9ce28237)
Microsoft (R) C/C++ Optimizing Compiler Version 19.38.33130 for x64
  • Full command used for compiling, including all compiler flags: The one in the script.

Fix for hanging ref when the task includes capture of shared ref, which
has to be released after the task is processed.
@bshoshany
Copy link
Owner

Hi @risa2000 and thanks for the PR! This was already suggested in #124, and has been implemented in the upcoming v4.0.0 release. The new release should be out within the next few weeks.

@bshoshany bshoshany closed this Nov 23, 2023
@risa2000
Copy link
Author

risa2000 commented Nov 23, 2023

@bshoshany Hi there!
I checked #124 and you may notice that there is a slight difference in how the task is destroyed. I have included it into "task execution" interval by explicitly calling destructor inside the section where the running task counter is incremented (and the global task lock is unlocked). I believe this is a cleaner solution.
My first approach was the same as in #124 but this way the task destruction happens at the end of the while loop, out of the execution tracking and potentially keeping the lock unnecessarily.

@bshoshany
Copy link
Owner

I see. I think an even cleaner solution is to have task created inside its own code block, so it's destructed automatically at the end of the block. In v4.0.0 the worker function has actually been substantially rewritten, to allow for stuff like initialization functions and task priority, so I'll look into the best way to do this with the new worker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants