Re: [AUDIT PATCH] comm cleanup - pconn changes

On 10/05/2010 02:46 AM, Amos Jeffries wrote:

>>>> - list->removeFD(fd); /* might delete list */
>>>> - comm_close(fd);
>>>> + /* might delete list */
>>>> + if (list&& list->remove(conn)) {
>>>> + Comm::ConnectionPointer nonConst = conn;
>>>> + nonConst->close();
>>>> + }

>>> The list member cannot be NULL until the remove is called. The nonConst
>>> conversion is also weird but I will discuss that later. Should probably
>>> be rewritten as:
>>>
>>> Comm::ConnectionPointer deletedConn =
>>> list->remove(conn); // might delete list
>>> if (deletedConn != NULL)
>>> deletedConn->close();
>>>
>>> However, this is still not equivalent to the old code because the old
>>> code was closing the connection regardless of whether it was found in
>>> the list. Is that an intentional change? Perhaps add a comment about it?

> After a closer look. PconnPool::pop is the only external user of
> remove() and that is already doing fingUseable().
>
> By doing:
> * move that instance of remove() into the end of findUseable().
> * move findIndex() out of remove() into the Read and Timeout methods.
> * make remove(conn) into removeAt(int) taking the found index,
>
> results in:
> * making findUseable equivalent to a search + pop()
> * removing one pass over the list in the 'fast' path.

This may address a few comments indeed. Please do not forget about the
"we used to always close and now we do not" comment that seems unrelated
to the above change plan.

>>>> + for (int index = 0; index< nfds; index++)
>>>> + theList[index] = oldList[index];
>>>
>>> This makes IdleConnList::push slower that the old code that used
>>> xmemcpy(). The shifting overhead is one of the reasons we should not use
>>> [C] arrays here, IMO.
>
> Um, I didn't think it safe to memcpy an array of virtual classes.

No, we must not. There are standard container classes, however, that use
neither expensive shift nor class-violating memcpy to add or remove an
element. And they are safer than a C array too! IIRC, we are discussing
this in another message on this thread.

> If it is we should do it in removeAt() as well for faster shuffle there
> too where its more critical.

We should not shuffle, IMO.

> push() is not a critical path, since it occurs between requests.

There is no such thing as "between requests" in Squid as long as there
are concurrent requests waiting to be served. Push() is on the critical
path just like pop() because, on a busy server, there is usually an
[unrelated] transaction waiting for that push() to finish.

>>>> + /**
>>>> + * Updates destLink to point at an existing open connection if
>>>> available and retriable.
>>>> + * Otherwise, return false.
>>>> + *
>>>> + * We close available persistent connection if the caller
>>>> transaction is not
>>>> + * retriable to avoid having a growing number of open connections
>>>> when many
>>>> + * transactions create persistent connections but are not
>> retriable.
>>>> + */
>>>> + bool pop(Comm::ConnectionPointer&serverConn, const char *domain,
>>>> bool retriable);
>>>
>>> Why not always return the [possibly nil] popped connection instead?
>>> Seems like a simpler and safer interface then separating the serverConn
>>> value from the pop() status value.
>>
>> Hmm, will think it over. It started that way, I can't recall the details
>> why I changed it.
>
> Found it, saved some lines in the callers. On second thoughts, doing pop
> the other way while being a bit more code lets the forwarding keep its
> 'key' serverDestinations[0] conn unchanged for a second lookup if the
> first fails.
>
> Reverted this change.

Yeah. We need to add a "nil" test to RefCounter pointers so that we can
just write

     if (Some::Pointer p = foo->bar()) {
         ... p is not nil ...
     }

and avoid "saves a few lines" doubts.

It is not trivial to do safely, but there are a few simple tricks that
modern compilers should support well.

HTH,

Alex.
Received on Wed Oct 06 2010 - 05:35:52 MDT