Move SpdySessionRequestMap to SpdySessionPool

net/spdy/chromium/spdy_session_pool.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/spdy/chromium/spdy_session_pool.h"
 
 #include <utility>
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/stl_util.h"
 #include "base/trace_event/memory_allocator_dump.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "base/trace_event/trace_event.h"
 #include "base/values.h"
 #include "net/base/address_list.h"
 #include "net/base/trace_constants.h"
 #include "net/http/http_network_session.h"
 #include "net/http/http_server_properties.h"
 #include "net/log/net_log_event_type.h"
 #include "net/log/net_log_source.h"
 #include "net/log/net_log_with_source.h"
+#include "net/spdy/chromium/bidirectional_stream_spdy_impl.h"
+#include "net/spdy/chromium/spdy_http_stream.h"
 #include "net/spdy/chromium/spdy_session.h"
 #include "net/spdy/core/hpack/hpack_constants.h"
 #include "net/spdy/core/hpack/hpack_huffman_table.h"
 #include "net/spdy/core/hpack/hpack_static_table.h"
 #include "net/spdy/platform/api/spdy_estimate_memory_usage.h"
 #include "net/spdy/platform/api/spdy_string_utils.h"
 
 namespace net {
 
 namespace {
@@ skipping 30 matching lines @@
       time_func_(time_func),
       push_delegate_(nullptr),
       proxy_delegate_(proxy_delegate) {
   NetworkChangeNotifier::AddIPAddressObserver(this);
   if (ssl_config_service_.get())
     ssl_config_service_->AddObserver(this);
   CertDatabase::GetInstance()->AddObserver(this);
 }
 
 SpdySessionPool::~SpdySessionPool() {
+  DCHECK(spdy_session_request_map_.empty());
   CloseAllSessions();
 
   while (!sessions_.empty()) {
     // Destroy sessions to enforce that lifetime is scoped to SpdySessionPool.
     // Write callbacks queued upon session drain are not invoked.
     RemoveUnavailableSession((*sessions_.begin())->GetWeakPtr());
   }
 
   if (ssl_config_service_.get())
     ssl_config_service_->RemoveObserver(this);
@@ skipping 303 matching lines @@
 }
 
 void SpdySessionPool::OnSSLConfigChanged() {
   CloseCurrentSessions(ERR_NETWORK_CHANGED);
 }
 
 void SpdySessionPool::OnCertDBChanged() {
   CloseCurrentSessions(ERR_CERT_DATABASE_CHANGED);
 }
 
+void SpdySessionPool::OnNewSpdySessionReady(
+    const base::WeakPtr<SpdySession>& spdy_session,
+    bool direct,
+    const SSLConfig& used_ssl_config,
+    const ProxyInfo& used_proxy_info,
+    bool was_alpn_negotiated,
+    NextProto negotiated_protocol,
+    bool using_spdy,
+    NetLogSource source_dependency) {
+  while (true) {

[Bence, 2017/06/07 17:41:43]

Optional: while(spdy_session) {

[xunjieli, 2017/06/07 21:55:56]

Done.

+    if (!spdy_session)
+      break;
+    const SpdySessionKey& spdy_session_key = spdy_session->spdy_session_key();
+    // Each iteration may empty out the RequestSet for |spdy_session_key| in
+    // |spdy_session_request_map_|. So each time, check for RequestSet and use
+    // the first one.
+    //
+    // TODO(willchan): If it's important, switch RequestSet out for a FIFO
+    // queue (Order by priority first, then FIFO within same priority). Unclear
+    // that it matters here.
+    if (!base::ContainsKey(spdy_session_request_map_, spdy_session_key))

[Bence, 2017/06/07 17:41:43]

ContainsKey and operator[] below are two lookups, you could cut down on one of
them by:
iterator it = find();
if (it == end)
  return;

[xunjieli, 2017/06/07 21:55:56]

Done.

+      break;

[Bence, 2017/06/07 17:41:43]

Optional: return instead of break might reflect our intention better.

[xunjieli, 2017/06/07 21:55:56]

Done.

+    HttpStreamFactoryImpl::Request* request =
+        *spdy_session_request_map_[spdy_session_key].begin();
+    request->Complete(was_alpn_negotiated, negotiated_protocol, using_spdy);
+    if (request->stream_type() == HttpStreamRequest::BIDIRECTIONAL_STREAM) {
+      request->OnBidirectionalStreamImplReady(
+          used_ssl_config, used_proxy_info,
+          new BidirectionalStreamSpdyImpl(spdy_session, source_dependency));
+    } else {
+      bool use_relative_url =
+          direct || request->url().SchemeIs(url::kHttpsScheme);
+      request->OnStreamReady(used_ssl_config, used_proxy_info,
+                             new SpdyHttpStream(spdy_session, use_relative_url,
+                                                source_dependency));
+    }
+  }
+  // TODO(mbelshe): Alert other valid requests.
+}
+
+void SpdySessionPool::RemoveRequestFromSpdySessionRequestMap(
+    HttpStreamFactoryImpl::Request* request) {
+  if (request->HasSpdySessionKey()) {

[Bence, 2017/06/07 17:41:43]

Optional: early return instead of wrapping entire function body in if.

[xunjieli, 2017/06/07 21:55:56]

Done.

+    const SpdySessionKey& spdy_session_key = request->GetSpdySessionKey();
+    DCHECK(base::ContainsKey(spdy_session_request_map_, spdy_session_key));

[Bence, 2017/06/07 17:41:43]

Optional: do only one lookup instead of two in debug build with find(). 
Optional because debug build performance is not critical.

[xunjieli, 2017/06/07 21:55:56]

Done.

+    RequestSet& request_set = spdy_session_request_map_[spdy_session_key];
+    DCHECK(base::ContainsKey(request_set, request));

[Bence, 2017/06/07 17:41:43]

Optional: do only one lookup instead of two in debug build with find(). 
Optional because debug build performance is not critical.

[xunjieli, 2017/06/07 21:55:56]

Acknowledged.
One lookup is in |request_set| and the other is in |spdy_session_request_map_|.

+    request_set.erase(request);
+    if (request_set.empty())
+      spdy_session_request_map_.erase(spdy_session_key);
+    request->ResetSpdySessionKey();
+  }
+}
+
+void SpdySessionPool::AddRequestToSpdySessionRequestMap(
+    const SpdySessionKey& spdy_session_key,
+    HttpStreamFactoryImpl::Request* request) {
+  if (!request->HasSpdySessionKey()) {

[Bence, 2017/06/07 17:41:43]

Optional: early return instead of wrapping entire function body in if.

[xunjieli, 2017/06/07 21:55:56]

Done.

+    RequestSet& request_set = spdy_session_request_map_[spdy_session_key];
+    DCHECK(!base::ContainsKey(request_set, request));

[Bence, 2017/06/07 17:41:42]

Optional: cut down on lookups by
auto result = insert();
DCHECK(result.second);

[xunjieli, 2017/06/07 21:55:56]

Acknowledged.
One lookup is in |request_set| and the other is in |spdy_session_request_map_|.

+    request_set.insert(request);
+    request->SetSpdySessionKey(spdy_session_key);
+  }
+}
+
 void SpdySessionPool::DumpMemoryStats(
     base::trace_event::ProcessMemoryDump* pmd,
     const SpdyString& parent_dump_absolute_name) const {
   if (sessions_.empty())
     return;
   size_t total_size = 0;
   size_t buffer_size = 0;
   size_t cert_count = 0;
   size_t cert_size = 0;
   size_t num_active_sessions = 0;
@@ skipping 97 matching lines @@
 
     if (idle_only && (*it)->is_active())
       continue;
 
     (*it)->CloseSessionOnError(error, description);
     DCHECK(!IsSessionAvailable(*it));
   }
 }
 
 }  // namespace net