Commit 9338c379 authored by Matt Menke's avatar Matt Menke Committed by Commit Bot

SpdySessionPool: Invoke request deleted callbacks more discriminately.

Previously, we'd invoke them whenever any request was deleted or when a
SpdySession is created. With this CL, we invoke them only when the
first request is deleted or a SpdySession is created. This reduces the
number of PostTasks and searches through std::maps when we have a bunch
of requests all waiting on the creation of the same SpdySession.

Bug: 912727
Change-Id: I5083137cf0726e3b1ddb3ac4608b706121964cbf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1570914
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: default avatarBence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652597}
parent 2c90c99c
......@@ -837,13 +837,13 @@ int HttpStreamFactory::Job::DoInitConnectionImpl() {
ptr_factory_.GetWeakPtr())
: base::RepeatingClosure();
bool is_first_request_for_session;
bool is_blocking_request_for_session;
existing_spdy_session_ = session_->spdy_session_pool()->RequestSession(
spdy_session_key_, enable_ip_based_pooling_,
try_websocket_over_http2_, net_log_, resume_callback, this,
&spdy_session_request_, &is_first_request_for_session);
&spdy_session_request_, &is_blocking_request_for_session);
if (!existing_spdy_session_ && should_throttle_connect &&
!is_first_request_for_session) {
!is_blocking_request_for_session) {
net_log_.AddEvent(NetLogEventType::HTTP_STREAM_JOB_THROTTLED);
next_state_ = STATE_INIT_CONNECTION;
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
......
......@@ -56,11 +56,13 @@ SpdySessionPool::SpdySessionRequest::SpdySessionRequest(
const SpdySessionKey& key,
bool enable_ip_based_pooling,
bool is_websocket,
bool is_blocking_request_for_session,
Delegate* delegate,
SpdySessionPool* spdy_session_pool)
: key_(key),
enable_ip_based_pooling_(enable_ip_based_pooling),
is_websocket_(is_websocket),
is_blocking_request_for_session_(is_blocking_request_for_session),
delegate_(delegate),
spdy_session_pool_(spdy_session_pool) {}
......@@ -318,10 +320,10 @@ base::WeakPtr<SpdySession> SpdySessionPool::RequestSession(
bool enable_ip_based_pooling,
bool is_websocket,
const NetLogWithSource& net_log,
base::RepeatingClosure on_request_destroyed_callback,
base::RepeatingClosure on_blocking_request_destroyed_callback,
SpdySessionRequest::Delegate* delegate,
std::unique_ptr<SpdySessionRequest>* spdy_session_request,
bool* is_first_request_for_session) {
bool* is_blocking_request_for_session) {
DCHECK(delegate);
base::WeakPtr<SpdySession> spdy_session =
......@@ -329,19 +331,22 @@ base::WeakPtr<SpdySession> SpdySessionPool::RequestSession(
if (spdy_session) {
// This value doesn't really matter, but best to always populate it, for
// consistency.
*is_first_request_for_session = true;
*is_blocking_request_for_session = true;
return spdy_session;
}
RequestSet* request_set = &spdy_session_request_map_[key];
*is_first_request_for_session = request_set->empty();
RequestInfoForKey* request_info = &spdy_session_request_map_[key];
*is_blocking_request_for_session = !request_info->has_blocking_request;
*spdy_session_request = std::make_unique<SpdySessionRequest>(
key, enable_ip_based_pooling, is_websocket, delegate, this);
request_set->insert(spdy_session_request->get());
if (on_request_destroyed_callback && !*is_first_request_for_session) {
spdy_session_pending_request_map_[key].push_back(
on_request_destroyed_callback);
key, enable_ip_based_pooling, is_websocket,
*is_blocking_request_for_session, delegate, this);
request_info->request_set.insert(spdy_session_request->get());
if (*is_blocking_request_for_session) {
request_info->has_blocking_request = true;
} else if (on_blocking_request_destroyed_callback) {
request_info->deferred_callbacks.push_back(
on_blocking_request_destroyed_callback);
}
return nullptr;
}
......@@ -446,21 +451,21 @@ void SpdySessionPool::OnCertDBChanged() {
void SpdySessionPool::RemoveRequestForSpdySession(SpdySessionRequest* request) {
DCHECK_EQ(this, request->spdy_session_pool());
const SpdySessionKey& spdy_session_key = request->key();
// Resume all pending requests now that |request| is done/canceled.
if (spdy_session_pending_request_map_.find(spdy_session_key) !=
spdy_session_pending_request_map_.end()) {
auto iter = spdy_session_request_map_.find(request->key());
DCHECK(iter != spdy_session_request_map_.end());
// Resume all pending requests if it is the blocking request, which is either
// being canceled, or has completed.
if (request->is_blocking_request_for_session() &&
!iter->second.deferred_callbacks.empty()) {
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE,
base::BindOnce(&SpdySessionPool::UpdatePendingRequests,
weak_ptr_factory_.GetWeakPtr(), spdy_session_key));
weak_ptr_factory_.GetWeakPtr(), request->key()));
}
auto iter = spdy_session_request_map_.find(spdy_session_key);
DCHECK(iter != spdy_session_request_map_.end());
RequestSet& request_set = iter->second;
DCHECK(base::ContainsKey(request_set, request));
RemoveRequestInternal(iter, request_set.find(request));
DCHECK(base::ContainsKey(iter->second.request_set, request));
RemoveRequestInternal(iter, iter->second.request_set.find(request));
}
void SpdySessionPool::DumpMemoryStats(
......@@ -510,6 +515,9 @@ void SpdySessionPool::DumpMemoryStats(
cert_size);
}
SpdySessionPool::RequestInfoForKey::RequestInfoForKey() = default;
SpdySessionPool::RequestInfoForKey::~RequestInfoForKey() = default;
bool SpdySessionPool::IsSessionAvailable(
const base::WeakPtr<SpdySession>& session) const {
for (auto it = available_sessions_.begin(); it != available_sessions_.end();
......@@ -648,7 +656,7 @@ void SpdySessionPool::UpdatePendingRequests(const SpdySessionKey& key) {
auto iter = spdy_session_request_map_.find(key);
if (iter == spdy_session_request_map_.end())
break;
RequestSet* request_set = &iter->second;
RequestSet* request_set = &iter->second.request_set;
// Find a request that can use the socket, if any.
RequestSet::iterator request;
for (request = request_set->begin(); request != request_set->end();
......@@ -671,19 +679,24 @@ void SpdySessionPool::UpdatePendingRequests(const SpdySessionKey& key) {
}
}
auto iter = spdy_session_pending_request_map_.find(key);
if (iter == spdy_session_pending_request_map_.end())
auto iter = spdy_session_request_map_.find(key);
if (iter == spdy_session_request_map_.end())
return;
// Remove all pending requests, if there are any. As a result, if one of these
// callbacks triggers a new RequestSession() call,
// |is_first_request_for_session| will be true.
std::list<base::RepeatingClosure> pending_requests = std::move(iter->second);
spdy_session_pending_request_map_.erase(iter);
// |is_blocking_request_for_session| will be true.
std::list<base::RepeatingClosure> deferred_requests =
std::move(iter->second.deferred_callbacks);
// Resume any pending requests. This needs to be after the
// Delete the RequestMap if there are no SpdySessionRequests, and no deferred
// requests.
if (iter->second.request_set.empty())
spdy_session_request_map_.erase(iter);
// Resume any deferred requests. This needs to be after the
// OnSpdySessionAvailable() calls, to prevent requests from calling into the
// socket pools in cases where that's not necessary.
for (auto callback : pending_requests) {
for (auto callback : deferred_requests) {
callback.Run();
}
}
......@@ -692,9 +705,17 @@ void SpdySessionPool::RemoveRequestInternal(
SpdySessionRequestMap::iterator request_map_iterator,
RequestSet::iterator request_set_iterator) {
SpdySessionRequest* request = *request_set_iterator;
request_map_iterator->second.erase(request_set_iterator);
if (request_map_iterator->second.empty())
request_map_iterator->second.request_set.erase(request_set_iterator);
if (request->is_blocking_request_for_session()) {
DCHECK(request_map_iterator->second.has_blocking_request);
request_map_iterator->second.has_blocking_request = false;
}
// If both lists of requests are empty, can now remove the entry from the map.
if (request_map_iterator->second.request_set.empty() &&
request_map_iterator->second.deferred_callbacks.empty()) {
spdy_session_request_map_.erase(request_map_iterator);
}
request->OnRemovedFromPool();
}
......
......@@ -81,11 +81,11 @@ class NET_EXPORT SpdySessionPool
// request must be destroyed before the SpdySessionPool is.
//
// TODO(mmenke): Remove the dependency on OnNewSpdySessionReady.
class SpdySessionRequest {
class NET_EXPORT_PRIVATE SpdySessionRequest {
public:
// Interface for watching for when a SpdySession with a provided key is
// created.
class Delegate {
class NET_EXPORT_PRIVATE Delegate {
public:
Delegate();
virtual ~Delegate();
......@@ -102,6 +102,7 @@ class NET_EXPORT SpdySessionPool
SpdySessionRequest(const SpdySessionKey& key,
bool enable_ip_based_pooling,
bool is_websocket,
bool is_blocking_request_for_session,
Delegate* delegate,
SpdySessionPool* spdy_session_pool);
......@@ -114,6 +115,9 @@ class NET_EXPORT SpdySessionPool
const SpdySessionKey& key() const { return key_; }
bool enable_ip_based_pooling() const { return enable_ip_based_pooling_; }
bool is_websocket() const { return is_websocket_; }
bool is_blocking_request_for_session() const {
return is_blocking_request_for_session_;
}
Delegate* delegate() { return delegate_; }
// The associated SpdySessionPool, or nullptr if OnRemovedFromPool() has
......@@ -124,6 +128,7 @@ class NET_EXPORT SpdySessionPool
const SpdySessionKey key_;
const bool enable_ip_based_pooling_;
const bool is_websocket_;
const bool is_blocking_request_for_session_;
Delegate* const delegate_;
SpdySessionPool* spdy_session_pool_;
......@@ -201,28 +206,33 @@ class NET_EXPORT SpdySessionPool
// available through the creation of a new SpdySession (as opposed to by
// creating an alias for an existing session with a new host).
//
// |is_first_request_for_session| will be set to |true| if this is the first
// request for the session. If |on_request_destroyed_callback| is non-null and
// there is already at least one pending request for the session (i.e.,
// |is_first_request_for_session| is set to false), it will be invoked
// asynchronously whenever any matching |spdy_session_request| is destroyed or
// a matching SpdySession is created.
// |is_blocking_request_for_session| will be set to |true| if there is not
// another "blocking" request already pending. For example, the first request
// created will be considered "blocking", but subsequent requests will not as
// long as the "blocking" request is not destroyed. Once the "blocking"
// request is destroyed, the next created request will be marked "blocking".
//
// |delegate|, |spdy_session_request|, and |is_first_request_for_session| must
// all be non-null.
// If a request is created, that request is not the "blocking" request, and
// |on_blocking_request_destroyed_callback| is non-null,
// then |on_blocking_request_destroyed_callback| will be invoked
// asynchronously when the "blocking" request is destroyed. The callback
// associated with the "blocking" request is never invoked.
//
// |delegate|, |spdy_session_request|, and |is_blocking_request_for_session|
// must all be non-null.
//
// TODO(mmenke): Merge this into FindAvailableSession().
// TODO(mmenke): Don't invoke |on_request_destroyed_callback| when all
// requests for a session have been successfully responded to.
// TODO(mmenke): Don't invoke |on_blocking_request_destroyed_callback| when
// all requests for a session have been successfully responded to.
base::WeakPtr<SpdySession> RequestSession(
const SpdySessionKey& key,
bool enable_ip_based_pooling,
bool is_websocket,
const NetLogWithSource& net_log,
base::RepeatingClosure on_request_destroyed_callback,
base::RepeatingClosure on_blocking_request_destroyed_callback,
SpdySessionRequest::Delegate* delegate,
std::unique_ptr<SpdySessionRequest>* spdy_session_request,
bool* is_first_request_for_session);
bool* is_blocking_request_for_session);
// Remove all mappings and aliases for the given session, which must
// still be available. Except for in tests, this must be called by
......@@ -296,14 +306,29 @@ class NET_EXPORT SpdySessionPool
private:
friend class SpdySessionPoolPeer; // For testing.
typedef std::set<SpdySessionRequest*> RequestSet;
typedef std::map<SpdySessionKey, RequestSet> SpdySessionRequestMap;
typedef std::set<SpdySession*> SessionSet;
typedef std::vector<base::WeakPtr<SpdySession> > WeakSessionList;
typedef std::map<SpdySessionKey, base::WeakPtr<SpdySession> >
typedef std::map<SpdySessionKey, base::WeakPtr<SpdySession>>
AvailableSessionMap;
typedef std::multimap<IPEndPoint, SpdySessionKey> AliasMap;
typedef std::set<SpdySessionRequest*> RequestSet;
struct RequestInfoForKey {
RequestInfoForKey();
~RequestInfoForKey();
// Whether one of the requests in |RequestSet| has its
// is_blocking_request_for_session() bit set.
bool has_blocking_request = false;
RequestSet request_set;
// Set of callbacks watching for the blocking request to be destroyed.
std::list<base::RepeatingClosure> deferred_callbacks;
};
typedef std::map<SpdySessionKey, RequestInfoForKey> SpdySessionRequestMap;
// Removes |request| from |spdy_session_request_map_|.
void RemoveRequestForSpdySession(SpdySessionRequest* request);
......@@ -410,11 +435,7 @@ class NET_EXPORT SpdySessionPool
// https://tools.ietf.org/html/draft-bishop-httpbis-grease-00.
const base::Optional<GreasedHttp2Frame> greased_http2_frame_;
// TODO(xunjieli): Merge these two.
SpdySessionRequestMap spdy_session_request_map_;
typedef std::map<SpdySessionKey, std::list<base::RepeatingClosure>>
SpdySessionPendingRequestMap;
SpdySessionPendingRequestMap spdy_session_pending_request_map_;
TimeFunc time_func_;
ServerPushDelegate* push_delegate_;
......
......@@ -7,9 +7,11 @@
#include <cstddef>
#include <utility>
#include "base/bind.h"
#include "base/memory/ref_counted.h"
#include "base/run_loop.h"
#include "base/stl_util.h"
#include "base/test/bind_test_util.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/trace_event/memory_allocator_dump.h"
#include "base/trace_event/process_memory_dump.h"
......@@ -1146,4 +1148,195 @@ TEST_F(SpdySessionPoolTest, FindAvailableSessionForWebSocket) {
EXPECT_TRUE(data.AllReadDataConsumed());
EXPECT_TRUE(data.AllWriteDataConsumed());
}
class TestOnRequestDeletedCallback {
public:
TestOnRequestDeletedCallback() = default;
~TestOnRequestDeletedCallback() = default;
base::RepeatingClosure Callback() {
return base::BindRepeating(&TestOnRequestDeletedCallback::OnRequestDeleted,
base::Unretained(this));
}
bool invoked() const { return invoked_; }
void WaitUntilInvoked() { run_loop_.Run(); }
void SetRequestDeletedCallback(base::OnceClosure request_deleted_callback) {
DCHECK(!request_deleted_callback_);
request_deleted_callback_ = std::move(request_deleted_callback);
}
private:
void OnRequestDeleted() {
EXPECT_FALSE(invoked_);
invoked_ = true;
if (request_deleted_callback_)
std::move(request_deleted_callback_).Run();
run_loop_.Quit();
}
bool invoked_ = false;
base::RunLoop run_loop_;
base::OnceClosure request_deleted_callback_;
DISALLOW_COPY_AND_ASSIGN(TestOnRequestDeletedCallback);
};
class TestRequestDelegate
: public SpdySessionPool::SpdySessionRequest::Delegate {
public:
TestRequestDelegate() = default;
~TestRequestDelegate() override = default;
// SpdySessionPool::SpdySessionRequest::Delegate implementation:
void OnSpdySessionAvailable(
base::WeakPtr<SpdySession> spdy_session) override {}
private:
DISALLOW_COPY_AND_ASSIGN(TestRequestDelegate);
};
TEST_F(SpdySessionPoolTest, RequestSessionWithNoSessions) {
const SpdySessionKey kSessionKey(HostPortPair("foo.test", 443),
ProxyServer::Direct(), PRIVACY_MODE_DISABLED,
SpdySessionKey::IsProxySession::kFalse,
SocketTag());
CreateNetworkSession();
// First request. Its request deleted callback should never be invoked.
TestOnRequestDeletedCallback request_deleted_callback1;
TestRequestDelegate request_delegate1;
std::unique_ptr<SpdySessionPool::SpdySessionRequest> spdy_session_request1;
bool is_first_request_for_session;
EXPECT_FALSE(spdy_session_pool_->RequestSession(
kSessionKey, /* enable_ip_based_pooling = */ false,
/* is_websocket = */ false, NetLogWithSource(),
request_deleted_callback1.Callback(), &request_delegate1,
&spdy_session_request1, &is_first_request_for_session));
EXPECT_TRUE(is_first_request_for_session);
// Second request.
TestOnRequestDeletedCallback request_deleted_callback2;
TestRequestDelegate request_delegate2;
std::unique_ptr<SpdySessionPool::SpdySessionRequest> spdy_session_request2;
EXPECT_FALSE(spdy_session_pool_->RequestSession(
kSessionKey, /* enable_ip_based_pooling = */ false,
/* is_websocket = */ false, NetLogWithSource(),
request_deleted_callback2.Callback(), &request_delegate2,
&spdy_session_request2, &is_first_request_for_session));
EXPECT_FALSE(is_first_request_for_session);
// Third request.
TestOnRequestDeletedCallback request_deleted_callback3;
TestRequestDelegate request_delegate3;
std::unique_ptr<SpdySessionPool::SpdySessionRequest> spdy_session_request3;
EXPECT_FALSE(spdy_session_pool_->RequestSession(
kSessionKey, /* enable_ip_based_pooling = */ false,
/* is_websocket = */ false, NetLogWithSource(),
request_deleted_callback3.Callback(), &request_delegate3,
&spdy_session_request3, &is_first_request_for_session));
EXPECT_FALSE(is_first_request_for_session);
// Destroying the second request shouldn't cause anything to happen.
spdy_session_request2.reset();
base::RunLoop().RunUntilIdle();
EXPECT_FALSE(request_deleted_callback1.invoked());
EXPECT_FALSE(request_deleted_callback2.invoked());
EXPECT_FALSE(request_deleted_callback3.invoked());
// But destroying the first request should cause the second and third
// callbacks to be invoked.
spdy_session_request1.reset();
request_deleted_callback2.WaitUntilInvoked();
request_deleted_callback3.WaitUntilInvoked();
EXPECT_FALSE(request_deleted_callback1.invoked());
// Nothing should happen when the third request is destroyed.
spdy_session_request3.reset();
base::RunLoop().RunUntilIdle();
EXPECT_FALSE(request_deleted_callback1.invoked());
}
TEST_F(SpdySessionPoolTest, RequestSessionDuringNotification) {
const SpdySessionKey kSessionKey(HostPortPair("foo.test", 443),
ProxyServer::Direct(), PRIVACY_MODE_DISABLED,
SpdySessionKey::IsProxySession::kFalse,
SocketTag());
CreateNetworkSession();
// First request. Its request deleted callback should never be invoked.
TestOnRequestDeletedCallback request_deleted_callback1;
TestRequestDelegate request_delegate1;
std::unique_ptr<SpdySessionPool::SpdySessionRequest> spdy_session_request1;
bool is_first_request_for_session;
EXPECT_FALSE(spdy_session_pool_->RequestSession(
kSessionKey, /* enable_ip_based_pooling = */ false,
/* is_websocket = */ false, NetLogWithSource(),
request_deleted_callback1.Callback(), &request_delegate1,
&spdy_session_request1, &is_first_request_for_session));
EXPECT_TRUE(is_first_request_for_session);
// Second request.
TestOnRequestDeletedCallback request_deleted_callback2;
TestRequestDelegate request_delegate2;
std::unique_ptr<SpdySessionPool::SpdySessionRequest> spdy_session_request2;
EXPECT_FALSE(spdy_session_pool_->RequestSession(
kSessionKey, /* enable_ip_based_pooling = */ false,
/* is_websocket = */ false, NetLogWithSource(),
request_deleted_callback2.Callback(), &request_delegate2,
&spdy_session_request2, &is_first_request_for_session));
EXPECT_FALSE(is_first_request_for_session);
TestOnRequestDeletedCallback request_deleted_callback3;
TestRequestDelegate request_delegate3;
std::unique_ptr<SpdySessionPool::SpdySessionRequest> spdy_session_request3;
TestOnRequestDeletedCallback request_deleted_callback4;
TestRequestDelegate request_delegate4;
std::unique_ptr<SpdySessionPool::SpdySessionRequest> spdy_session_request4;
request_deleted_callback2.SetRequestDeletedCallback(
base::BindLambdaForTesting([&]() {
// Third request. It should again be marked as the first request for the
// session, since it's only created after the original two have been
// removed.
bool is_first_request_for_session;
EXPECT_FALSE(spdy_session_pool_->RequestSession(
kSessionKey, /* enable_ip_based_pooling = */ false,
/* is_websocket = */ false, NetLogWithSource(),
request_deleted_callback3.Callback(), &request_delegate3,
&spdy_session_request3, &is_first_request_for_session));
EXPECT_TRUE(is_first_request_for_session);
// Fourth request.
EXPECT_FALSE(spdy_session_pool_->RequestSession(
kSessionKey, /* enable_ip_based_pooling = */ false,
/* is_websocket = */ false, NetLogWithSource(),
request_deleted_callback4.Callback(), &request_delegate4,
&spdy_session_request4, &is_first_request_for_session));
EXPECT_FALSE(is_first_request_for_session);
}));
// Destroying the first request should cause the second callback to be
// invoked, and the third and fourth request to be made.
spdy_session_request1.reset();
request_deleted_callback2.WaitUntilInvoked();
base::RunLoop().RunUntilIdle();
EXPECT_FALSE(request_deleted_callback1.invoked());
EXPECT_FALSE(request_deleted_callback3.invoked());
EXPECT_FALSE(request_deleted_callback4.invoked());
EXPECT_TRUE(spdy_session_request3);
EXPECT_TRUE(spdy_session_request4);
// Destroying the third request should cause the fourth callback to be
// invoked.
spdy_session_request3.reset();
request_deleted_callback4.WaitUntilInvoked();
EXPECT_FALSE(request_deleted_callback1.invoked());
EXPECT_FALSE(request_deleted_callback3.invoked());
}
} // namespace net
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment