Re: [PATCH] ConnOpener fixes

On 01/30/2013 08:18 PM, Amos Jeffries wrote:

> One performane optimization I can see:
>
> test for the
> callback_->cancelled() case and debugs() report it instead of scheduling.

Will add.

> The result of omitting this check saves one if() test per connection by
> adding the full overheads of memory, scheduling, and CPU for one
> AsyncCall queue operation per connection. ConnOpener operates on the
> server-side where multiple client-side Jobs may result in TIMEOUT or
> ABORTED. This cancellation case is a small but common case.

FWIW, I consider the fact that Squid schedules dead-end async calls a
performance bug. IIRC, I made ScheduleCall() return bool specifically to
return false when there is no reason to schedule the call (because it
cannot be dialed). AFAIK, it is a common enough case to be worth an
extra check -- I see a lot of dead calls scheduled in production
debugging logs, especially around job termination time.

It would be nice to investigate whether that check was removed (why?) or
never written. It would not prevent AsyncCall memory allocation, but it
would still save a lot of cycles _and_ improve code quality and debugging.

In the same time, somebody should investigate whether asyncCall() can
return nil when the dialer cannot dial -- that would prevent even the
memory allocation for the stillborn async call!

Thank you,

Alex.
Received on Thu Jan 31 2013 - 06:03:44 MST