From 11a28b361de6a93b5f9d881e3ca04720580b202f Mon Sep 17 00:00:00 2001 From: Cameron Kaiser Date: Sun, 22 Mar 2020 18:58:04 -0700 Subject: [PATCH] #594: M865314 M1275917 (improve TLS parallelism) --- netwerk/base/Predictor.cpp | 7 -- netwerk/base/nsISpeculativeConnect.idl | 9 +- netwerk/protocol/http/AlternateServices.cpp | 7 -- .../protocol/http/ConnectionDiagnostics.cpp | 4 +- netwerk/protocol/http/Http2Session.cpp | 14 +++ netwerk/protocol/http/SpdySession31.cpp | 14 +++ netwerk/protocol/http/nsHttpConnection.cpp | 2 - netwerk/protocol/http/nsHttpConnectionMgr.cpp | 98 ++++++++++++------- netwerk/protocol/http/nsHttpConnectionMgr.h | 8 +- 9 files changed, 93 insertions(+), 70 deletions(-) diff --git a/netwerk/base/Predictor.cpp b/netwerk/base/Predictor.cpp index a04b4e2b0..b6287b5a3 100644 --- a/netwerk/base/Predictor.cpp +++ b/netwerk/base/Predictor.cpp @@ -466,13 +466,6 @@ Predictor::GetIgnoreIdle(bool *ignoreIdle) return NS_OK; } -NS_IMETHODIMP -Predictor::GetIgnorePossibleSpdyConnections(bool *ignorePossibleSpdyConnections) -{ - *ignorePossibleSpdyConnections = true; - return NS_OK; -} - NS_IMETHODIMP Predictor::GetParallelSpeculativeConnectLimit( uint32_t *parallelSpeculativeConnectLimit) diff --git a/netwerk/base/nsISpeculativeConnect.idl b/netwerk/base/nsISpeculativeConnect.idl index 15d63e107..e9ca29fb1 100644 --- a/netwerk/base/nsISpeculativeConnect.idl +++ b/netwerk/base/nsISpeculativeConnect.idl @@ -37,7 +37,7 @@ interface nsISpeculativeConnect : nsISupports * inline) to determine whether or not to actually make a speculative * connection. */ -[builtinclass, uuid(f6a0d1e5-369f-4abc-81ae-d370d36e4006)] +[builtinclass, uuid(1040ebe3-6ed1-45a6-8587-995e082518d7)] interface nsISpeculativeConnectionOverrider : nsISupports { /** @@ -46,13 +46,6 @@ interface nsISpeculativeConnectionOverrider : nsISupports */ [infallible] readonly attribute unsigned long parallelSpeculativeConnectLimit; - /** - * Used to loosen the restrictions nsHttpConnectionMgr::RestrictConnections - * to allow more speculative connections when we're unsure if a host will - * connect via SPDY or not. - */ - [infallible] readonly attribute boolean ignorePossibleSpdyConnections; - /** * Used to determine if we will ignore the existence of any currently idle * connections when we decide whether or not to make a speculative diff --git a/netwerk/protocol/http/AlternateServices.cpp b/netwerk/protocol/http/AlternateServices.cpp index bfe97a844..4442dc1e0 100644 --- a/netwerk/protocol/http/AlternateServices.cpp +++ b/netwerk/protocol/http/AlternateServices.cpp @@ -550,13 +550,6 @@ AltSvcOverride::GetIgnoreIdle(bool *ignoreIdle) return NS_OK; } -NS_IMETHODIMP -AltSvcOverride::GetIgnorePossibleSpdyConnections(bool *ignorePossibleSpdyConnections) -{ - *ignorePossibleSpdyConnections = true; - return NS_OK; -} - NS_IMETHODIMP AltSvcOverride::GetParallelSpeculativeConnectLimit( uint32_t *parallelSpeculativeConnectLimit) diff --git a/netwerk/protocol/http/ConnectionDiagnostics.cpp b/netwerk/protocol/http/ConnectionDiagnostics.cpp index 5a3b818b2..61cc156a5 100644 --- a/netwerk/protocol/http/ConnectionDiagnostics.cpp +++ b/netwerk/protocol/http/ConnectionDiagnostics.cpp @@ -72,8 +72,8 @@ nsHttpConnectionMgr::PrintDiagnosticsCB(const nsACString &key, ent->mHalfOpens.Length()); self->mLogData.AppendPrintf(" Coalescing Keys Length = %u\n", ent->mCoalescingKeys.Length()); - self->mLogData.AppendPrintf(" Spdy using = %d, tested = %d, preferred = %d\n", - ent->mUsingSpdy, ent->mTestedSpdy, ent->mInPreferredHash); + self->mLogData.AppendPrintf(" Spdy using = %d, preferred = %d\n", + ent->mUsingSpdy, ent->mInPreferredHash); self->mLogData.AppendPrintf(" pipelinestate = %d penalty = %d\n", ent->mPipelineState, ent->mPipeliningPenalty); for (i = 0; i < nsAHttpTransaction::CLASS_MAX; ++i) { diff --git a/netwerk/protocol/http/Http2Session.cpp b/netwerk/protocol/http/Http2Session.cpp index 163eb371c..8c07487a0 100644 --- a/netwerk/protocol/http/Http2Session.cpp +++ b/netwerk/protocol/http/Http2Session.cpp @@ -405,6 +405,20 @@ Http2Session::AddStream(nsAHttpTransaction *aHttpTransaction, mConnection = aHttpTransaction->Connection(); } + if (mClosed || mShouldGoAway) { + nsHttpTransaction *trans = aHttpTransaction->QueryHttpTransaction(); + if (trans) { + RefPtr wrapr = trans->GetPushedStream(); + if (!wrapr) { + LOG3(("Http2Session::AddStream %p atrans=%p trans=%p session unusable - resched.\n", + this, aHttpTransaction, trans)); + aHttpTransaction->SetConnection(nullptr); + gHttpHandler->InitiateTransaction(trans, trans->Priority()); + return true; + } + } + } + aHttpTransaction->SetConnection(this); if (aUseTunnel) { diff --git a/netwerk/protocol/http/SpdySession31.cpp b/netwerk/protocol/http/SpdySession31.cpp index b3c9ada7d..1995027b1 100644 --- a/netwerk/protocol/http/SpdySession31.cpp +++ b/netwerk/protocol/http/SpdySession31.cpp @@ -357,6 +357,20 @@ SpdySession31::AddStream(nsAHttpTransaction *aHttpTransaction, mConnection = aHttpTransaction->Connection(); } + if (mClosed || mShouldGoAway) { + nsHttpTransaction *trans = aHttpTransaction->QueryHttpTransaction(); + if (trans) { + RefPtr wrapr = trans->GetPushedStream(); + if (!wrapr) { + LOG3(("SpdySession31::AddStream %p atrans=%p trans=%p session unusable - resched.\n", + this, aHttpTransaction, trans)); + aHttpTransaction->SetConnection(nullptr); + gHttpHandler->InitiateTransaction(trans, trans->Priority()); + return true; + } + } + } + aHttpTransaction->SetConnection(this); if (aUseTunnel) { diff --git a/netwerk/protocol/http/nsHttpConnection.cpp b/netwerk/protocol/http/nsHttpConnection.cpp index 2066117e6..851ca5fac 100644 --- a/netwerk/protocol/http/nsHttpConnection.cpp +++ b/netwerk/protocol/http/nsHttpConnection.cpp @@ -260,8 +260,6 @@ nsHttpConnection::StartSpdy(uint8_t spdyVersion) mUsingSpdyVersion = spdyVersion; mEverUsedSpdy = true; - // see also bug 865314 for backbugs - // https://hg.mozilla.org/mozilla-central/rev/b1e8d6cf54e6 if (!mDid0RTTSpdy) { mSpdySession = ASpdySession::NewSpdySession(spdyVersion, mSocketTransport, false); diff --git a/netwerk/protocol/http/nsHttpConnectionMgr.cpp b/netwerk/protocol/http/nsHttpConnectionMgr.cpp index 22166543b..eb444fd02 100644 --- a/netwerk/protocol/http/nsHttpConnectionMgr.cpp +++ b/netwerk/protocol/http/nsHttpConnectionMgr.cpp @@ -395,7 +395,6 @@ public: // intentional! bool mOverridesOK; uint32_t mParallelSpeculativeConnectLimit; bool mIgnoreIdle; - bool mIgnorePossibleSpdyConnections; bool mIsFromPredictor; bool mAllow1918; @@ -447,8 +446,6 @@ nsHttpConnectionMgr::SpeculativeConnect(nsHttpConnectionInfo *ci, overrider->GetParallelSpeculativeConnectLimit( &args->mParallelSpeculativeConnectLimit); overrider->GetIgnoreIdle(&args->mIgnoreIdle); - overrider->GetIgnorePossibleSpdyConnections( - &args->mIgnorePossibleSpdyConnections); overrider->GetIsFromPredictor(&args->mIsFromPredictor); overrider->GetAllow1918(&args->mAllow1918); } @@ -690,8 +687,6 @@ nsHttpConnectionMgr::ReportSpdyConnection(nsHttpConnection *conn, if (!ent) return; - ent->mTestedSpdy = true; - if (!usingSpdy) return; @@ -711,13 +706,14 @@ nsHttpConnectionMgr::ReportSpdyConnection(nsHttpConnection *conn, nsConnectionEntry *joinedConnection; nsConnectionEntry *preferred = LookupPreferredHash(ent); - LOG(("ReportSpdyConnection %p,%s prefers %p,%s\n", - ent, ent->mConnInfo->Origin(), preferred, + LOG(("ReportSpdyConnection %p,%s conn %p prefers %p,%s\n", + ent, ent->mConnInfo->Origin(), conn, preferred, preferred ? preferred->mConnInfo->Origin() : "")); if (!preferred) { // this becomes the preferred entry StorePreferredHash(ent); + preferred = ent; } else if ((preferred != ent) && (joinedConnection = GetSpdyPreferredEnt(ent)) && (joinedConnection != ent)) { @@ -728,8 +724,7 @@ nsHttpConnectionMgr::ReportSpdyConnection(nsHttpConnection *conn, // new transactions migrate over. LOG(("ReportSpdyConnection graceful close of conn=%p ent=%p to " - "migrate to preferred\n", conn, ent)); - + "migrate to preferred (desharding)\n", conn, ent)); conn->DontReuse(); } else if (preferred != ent) { LOG (("ReportSpdyConnection preferred host may be in false start or " @@ -737,6 +732,36 @@ nsHttpConnectionMgr::ReportSpdyConnection(nsHttpConnection *conn, "abandon this connection yet.")); } + if ((preferred == ent) && conn->CanDirectlyActivate()) { + // this is a new spdy connection to the preferred entry + + // Cancel any other pending connections - their associated transactions + // are in the pending queue and will be dispatched onto this connection + for (int32_t index = ent->mHalfOpens.Length() - 1; + index >= 0; --index) { + LOG(("ReportSpdyConnection forcing halfopen abandon %p\n", + ent->mHalfOpens[index])); + ent->mHalfOpens[index]->Abandon(); + } + + if (ent->mActiveConns.Length() > 1) { + // this is a new connection to an established preferred spdy host. + // if there is more than 1 live and established spdy connection (e.g. + // some could still be handshaking, shutting down, etc..) then close + // this one down after any transactions that are on it are complete. + // This probably happened due to the parallel connection algorithm + // that is used only before the host is known to speak spdy. + for (uint32_t index = 0; index < ent->mActiveConns.Length(); ++index) { + nsHttpConnection *otherConn = ent->mActiveConns[index]; + if (otherConn != conn) { + LOG(("ReportSpdyConnection shutting down connection (%p) because new " + "spdy connection (%p) takes precedence\n", otherConn, conn)); + otherConn->DontReuse(); + } + } + } + } + ProcessPendingQ(ent->mConnInfo); PostEvent(&nsHttpConnectionMgr::OnMsgProcessAllSpdyPendingQ); } @@ -926,9 +951,7 @@ nsHttpConnectionMgr::PruneDeadConnectionsCB(const nsACString &key, ent->mActiveConns.Length() == 0 && ent->mHalfOpens.Length() == 0 && ent->mPendingQ.Length() == 0 && - ((!ent->mTestedSpdy && !ent->mUsingSpdy) || - !gHttpHandler->IsSpdyEnabled() || - self->mCT.Count() > 300)) { + (!ent->mUsingSpdy || self->mCT.Count() > 300)) { LOG((" removing empty connection entry\n")); return PL_DHASH_REMOVE; } @@ -1303,8 +1326,7 @@ nsHttpConnectionMgr::ClosePersistentConnectionsCB(const nsACString &key, } bool -nsHttpConnectionMgr::RestrictConnections(nsConnectionEntry *ent, - bool ignorePossibleSpdyConnections) +nsHttpConnectionMgr::RestrictConnections(nsConnectionEntry *ent) { MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread); @@ -1312,11 +1334,9 @@ nsHttpConnectionMgr::RestrictConnections(nsConnectionEntry *ent, // don't create any new ssl connections until the result of the // negotiation is known. - bool doRestrict = ent->mConnInfo->FirstHopSSL() && - gHttpHandler->IsSpdyEnabled() && - ((!ent->mTestedSpdy && !ignorePossibleSpdyConnections) || - ent->mUsingSpdy) && - (ent->mHalfOpens.Length() || ent->mActiveConns.Length()); + bool doRestrict = + ent->mConnInfo->FirstHopSSL() && gHttpHandler->IsSpdyEnabled() && + ent->mUsingSpdy && (ent->mHalfOpens.Length() || ent->mActiveConns.Length()); // If there are no restrictions, we are done if (!doRestrict) @@ -1324,8 +1344,9 @@ nsHttpConnectionMgr::RestrictConnections(nsConnectionEntry *ent, // If the restriction is based on a tcp handshake in progress // let that connect and then see if it was SPDY or not - if (ent->UnconnectedHalfOpens() && !ignorePossibleSpdyConnections) + if (ent->UnconnectedHalfOpens()) { return true; + } // There is a concern that a host is using a mix of HTTP/1 and SPDY. // In that case we don't want to restrict connections just because @@ -2947,14 +2968,12 @@ nsHttpConnectionMgr::OnMsgSpeculativeConnect(int32_t, ARefBase *param) uint32_t parallelSpeculativeConnectLimit = gHttpHandler->ParallelSpeculativeConnectLimit(); - bool ignorePossibleSpdyConnections = false; bool ignoreIdle = false; bool isFromPredictor = false; bool allow1918 = false; if (args->mOverridesOK) { parallelSpeculativeConnectLimit = args->mParallelSpeculativeConnectLimit; - ignorePossibleSpdyConnections = args->mIgnorePossibleSpdyConnections; ignoreIdle = args->mIgnoreIdle; isFromPredictor = args->mIsFromPredictor; allow1918 = args->mAllow1918; @@ -2964,11 +2983,10 @@ nsHttpConnectionMgr::OnMsgSpeculativeConnect(int32_t, ARefBase *param) if (mNumHalfOpenConns < parallelSpeculativeConnectLimit && ((ignoreIdle && (ent->mIdleConns.Length() < parallelSpeculativeConnectLimit)) || !ent->mIdleConns.Length()) && - !(keepAlive && RestrictConnections(ent, ignorePossibleSpdyConnections)) && + !(keepAlive && RestrictConnections(ent)) && !AtActiveConnectionLimit(ent, args->mTrans->Caps())) { CreateTransport(ent, args->mTrans, args->mTrans->Caps(), true, isFromPredictor, allow1918); - } - else { + } else { LOG((" Transport not created due to existing connection count\n")); } } @@ -3204,6 +3222,8 @@ nsresult nsHttpConnectionMgr::nsHalfOpenSocket::SetupBackupStreams() { MOZ_ASSERT(mTransaction); + MOZ_ASSERT(!mTransaction->IsNullTransaction(), + "null transactions dont have backup streams"); mBackupSynStarted = TimeStamp::Now(); nsresult rv = SetupStreams(getter_AddRefs(mBackupTransport), @@ -3227,7 +3247,7 @@ nsHttpConnectionMgr::nsHalfOpenSocket::SetupBackupTimer() { uint16_t timeout = gHttpHandler->GetIdleSynTimeout(); MOZ_ASSERT(!mSynTimer, "timer already initd"); - if (timeout && !mTransaction->IsDone()) { + if (timeout && !mTransaction->IsDone() && !mTransaction->IsNullTransaction()) { // Setup the timer that will establish a backup socket // if we do not get a writable event on the main one. // We do this because a lost SYN takes a very long time @@ -3241,10 +3261,8 @@ nsHttpConnectionMgr::nsHalfOpenSocket::SetupBackupTimer() mSynTimer->InitWithCallback(this, timeout, nsITimer::TYPE_ONE_SHOT); LOG(("nsHalfOpenSocket::SetupBackupTimer() [this=%p]", this)); } - } - else if (timeout) { - LOG(("nsHalfOpenSocket::SetupBackupTimer() [this=%p]," - " transaction already done!", this)); + } else if (timeout) { + LOG(("nsHalfOpenSocket::SetupBackupTimer() [this=%p], did not arm", this)); } } @@ -3264,8 +3282,10 @@ nsHttpConnectionMgr::nsHalfOpenSocket::CancelBackupTimer() void nsHttpConnectionMgr::nsHalfOpenSocket::Abandon() { - LOG(("nsHalfOpenSocket::Abandon [this=%p ent=%s]", - this, mEnt->mConnInfo->Origin())); + LOG(("nsHalfOpenSocket::Abandon [this=%p ent=%s] %p %p %p %p", + this, mEnt->mConnInfo->Origin(), + mSocketTransport.get(), mBackupTransport.get(), + mStreamOut.get(), mBackupStreamOut.get())); MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread); @@ -3322,6 +3342,8 @@ nsHttpConnectionMgr::nsHalfOpenSocket::Notify(nsITimer *timer) { MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread); MOZ_ASSERT(timer == mSynTimer, "wrong timer"); + MOZ_ASSERT(mTransaction && !mTransaction->IsNullTransaction(), + "null transactions dont have backup streams"); SetupBackupStreams(); @@ -3375,8 +3397,9 @@ nsHalfOpenSocket::OnOutputStreamReady(nsIAsyncOutputStream *out) mStreamOut = nullptr; mStreamIn = nullptr; mSocketTransport = nullptr; - } - else { + } else if (out == mBackupStreamOut) { + MOZ_ASSERT(!mTransaction->IsNullTransaction(), + "null transactions dont have backup streams"); TimeDuration rtt = TimeStamp::Now() - mBackupSynStarted; rv = conn->Init(mEnt->mConnInfo, gHttpHandler->ConnMgr()->mMaxRequestDelay, @@ -3392,6 +3415,9 @@ nsHalfOpenSocket::OnOutputStreamReady(nsIAsyncOutputStream *out) mBackupStreamOut = nullptr; mBackupStreamIn = nullptr; mBackupTransport = nullptr; + } else { + MOZ_ASSERT(false, "unexpected stream"); + rv = NS_ERROR_UNEXPECTED; } if (NS_FAILED(rv)) { @@ -3531,7 +3557,7 @@ nsHttpConnectionMgr::nsHalfOpenSocket::OnTransportStatus(nsITransport *trans, // this half open socket has already been abandoned. It may happen // when we get this notification right between main-thread calls to // nsHttpConnectionMgr::Shutdown and nsSocketTransportService::Shutdown - // where the first abandones all half open socket instances and only + // where the first abandons all half open socket instances and only // after that the second stops the socket thread. if (mEnt && !mBackupTransport && !mSynTimer) SetupBackupTimer(); @@ -3607,7 +3633,6 @@ nsConnectionEntry::nsConnectionEntry(nsHttpConnectionInfo *ci) , mGreenDepth(kPipelineOpen) , mPipeliningPenalty(0) , mUsingSpdy(false) - , mTestedSpdy(false) , mInPreferredHash(false) , mPreferIPv4(false) , mPreferIPv6(false) @@ -4010,7 +4035,6 @@ nsHttpConnectionMgr::MoveToWildCardConnEntry(nsHttpConnectionInfo *specificCI, return; } wcEnt->mUsingSpdy = true; - wcEnt->mTestedSpdy = true; LOG(("nsHttpConnectionMgr::MakeConnEntryWildCard ent %p " "idle=%d active=%d half=%d pending=%d\n", ent, diff --git a/netwerk/protocol/http/nsHttpConnectionMgr.h b/netwerk/protocol/http/nsHttpConnectionMgr.h index 368e75094..3729f2b9e 100644 --- a/netwerk/protocol/http/nsHttpConnectionMgr.h +++ b/netwerk/protocol/http/nsHttpConnectionMgr.h @@ -371,12 +371,6 @@ private: // connection is currently using spdy. bool mUsingSpdy : 1; - // mTestedSpdy is set after NPN negotiation has occurred and we know - // with confidence whether a host speaks spdy or not (which is reflected - // in mUsingSpdy). Before mTestedSpdy is set, handshake parallelism is - // minimized so that we can multiplex on a single spdy connection. - bool mTestedSpdy : 1; - bool mInPreferredHash : 1; // Flags to remember our happy-eyeballs decision. @@ -538,7 +532,7 @@ private: nsresult BuildPipeline(nsConnectionEntry *, nsAHttpTransaction *, nsHttpPipeline **); - bool RestrictConnections(nsConnectionEntry *, bool = false); + bool RestrictConnections(nsConnectionEntry *); nsresult ProcessNewTransaction(nsHttpTransaction *); nsresult EnsureSocketThreadTarget(); void ClosePersistentConnections(nsConnectionEntry *ent);