[squid-dev] [PATCH] Happy Eyeballs: Use each fully resolved destination ASAP

[Alex Rousskov]

On 05/24/2017 09:04 AM, Amos Jeffries wrote:
> For the 1st destination ps_state::handlePath() calls noteDestination()
> which results in the entire TunnelState::startConnecting() /
> FwdState::startConnectionOrFail() sequence happening synchronously
> before returning to ps_state::handlePaths() and peerSelectDnsResults()
> for the 2nd path.

That is correct, although the phrasing "ENTIRE sequence" implies some
significant delays that are usually not there. The "sequence" usually
stops "soon" because it starts an async job (e.g., a PeerConnector or a
Client job).


> There do not seem to be any performance gains from this redesign for
> transactions where there are less than 3 upstream servers.

This is both incorrect and largely irrelevant:

* Incorrect: The new code makes forwarding faster in cases where there
are two (or more) destinations and the second destination requires a DNS
lookup -- the new code will start using the first destination sooner
and, in typical cases, will finish forwarding sooner. See my earlier
specific example for an illustration.

* Irrelevant: This performance gain is just a nice side effect. The
primary goal is to facilitate Happy Eyeballs (which requires this
parallelization and more).


> The synchronous processing may make the one transaction
> reach connection faster, but that is just taking CPU cycles from /
> slowing other transactions.

The patch parallelizes second DNS lookup and first HTTP transaction
execution. That parallelization is not about CPU cycles but about the
time the first HTTP transaction spends waiting for (blocked on) that
second (and usually unused) DNS lookup in the old code. The patched code
does not wait/block. Again, the gains from this optimization are not the
goal, but some environments will see them.


> It might be worth initiating that noteDestination() sub-path as async
> step to avoid the remaining delay.

Yes, and my second thread email stated that much. However, the reason
for making that call async is not performance optimization (async calls
make performance worse, not better!) or delays. The reason is to shorten
the call stack, reducing the number of balls in the air and localizing bugs.


> The [forwarding timeout] is no longer determined by which component is
> being problematic. It was nice being able to point squid-users questions
> in roughly the right direction without resorting to logs very often.

Agreed. Happy Eyeballs is a more complex "parallel" algorithm than the
current sequential DNS-then-HTTP approach. Parallel algorithms are more
difficult to implement, understand, and support. Hopefully, we agree
that Squid should support Happy Eyeballs despite those costs. Thus, I do
not understand why you are discussing multiple possible reasons behind
timeouts here.

( FWIW, resorting to messy debugging logs is necessary primarily because
of poor reporting/triaging interfaces in Squid. It is possible to make
triage much better without resorting to debugging logs. Such
improvements would help triaging Happy Eyeballs as well. )


> 1c) server 5xx response arrives for A,
>   - startConnectionOrFail() now has serverDestinations.empty() and an
> error to deliver

Fixed in the attached take4 patch. The tunneling code already has
similar logic, but I missed it in the forwarding case.

I also explicitly cleared peer selection subscription when FwdState
tries to stop, just in case the self-clearing hack does not work.

An incremental diff showing changes since the last take is also attached.


Thank you,

Alex.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: SQUID69-use-destination-asap-t4.patch
Type: text/x-diff
Size: 89429 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20170524/f54c70f4/attachment-0002.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: incremental.patch
Type: text/x-diff
Size: 32351 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20170524/f54c70f4/attachment-0003.patch>