#51151 Issue 51072 - Set the default minimum worker threads
Closed by spichugi. Opened by mreynolds.
mreynolds/389-ds-base issue51072  into  master

Download 51151.patch

Description:

Testing has shown that using current number of CPU cores to set the thread number gives the best performance, but when there are expensive operations total throughput drops.

We still need a minimum number of workers threads to handle a wide range of operations. We decided for now that the minimum should be 16 workers.

relates: https://pagure.io/389-ds-base/issue/51072

Only comment is to make 16 a #define MIN_THREADS because we could always keep this so that if we lower it later that we have say MIN_THREADS = 1, then we avoid the -1 case if we can't detect.

Could we also have a case to handle hw_threads == 0/negative which returns a min too, and gives the proper error?

Beside that little implementation detail it's good. Thanks :) Happy to re-read if you tweak it.

Only comment is to make 16 a #define MIN_THREADS because we could always keep this so that if we lower it later that we have say MIN_THREADS = 1, then we avoid the -1 case if we can't detect.

Sure

Could we also have a case to handle hw_threads == 0/negative which returns a min too, and gives the proper error?

It already handles this case. If the syscall fails it returns -1, but in the code, if its less than 16 we set it to 16 anyway. So this handles all the cases correctly. So the function will never return a value less than 16 to the caller. That's how I was able to remove the checks for -1

I think it's more, let's say we get the -1. Today we return min_threads, but I think we should have "on -1 make noise and return some error level of threads like 16 because we might be on a real server", vs "we have 4 cores, okay, let's set min to 16".

Imagine we knock min threads down to say ... 2? Then on a -1 we have a bad-time awaiting, so I want to make angry loud noises in this case :)

Thoughts? Perhaps just on the hw_threads < 1 case, we should raise an error, and move on with our lives?

Shouldn't be tested against 'threads' value ?
I agree with the use of a define MIN_THREADS or MIN_THREADNUMBER

1 new commit added

  • Define MAX and MIN threads, and improve logging

2 new commits added

  • Define MAX and MIN threads, and improve logging
  • Issue 51072 - Set the default minimum worker threads

Changes applied, please review...

Looks good to me. Ack

rebased onto dc7bf4a767a6aec72a664e99677f0075ab89ac2c

Pull-Request has been merged by mreynolds

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4204

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi

Metadata