On 10/13/21 3:38 PM, Yann Ylavic wrote:
> On Tue, Oct 12, 2021 at 6:05 PM Ruediger Pluem <rpl...@apache.org> wrote:
>>
>> On 10/12/21 4:58 PM, Yann Ylavic wrote:
>>>
>>> Shouldn't we also compute a busy_thread_count and kill children
>>> if/when later things settle down and thus busy_thread_count <
>>> max_spare_threads (like in the attached patch)?
>>
>> Hm. Wouldn't it be better instead to ensure that idle_thread_count is "far"
>> larger than the min_spare_threads something like.
>> idle_thread_count > active_daemons_limit/2/num_buckets*ThreadsPerChild +
>> min_spare_threads
>
> Right, better. Given that max_workers = active_daemons_limit *
> ThreadsPerChild, let's rewrite this as:
> idle_thread_count > (max_workers/2 + min_spare_threads) / num_buckets
> I don't get the +min_spare_threads though, if "min_spare_threads >=
> max_workers/2" this would be above max_workers thus never trigger?
You are correct, probably bad. I just wanted to ensure that we have enough
distance to min_spare_threads, but max_workers/2 should do.
>
> When about to kill children we already know that "idle_thread_count >
> max_spare_threads/num_buckets" (the block we are in), so above
> max_spare_threads is not necessarily far larger than min_spare_threads
> but far enough?
I guess this was the key point here, that max_spare_threads and
min_spare_threads are too close together (probably only one or two
processes) and that a process we shutdown does not die fast enough to make room
in the scoreboard once we go below
min_spare_threads. This is no problem at at all if we still have plenty of room
until reaching server_limit, but if not we could
end up with scoreboard full situation.
>
> Given also that "active_daemons_limit <= server_limit" here (per
I guess the point is that active_daemons_limit could be == server_limit if
MaxRequestWworker / Threadsperchild == ServerLimit.
The idea was that we don't kill any process if we are at ServerLimit. The new
approach would be to allow that if the idle threads
are far larger than min_spare_threads and hence the danger that we need to
replace the dying process "soon" with a new one as we
go below min_spare_threads and can't because of a full scoreboard does not
happen.
> event_check_config), how about we simply test for:
> if (retained->total_daemons <= active_daemons_limit
As explained above I would keep the current condition. In a "sensible"
configured server where MaxRequestWworker / Threadsperchild
< ServerLimit the condition would not be needed as you state.
> || idle_thread_count > max_workers/{2,3,4} / num_buckets) {
> /* kill one child */
> }
> else {
> /* still busy enough, don't kill */
> }
> ?
>
>>
>> (2 is arbitrary choice could be also larger to start earlier reducing
>> daemons)
>
> 4 would be fine by me, opinions?
4 is fine for me as well. I would choose a power of two as this should provide
a fast integer division by just shifting bits.
>
>>
>> This would prevent the situation that we have a dying daemon and a rise of
>> requests would cause the start of a new daemon "soon".
>
> By killing one child we'll "retained->total_daemons <=
> active_daemons_limit" again and come back to normal logic: kill while
> "idle_thread_count > max_spare_threads/num_buckets" and no new child
> started until "idle_thread_count < min_spare_threads / num_buckets"
> (and since idle_spawn_rate was reset to 1 there will be no storm
> when/if the latter happens).
> So we should be good?
Yes.
Regards
Rüdiger
