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

Motivated by "The load balancing issue in Poco::ActiveThreadPool" #4544 #4548

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bas524
Copy link
Contributor

@bas524 bas524 commented May 7, 2024

Optimization allows redistribute tasks to the idle threads
When ActiveThread dequeue next Runnable, it checks is optimization is true and if yes it try to find idle thread and resend task
By default optimization is false

…project#4544"

Optimization allows redistribute tasks to the idle threads
@matejk
Copy link
Contributor

matejk commented May 7, 2024

Thanks for the contribution.

IMO partial refactoring of ActiveThreadPool (and related classes) to have a single task queue in the pool where the threads take the work from would automatically create optimal load-balancing and simplify the implementation at the same time.

@bas524
Copy link
Contributor Author

bas524 commented May 7, 2024

Thanks for the contribution.

IMO partial refactoring of ActiveThreadPool (and related classes) to have a single task queue in the pool where the threads take the work from would automatically create optimal load-balancing and simplify the implementation at the same time.

Hi!
I thougt about it. If notificationqueue was lock-free than you proposition shoud be correct. In our case realisation with single queue would contains many locks and bad performance.
May be flat combinig implementation is better choise for us...

@andrewauclair
Copy link
Contributor

This change appears to fail with the Thread Sanitizer. The actual log has much more in it https://github.com/pocoproject/poco/actions/runs/8982526627/job/24670333088?pr=4548.

testActiveThreadLoadBalancing: ==================
WARNING: ThreadSanitizer: data race (pid=33153)
  Read of size 8 at 0x7ffe63aa0b60 by thread T16:
    #0 Poco::ActiveThread::run() <null> (libPocoFoundation.so.103+0xf9197)
    #1 <null> <null> (libPocoFoundation.so.103+0x1cc80e)
    #2 Poco::ThreadImpl::runnableEntry(void*) <null> (libPocoFoundation.so.103+0x1cdebd)

bas524 added 2 commits May 8, 2024 13:55
try increase a number of tasks and remove sleep from short-task for
tests
@matejk
Copy link
Contributor

matejk commented Aug 31, 2024

@bas524 Does the PR #4624 with different implementation that was merged to main satisfy the needs?

@bas524
Copy link
Contributor Author

bas524 commented Sep 3, 2024

@bas524 Does the PR #4624 with different implementation that was merged to main satisfy the needs?

Hi, I'll check it next week

@bas524
Copy link
Contributor Author

bas524 commented Oct 12, 2024

@bas524 Does the PR #4624 with different implementation that was merged to main satisfy the needs?

@matejk
I answered into #4624 (comment)

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.

3 participants