Re: [PATCH] Comm::Connection and friends

From: Amos Jeffries <squid3_at_treenet.co.nz>
Date: Sun, 05 Jun 2011 01:45:21 +1200

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
Received on Sat Jun 04 2011 - 13:45:47 MDT

This archive was generated by hypermail 2.2.0 : Sat Jun 04 2011 - 12:00:03 MDT