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

Issue 5761 - Worker thread dynamic management #5796

Merged
merged 4 commits into from Sep 18, 2023
Merged

Conversation

progier389
Copy link
Contributor

@progier389 progier389 commented Jun 12, 2023

Objectives:

  • Allow to configure the number of worker threads without having to restart the server
  • Decrease the worker thread global mutex contention but removing the associated condition variable
    ==> Increase the "searchrate" performance

Solution: See https://github.com/389ds/389ds.github.io/blob/main/docs/389ds/design/worker-threads.md

Issue: 5761

Reviewed by: @tbordaz (Thanks!)

@progier389 progier389 added the work in progress Work in Progress - can be reviewed, but not ready for merge. label Jun 12, 2023
@progier389
Copy link
Contributor Author

Creating the PR to be able to easily share the prototype.
So far the firsts tests using ldclt -e bindeach,esearch with default config on my laptop and was a bit surprised by the result and had to double check in the access log shows that the bind and search are really done ! -;)
Looking forward for the real performance tests ...

@progier389 progier389 removed the work in progress Work in Progress - can be reviewed, but not ready for merge. label Aug 9, 2023
@progier389 progier389 self-assigned this Aug 9, 2023
@progier389 progier389 linked an issue Aug 9, 2023 that may be closed by this pull request
@progier389
Copy link
Contributor Author

Performance tests shows that there are no performance regression during stress tests but no much improvement either
Work queue lock contention time decreased but once again it does not seems to impact the stress performance
The main interest is that the number of thread can now be changed dynamically.

Copy link
Contributor

@tbordaz tbordaz left a comment

Choose a reason for hiding this comment

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

The code looks very nice. Just few minor comments/questions

void
op_thread_set_threads_number(int threadsnumber)
{
int oldnbthreads = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

If op_thread_set_thread_number is called in parallel, should not serialize the call ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I should add a specific mutex to serialize this function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is now fixed by latest commit

CONN_FOUND_WORK_TO_DO,
CONN_SHUTDOWN,
CONN_NOWORK,
CONN_DONE,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it still needed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, these values are still used by the worker thread (but anyway a merge with the turbo mode removal is needed)

Copy link
Contributor

Choose a reason for hiding this comment

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

I was more specifically talking about CONN_DONE (IIRC)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

grep -c CONN_DONE connection.c
18

ldap/servers/slapd/connection.c Show resolved Hide resolved
Copy link
Contributor

@tbordaz tbordaz left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. LGTM

@progier389 progier389 merged commit c3a69bb into 389ds:main Sep 18, 2023
183 of 193 checks passed
progier389 added a commit to progier389/389-ds-base that referenced this pull request Oct 24, 2023
progier389 added a commit that referenced this pull request Oct 25, 2023
This reverts commit c3a69bb about the
Worker thread dynamic management feature because it caused a regression
in freeipa CI tests due to a massive performance loss during a total update
( https://issues.redhat.com/browse/IDMDS-3781 )

Issue: #5761

Reviewed by: @tbordaz (Thanks!)
@progier389 progier389 deleted the i5761 branch March 28, 2024 13:33
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.

Dynamically configure number of worker threads
2 participants