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 wait_for_tasks() #62

Merged
merged 4 commits into from
Apr 25, 2024
Merged

Add wait_for_tasks() #62

merged 4 commits into from
Apr 25, 2024

Conversation

DeveloperPaul123
Copy link
Owner

@DeveloperPaul123 DeveloperPaul123 commented Apr 24, 2024

This PR builds on top of #61 and adds a wait_for_tasks() functions.
Fixes #23

@GabTux
Copy link
Contributor

GabTux commented Apr 25, 2024

I've tested and reviewed the code, and it functions flawlessly. I compiled the test suite with both Clang and GCC, using thread and address sanitizers. All test runs produced zero errors.

The only thing I am concerned about is performance. Consider a scenario where all threads are awake, spinning in the inner loop (while (pending_tasks_.load(std::memory_order_acquire) > 0);). The queues are empty, but one thread is finishing a long-running task, so the pending_tasks_ remains non-zero. The threads would iterate through the queues to get a task and rotate their IDs in the priority queue.

I benchmarked the current master version and this pull request using the first two count primes benchmarks. The results were very similar. So I think it is acceptable because that case does not happen so often.

@DeveloperPaul123
Copy link
Owner Author

@GabTux Awesome thanks for testing things out.

Nice catch with the case of one long running task keeping the rest of the threads awake. Maybe we do need 2 counters after all; maybe with just different names.

DeveloperPaul123 added a commit that referenced this pull request Apr 25, 2024
I re-named the counters to names I feel make their use more clear (and distinct). This helps make clear (in my opinion) why there are 2 counters instead of just one as GabTux noted in the comment on #62
@DeveloperPaul123 DeveloperPaul123 changed the title Prevent premature thread exit and add wait_for_tasks() Add wait_for_tasks() Apr 25, 2024
@DeveloperPaul123 DeveloperPaul123 force-pushed the fix/pool-premature-exit branch from 704c137 to 81d8484 Compare April 25, 2024 14:02
Copy link

codecov bot commented Apr 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.24%. Comparing base (cee2fce) to head (8fe6151).
Report is 17 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
+ Coverage   98.07%   98.24%   +0.16%     
==========================================
  Files           2        2              
  Lines         104      114      +10     
==========================================
+ Hits          102      112      +10     
  Misses          2        2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DeveloperPaul123 DeveloperPaul123 merged commit 9e94e28 into master Apr 25, 2024
6 checks passed
@DeveloperPaul123 DeveloperPaul123 deleted the fix/pool-premature-exit branch April 25, 2024 14:23
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.

[FEATURE] Add wait_for_tasks() Feature
2 participants