Re: [PATCH] Comm::Connection and friends

On 01/04/11 04:44, Alex Rousskov wrote:
> On 03/31/2011 07:50 AM, Amos Jeffries wrote:
>>> Please s/dataDescriptor/primaryConnection/ or something like that.
>>
>> Left that unchanged from the existing Server API. name and semantics both.
>> It needs co-ordinating with FTP dual-socket requirement (data channel +
>> control channel) thrown in for extra confusion against the HTTP which
>> would actually be best called "serverConnection".
>> "primary" in FTP is control. "data" is where the reply body comes from.
>> "primary" in HTTP has no meaning, "data" is where the reply comes from.
>> the others do not yet use the server API, but follow the HTTP semantics
>> more closely.
>>
>> I'm happy to change the name here, there is zero increase to the patch
>> but only if there is less confusion afterwards.
>
>
> If FTP calls the control connection "primary", then primaryConnection is
> a bad choice. Consider "dataConnection" then to remove the "descriptor"
> part that is no longer true, but it is not a big deal to leave as is
> until a better term is found.
>
>

Fixed. Using data.Connection()

>>> Several changes may have a negative performance effect (e.g., memory
>>> pools for persistent connection lists are gone?). There are also many
>>> rather complex changes that might change functionality even if we do not
>>> expect it. If this patch is committed now, are you going to port it to
>>> the v3.2 branch or is it a v3.3-only change?
>>
>> 3.2.
>> Some of those changes (eg forwarding and peer selection) are outstanding
>> blockers on 3.2 stability in their own right. The others appear to raise
>> performance a little as side effects. Then there are the bugs.
>
> Please document a potential performance regression in a commit message.
> I doubt the removal of memory pools will be noticeable here, but we
> should get into a habit declaring suspicious spots so that it is easier
> to identify them in a big range of revisions during regression testing.
>

Instead of only pooling the pconn structures. I'm now pooling that
Comm::Connection objects. Overall that should more than balance the
small loss here.

Updated patch re-submitted in a new thread.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.12
   Beta testers wanted for 3.2.0.8 and 3.1.12.2