HttpServerProperties - Implement MRU for AlternateProtocolMap. Persist

net/http/http_server_properties_impl.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/http/http_server_properties_impl.h"
 
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "net/http/http_pipelined_host_capability.h"
 
 namespace net {
 
 // TODO(simonjam): Run experiments with different values of this to see what
 // value is good at avoiding evictions without eating too much memory. Until
 // then, this is just a bad guess.
 static const int kDefaultNumHostsToRemember = 200;
 
 HttpServerPropertiesImpl::HttpServerPropertiesImpl()
-    : pipeline_capability_map_(
+    : alternate_protocol_map_(AlternateProtocolMap::NO_AUTO_EVICT),

[Ryan Hamilton, 2014/03/05 17:51:22]

With NO_AUTO_EVICT, this means the in-memory map will be unbounded, right?  (I
think you mentioned that in your mail).  This seems fine for this CL, but just
confirming.

nit: Should we add a test of the MRU behavior of Has|GetAlternateProtocol?

[ramant (doing other things), 2014/03/05 22:43:22]

I went back and forth on the above. The current implementation has no bounds.
Thus made the MRUCache to be unbounded.

Tested the current code with a limit 10 and verified in net-internals that
MRUCache is evicting the right server.

A small change to set the limit: use a different const from
AlternateProtocolMap::NO_AUTO_EVICT. 
Thanks much for the above suggestion. Added the unit tests.

[Ryan Hamilton, 2014/03/05 23:10:20]

SGTM.  We can revisit this at any point.

+      pipeline_capability_map_(
         new CachedPipelineCapabilityMap(kDefaultNumHostsToRemember)),
       weak_ptr_factory_(this) {
   canoncial_suffixes_.push_back(".c.youtube.com");
   canoncial_suffixes_.push_back(".googlevideo.com");
 }
 
 HttpServerPropertiesImpl::~HttpServerPropertiesImpl() {
 }
 
 void HttpServerPropertiesImpl::InitializeSpdyServers(
     std::vector<std::string>* spdy_servers,
     bool support_spdy) {
   DCHECK(CalledOnValidThread());
   spdy_servers_table_.clear();
   if (!spdy_servers)
     return;
   for (std::vector<std::string>::iterator it = spdy_servers->begin();
        it != spdy_servers->end(); ++it) {
     spdy_servers_table_[*it] = support_spdy;
   }
 }
 
 void HttpServerPropertiesImpl::InitializeAlternateProtocolServers(
     AlternateProtocolMap* alternate_protocol_map) {
-  // First swap, and then add back all the ALTERNATE_PROTOCOL_BROKEN ones since
-  // those don't get persisted.
-  alternate_protocol_map_.swap(*alternate_protocol_map);
+  // Keep all the ALTERNATE_PROTOCOL_BROKEN ones since those don't
+  // get persisted.
+  for (AlternateProtocolMap::iterator it = alternate_protocol_map_.begin();
+       it != alternate_protocol_map_.end();) {
+    if (it->second.protocol != ALTERNATE_PROTOCOL_BROKEN) {
+      AlternateProtocolMap::iterator old_it = it;
+      ++it;
+      alternate_protocol_map_.Erase(old_it);
+    } else {
+      ++it;
+    }

[Ryan Hamilton, 2014/03/05 17:51:22]

alternatively:

AlternateProtocolMap::iterator old_it = it;
++it;
if (it->second.protocol != ALTERNATE_PROTOCOL_BROKEN) {
  alternate_protocol_map_.Erase(old_it);
}

It's shorter, but not sure if that's clearer.  What do you think?

[ramant (doing other things), 2014/03/05 22:43:22]

Done.

+  }

[Ryan Hamilton, 2014/03/05 17:51:22]

I take it there is no swap() method in MRUCache?

[ramant (doing other things), 2014/03/05 22:43:22]

Correct.

+
+  // Add the entries from persisted data.
   for (AlternateProtocolMap::const_iterator it =
-       alternate_protocol_map->begin();
+           alternate_protocol_map->begin();
        it != alternate_protocol_map->end(); ++it) {
-    if (it->second.protocol == ALTERNATE_PROTOCOL_BROKEN)
-      alternate_protocol_map_[it->first] = it->second;
+    alternate_protocol_map_.Put(it->first, it->second);
   }
+
   // Attempt to find canonical servers.
   int canonical_ports[] = { 80, 443 };
   for (size_t i = 0; i < canoncial_suffixes_.size(); ++i) {
     std::string canonical_suffix = canoncial_suffixes_[i];
     for (size_t j = 0; j < arraysize(canonical_ports); ++j) {
       HostPortPair canonical_host(canonical_suffix, canonical_ports[j]);
       // If we already have a valid canonical server, we're done.
       if (ContainsKey(canonical_host_to_origin_map_, canonical_host) &&
-          ContainsKey(alternate_protocol_map_,
-                      canonical_host_to_origin_map_[canonical_host])) {
+          (alternate_protocol_map_.Get(canonical_host_to_origin_map_[
+               canonical_host]) != alternate_protocol_map_.end())) {
         continue;
       }
       // Now attempt to find a server which matches this origin and set it as
       // canonical .
       for (AlternateProtocolMap::const_iterator it =
                alternate_protocol_map_.begin();
            it != alternate_protocol_map_.end(); ++it) {
         if (EndsWith(it->first.host(), canoncial_suffixes_[i], false)) {
           canonical_host_to_origin_map_[canonical_host] = it->first;
           break;
@@ skipping 64 matching lines @@
   g_forced_alternate_protocol = NULL;
 }
 
 base::WeakPtr<HttpServerProperties> HttpServerPropertiesImpl::GetWeakPtr() {
   return weak_ptr_factory_.GetWeakPtr();
 }
 
 void HttpServerPropertiesImpl::Clear() {
   DCHECK(CalledOnValidThread());
   spdy_servers_table_.clear();
-  alternate_protocol_map_.clear();
+  alternate_protocol_map_.Clear();
   spdy_settings_map_.clear();
   pipeline_capability_map_->Clear();
 }
 
 bool HttpServerPropertiesImpl::SupportsSpdy(
     const net::HostPortPair& host_port_pair) const {
   DCHECK(CalledOnValidThread());
   if (host_port_pair.host().empty())
     return false;
   std::string spdy_server = GetFlattenedSpdyServer(host_port_pair);
@@ skipping 17 matching lines @@
       spdy_servers_table_.find(spdy_server);
   if ((spdy_host_port != spdy_servers_table_.end()) &&
       (spdy_host_port->second == support_spdy)) {
     return;
   }
   // Cache the data.
   spdy_servers_table_[spdy_server] = support_spdy;
 }
 
 bool HttpServerPropertiesImpl::HasAlternateProtocol(
-    const HostPortPair& server) const {
-  if (ContainsKey(alternate_protocol_map_, server) ||
+    const HostPortPair& server) {
+  if (alternate_protocol_map_.Get(server) != alternate_protocol_map_.end() ||
       g_forced_alternate_protocol)
     return true;
 
   return GetCanonicalHost(server) != canonical_host_to_origin_map_.end();
 }
 
 PortAlternateProtocolPair
 HttpServerPropertiesImpl::GetAlternateProtocol(
-    const HostPortPair& server) const {
+    const HostPortPair& server) {
   DCHECK(HasAlternateProtocol(server));
 
   // First check the map.
-  AlternateProtocolMap::const_iterator it =
-      alternate_protocol_map_.find(server);
+  AlternateProtocolMap::iterator it = alternate_protocol_map_.Get(server);
   if (it != alternate_protocol_map_.end())
     return it->second;
 
   // Next check the canonical host.
   CanonicalHostMap::const_iterator canonical_host = GetCanonicalHost(server);
   if (canonical_host != canonical_host_to_origin_map_.end())
-    return alternate_protocol_map_.find(canonical_host->second)->second;
+    return alternate_protocol_map_.Get(canonical_host->second)->second;
 
   // We must be forcing an alternate.
   DCHECK(g_forced_alternate_protocol);
   return *g_forced_alternate_protocol;
 }
 
 void HttpServerPropertiesImpl::SetAlternateProtocol(
     const HostPortPair& server,
     uint16 alternate_port,
     AlternateProtocol alternate_protocol) {
@@ skipping 19 matching lines @@
       LOG(WARNING) << "Changing the alternate protocol for: "
                    << server.ToString()
                    << " from [Port: " << existing_alternate.port
                    << ", Protocol: " << existing_alternate.protocol
                    << "] to [Port: " << alternate_port
                    << ", Protocol: " << alternate_protocol
                    << "].";
     }
   }
 
-  alternate_protocol_map_[server] = alternate;
+  alternate_protocol_map_.Put(server, alternate);
 
   // If this host ends with a canonical suffix, then set it as the
   // canonical host.
   for (size_t i = 0; i < canoncial_suffixes_.size(); ++i) {
     std::string canonical_suffix = canoncial_suffixes_[i];
     if (EndsWith(server.host(), canoncial_suffixes_[i], false)) {
       HostPortPair canonical_host(canonical_suffix, server.port());
       canonical_host_to_origin_map_[canonical_host] = server;
       break;
     }
   }
 }
 
 void HttpServerPropertiesImpl::SetBrokenAlternateProtocol(
     const HostPortPair& server) {
-  alternate_protocol_map_[server].protocol = ALTERNATE_PROTOCOL_BROKEN;
+  AlternateProtocolMap::iterator it = alternate_protocol_map_.Get(server);
+  if (it != alternate_protocol_map_.end()) {
+    it->second.protocol = ALTERNATE_PROTOCOL_BROKEN;
+    return;
+  }
+  PortAlternateProtocolPair alternate;
+  alternate.protocol = ALTERNATE_PROTOCOL_BROKEN;
+  alternate_protocol_map_.Put(server, alternate);
 }
 
 const AlternateProtocolMap&
 HttpServerPropertiesImpl::alternate_protocol_map() const {
   return alternate_protocol_map_;
 }
 
 const SettingsMap& HttpServerPropertiesImpl::GetSpdySettings(
     const HostPortPair& host_port_pair) const {
   SpdySettingsMap::const_iterator it = spdy_settings_map_.find(host_port_pair);
@@ skipping 93 matching lines @@
     if (EndsWith(server.host(), canoncial_suffixes_[i], false)) {
       HostPortPair canonical_host(canonical_suffix, server.port());
       return canonical_host_to_origin_map_.find(canonical_host);
     }
   }
 
   return canonical_host_to_origin_map_.end();
 }
 
 }  // namespace net