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.
