> On 1 Jun 2017, at 17:15, Yann Ylavic <ylavic....@gmail.com> wrote: > > On Thu, Jun 1, 2017 at 10:48 PM, Jim Riggs <apache-li...@riggs.me> wrote: >>> On 1 Jun 2017, at 15:25, Yann Ylavic <ylavic....@gmail.com> wrote: >>> >>> On Thu, Jun 1, 2017 at 10:22 PM, Yann Ylavic <ylavic....@gmail.com> >>> wrote: >>>> On Thu, Jun 1, 2017 at 7:29 PM, Jim Riggs <apache-li...@riggs.me> >>>> wrote: >>>>>> On 1 Jun 2017, at 07:55, Jim Jagielski <j...@jagunet.com> >>>>>> wrote: 2. I understand the logic behind creating the arrays, >>>>>> but doesn't this increase the overhead. We go thru the full >>>>>> list of workers one time, and then go thru the list of avail >>>>>> works and spares right after that. Assume that all workers >>>>>> are available; doesn't it mean we go thru that last 2x? >>>> [] >>>>> >>>>> The only way I can think of to avoid this without going back >>>>> to duplicating code across lbmethods, which I would argue is >>>>> worse, is to maybe have the lbmethod provide a callback >>>>> function and context pointer to >>>>> ap_proxy_balancer_usable_workers() that gets called during the >>>>> loop iteration to pick the best member in flight. >>>> >>>> Couldn't a simple 'best' for ap_proxy_balancer_usable_workers() >>>> make it return a single entry? >>> >>> ... a simple 'best' *flag* (as argument) ... >> >> I'm not sure I follow what this flag would be. The lbmethod would >> somehow have to tell ap_proxy_balancer_usable_workers() how to pick >> the best worker (e.g. by comparing the number of bytes sent or the >> number of requests processed). I'm not sure how that information >> could be passed as a flag unless we baked the behavior of byrequests, >> bybusyness, and bytraffic into ap_proxy_balancer_usable_workers(). >> But then how would we allow for plugging in additional lbmethods? > > Oh right, nevermind, I thought per lbmethod logic was already there. > After a better look into it, a callback (and its baton) with that > logic looks straightforward, though.
Indeed. I put a new patch that uses callbacks on BZ61140 (and visually available at https://github.com/jhriggs/httpd/pull/1/files). So, the lbmethods call ap_proxy_balancer_get_best_worker() with a callback and baton so that the best worker is chosen in flight during iteration of the workers, addressing Jim's overhead concern. Changes: - use a callback and baton to the lbmethod in ap_proxy_balancer_get_best_worker() (previously ap_proxy_balancer_usable_workers()) so that we can find the best worker in flight while iterating rather than returning an array of usable workers that has to then in turn be iterated - consider a draining worker unusable and suitable for replacement by a spare > Regarding: > worker = &APR_ARRAY_IDX(balancer->workers, i, proxy_worker *); > I don't see why worker needs to be a 'proxy_worker**' ('*worker' is > never updated IIUC). > > Looks like: > proxy_worker *worker = APR_ARRAY_IDX(balancer->workers, i, proxy_worker *); > would be fine and allow using 'worker->xxx' instead of > '(*worker)->xxx' all over the place... Much of what I did was based on what already existed. The existing code was iterating through proxy_worker**, so mine did too. You are correct, though, that there is no real reason for it. The latest patch switches to just using * rather than **. Thanks for putting eyes on this! I appreciate it.
