Re: [DRAFT][MERGE] Cleanup comm outgoing connections in trunk

On 06/24/2010 09:54 PM, Amos Jeffries wrote:
>> The value of your changes is in moving the handling of connection
>> retries and multiple destinations from high-level code to a dedicated
>> ConnOpener class. I agree that it is the right thing to do. No argument
>> there.
>>
>> I just think it should be done differently when it comes to handling
>> possible Connection destinations. I think it is wrong to store a vector
>> of unconnected Connections and then use the first element when we can
>> store a vector of Destinations and use one resulting Connection instead.
>>
>
> Agreed. The Destination being IPs, we end up with an object which stores:
> destination IP + port
> intended local IP + port
> intended TCP / TLS flags
> flags for whether FD is pconn or pinned
> ptr to the peer if a peer link.
>
> ... (the fields of Comm::Connection data storage).

Sounds like a starting point to me. It would be fairly easy to change
the Destination internals so I am not focusing on this now. I suspect we
will end up with either a "many kinds of Destinations in one" messy
class or several Destination subclasses.

> using two objects would call for this pathway:
> generated by peer selection, passed to forwarding,
> passed to comm, COPIED to comm with an FD number added, COPIED to
> logging, then discarded.

I believe the pathway through the code is the same for one or two
classes. In one-class case, you pass and carry around unconnected
Connections. In the two-class case, you pass and carry around
Destinations until the very last step when Connection appears.

If copying is a concern we can use pointers. In fact, since you want a
vector of Destinations, pointers are a good idea for both one- and
two-class solutions. Not much performance difference here, again.

> Using Comm::Connection from template stage through to completed
> connection with just an update of the FD field (instead of flag + field)
> gets rid of one copy already. Next upgrade will be towards getting rid
> of the second.
> Along with several other copies on the client conn.

I do not see extra copies (as explained above). The two-class solution
has one extra allocation (for the final Connection class), but its
average allocation may be smaller and simpler in many environments, so
it is really not clear which solution is "faster", and I bet both are
about the same.

Performance is a red herring in this debate, IMO. Both solutions are
fast enough and can be optimized further. Code quality is what we should
focus on here.

<skipping minor points that are not important for the primary debate
even if we disagree on them>

> There are several different ways this same identical data is being used,
> fields of ConnStateData, fde, HttpRequest meta fields, AccessLogEntry
> fields. function parameters passed around piece-meal with some complex
> lookups needed when any given piece of the data is not passed over some
> middle section of code (comm_local_port() etc).
>
> This is an aim to normalize the particular and small TCP link data
> end-to-end across the squid processing pathways without copying.
>
> I have not yet fully merged fde in yet, but when done the fd_table can
> be a table/vector/map of pointers to Comm::Connection which have been
> opened.

You are trying to solve many problems at once. I would not recommend it,
but I am happy to review Comm::Connection as a "socket plus metadata"
wrapper class that we definitely need, for many obvious reasons. My
objection to abusing that class as Destination replacement becomes even
stronger then.

Cheers,

Alex.
Received on Fri Jun 25 2010 - 18:03:21 MDT