Change HTTP Auth Cache from a list to a multimap

Previously, the HTTP Auth Cache was implemented as a list of auth
entries, which was searched linearly for a matching entry upon lookup.
This change converts the cache to a multimap keyed on the origin, which
is expected to provide faster lookups in cases where there are no
matching entries for a given origin (which is the majority of cases).
This will also allow for a higher cache size limit without a linear
increase in lookup time, hence the cache size limit is raised from 10 to
20. This is expected to lower the rate of evictions upon adding entries.
Obsolete histograms which no longer apply to the new implementation are
also removed.

Bug: 757116, 614108
Change-Id: Iabd2104b2d1c38d474450978a712039819b038e6
Reviewed-on: https://chromium-review.googlesource.com/c/1273820
Commit-Queue: Lily Chen <chlily@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599362}

net/http/http_auth_cache.cc

@@ -60,19 +60,6 @@ struct IsEnclosedBy {
   const std::string& path;
 };
 
-void RecordLookupPosition(int position) {
-  UMA_HISTOGRAM_COUNTS_100("Net.HttpAuthCacheLookupPosition", position);
-}
-
-void RecordLookupByPathPosition(int position) {
-  UMA_HISTOGRAM_COUNTS_100("Net.HttpAuthCacheLookupByPathPosition", position);
-}
-
-void RecordEntriesExaminedWhenNoMatch(int num_examined_entries) {
-  UMA_HISTOGRAM_COUNTS_100("Net.HttpAuthCacheEntriesExaminedWhenNoMatch",
-                           num_examined_entries);
-}
-
 }  // namespace
 
 namespace net {
@@ -81,36 +68,23 @@ HttpAuthCache::HttpAuthCache() = default;
 
 HttpAuthCache::~HttpAuthCache() = default;
 
-// Performance: O(n), where n is the number of realm entries.
+// Performance: O(logN+n), where N is the total number of entries, n is the
+// number of realm entries for the given origin.
 HttpAuthCache::Entry* HttpAuthCache::Lookup(const GURL& origin,
                                             const std::string& realm,
                                             HttpAuth::Scheme scheme) {
-  CheckOriginIsValid(origin);
-
-  int entries_examined = 0;
-  // Linear scan through the realm entries.
-  for (auto it = entries_.begin(); it != entries_.end(); ++it) {
-    ++entries_examined;
-    if (it->origin() == origin && it->realm() == realm &&
-        it->scheme() == scheme) {
-      it->last_use_time_ticks_ = tick_clock_->NowTicks();
-      RecordLookupPosition(entries_examined);
-      return MoveEntryTowardsBeginning(it);
-    }
-  }
-  RecordLookupPosition(0);
-  RecordEntriesExaminedWhenNoMatch(entries_examined);
-  return nullptr;  // No realm entry found.
+  EntryMap::iterator entry_it = LookupEntryIt(origin, realm, scheme);
+  if (entry_it == entries_.end())
+    return nullptr;
+  return &(entry_it->second);
 }
 
-// Performance: O(n*m), where n is the number of realm entries, m is the number
-// of path entries per realm. Both n amd m are expected to be small; m is
-// kept small because AddPath() only keeps the shallowest entry.
+// Performance: O(logN+n*m), where N is the total number of entries, n is the
+// number of realm entries for the given origin, m is the number of path entries
+// per realm. Both n amd m are expected to be small; m is kept small because
+// AddPath() only keeps the shallowest entry.
 HttpAuthCache::Entry* HttpAuthCache::LookupByPath(const GURL& origin,
                                                   const std::string& path) {
-  auto best_match_it = entries_.end();
-  size_t best_match_length = 0;
-  int best_match_position = 0;
   CheckOriginIsValid(origin);
   CheckPathIsValid(path);
 
@@ -120,24 +94,25 @@ HttpAuthCache::Entry* HttpAuthCache::LookupByPath(const GURL& origin,
   // within the protection space ...
   std::string parent_dir = GetParentDirectory(path);
 
-  int entries_examined = 0;
-  // Linear scan through the realm entries.
-  for (auto it = entries_.begin(); it != entries_.end(); ++it) {
-    ++entries_examined;
+  // Linear scan through the <scheme, realm> entries for the given origin.
+  auto entry_range = entries_.equal_range(origin);
+  auto best_match_it = entries_.end();
+  size_t best_match_length = 0;
+  for (auto it = entry_range.first; it != entry_range.second; ++it) {
     size_t len = 0;
-    if (it->origin() == origin && it->HasEnclosingPath(parent_dir, &len) &&
+    auto& entry = it->second;
+    DCHECK(entry.origin() == origin);
+    if (entry.HasEnclosingPath(parent_dir, &len) &&
         (best_match_it == entries_.end() || len > best_match_length)) {
       best_match_it = it;
       best_match_length = len;
-      best_match_position = entries_examined;
     }
   }
-  RecordLookupByPathPosition(best_match_position);
   if (best_match_it != entries_.end()) {
-    best_match_it->last_use_time_ticks_ = tick_clock_->NowTicks();
-    return MoveEntryTowardsBeginning(best_match_it);
+    Entry& best_match_entry = best_match_it->second;
+    best_match_entry.last_use_time_ticks_ = tick_clock_->NowTicks();
+    return &best_match_entry;
   }
-  RecordEntriesExaminedWhenNoMatch(entries_examined);
   return nullptr;
 }
 
@@ -159,19 +134,12 @@ HttpAuthCache::Entry* HttpAuthCache::Add(const GURL& origin,
     // Failsafe to prevent unbounded memory growth of the cache.
     if (entries_.size() >= kMaxNumRealmEntries) {
       LOG(WARNING) << "Num auth cache entries reached limit -- evicting";
-      UMA_HISTOGRAM_LONG_TIMES(
-          "Net.HttpAuthCacheAddEvictedCreation",
-          now_ticks - entries_.back().creation_time_ticks_);
-      UMA_HISTOGRAM_LONG_TIMES(
-          "Net.HttpAuthCacheAddEvictedLastUse",
-          now_ticks - entries_.back().last_use_time_ticks_);
-      entries_.pop_back();
+      EvictLeastRecentlyUsedEntry();
       evicted = true;
     }
     UMA_HISTOGRAM_BOOLEAN("Net.HttpAuthCacheAddEvicted", evicted);
 
-    entries_.push_front(Entry());
-    entry = &entries_.front();
+    entry = &(entries_.emplace(std::make_pair(origin, Entry()))->second);
     entry->origin_ = origin;
     entry->realm_ = realm;
     entry->scheme_ = scheme;
@@ -270,15 +238,13 @@ bool HttpAuthCache::Remove(const GURL& origin,
                            const std::string& realm,
                            HttpAuth::Scheme scheme,
                            const AuthCredentials& credentials) {
-  for (auto it = entries_.begin(); it != entries_.end(); ++it) {
-    if (it->origin() == origin && it->realm() == realm &&
-        it->scheme() == scheme) {
-      if (credentials.Equals(it->credentials())) {
-        entries_.erase(it);
-        return true;
-      }
-      return false;
-    }
+  EntryMap::iterator entry_it = LookupEntryIt(origin, realm, scheme);
+  if (entry_it == entries_.end())
+    return false;
+  Entry& entry = entry_it->second;
+  if (credentials.Equals(entry.credentials())) {
+    entries_.erase(entry_it);
+    return true;
   }
   return false;
 }
@@ -287,7 +253,8 @@ void HttpAuthCache::ClearEntriesAddedSince(base::Time begin_time) {
   if (begin_time.is_null()) {
     ClearAllEntries();
   } else {
-    base::EraseIf(entries_, [begin_time](const Entry& entry) {
+    base::EraseIf(entries_, [begin_time](EntryMap::value_type& entry_map_pair) {
+      Entry& entry = entry_map_pair.second;
       return entry.creation_time_ >= begin_time;
     });
   }
@@ -312,15 +279,15 @@ bool HttpAuthCache::UpdateStaleChallenge(const GURL& origin,
 void HttpAuthCache::UpdateAllFrom(const HttpAuthCache& other) {
   for (auto it = other.entries_.begin(); it != other.entries_.end(); ++it) {
     // Add an Entry with one of the original entry's paths.
-    DCHECK(it->paths_.size() > 0);
-    Entry* entry = Add(it->origin(), it->realm(), it->scheme(),
-                       it->auth_challenge(), it->credentials(),
-                       it->paths_.back());
+    const Entry& e = it->second;
+    DCHECK(e.paths_.size() > 0);
+    Entry* entry = Add(e.origin(), e.realm(), e.scheme(), e.auth_challenge(),
+                       e.credentials(), e.paths_.back());
     // Copy all other paths.
-    for (auto it2 = ++it->paths_.rbegin(); it2 != it->paths_.rend(); ++it2)
+    for (auto it2 = std::next(e.paths_.rbegin()); it2 != e.paths_.rend(); ++it2)
       entry->AddPath(*it2);
     // Copy nonce count (for digest authentication).
-    entry->nonce_count_ = it->nonce_count_;
+    entry->nonce_count_ = e.nonce_count_;
   }
 }
 
@@ -328,13 +295,49 @@ size_t HttpAuthCache::GetEntriesSizeForTesting() {
   return entries_.size();
 }
 
-HttpAuthCache::Entry* HttpAuthCache::MoveEntryTowardsBeginning(
-    EntryList::iterator entry_it) {
-  if (entry_it != entries_.begin()) {
-    std::iter_swap(entry_it, std::prev(entry_it));
-    return &(*std::prev(entry_it));
+HttpAuthCache::EntryMap::iterator HttpAuthCache::LookupEntryIt(
+    const GURL& origin,
+    const std::string& realm,
+    HttpAuth::Scheme scheme) {
+  CheckOriginIsValid(origin);
+
+  // Linear scan through the <scheme, realm> entries for the given origin.
+  auto entry_range = entries_.equal_range(origin);
+  for (auto it = entry_range.first; it != entry_range.second; ++it) {
+    Entry& entry = it->second;
+    DCHECK(entry.origin() == origin);
+    if (entry.scheme() == scheme && entry.realm() == realm) {
+      entry.last_use_time_ticks_ = tick_clock_->NowTicks();
+      return it;
+    }
+  }
+  return entries_.end();
+}
+
+// Linear scan through all entries to find least recently used entry (by oldest
+// |last_use_time_ticks_| and evict it from |entries_|.
+void HttpAuthCache::EvictLeastRecentlyUsedEntry() {
+  DCHECK(entries_.size() == kMaxNumRealmEntries);
+  base::TimeTicks now_ticks = tick_clock_->NowTicks();
+
+  EntryMap::iterator oldest_entry_it = entries_.end();
+  base::TimeTicks oldest_last_use_time_ticks = now_ticks;
+
+  for (auto it = entries_.begin(); it != entries_.end(); ++it) {
+    Entry& entry = it->second;
+    if (entry.last_use_time_ticks_ < oldest_last_use_time_ticks ||
+        oldest_entry_it == entries_.end()) {
+      oldest_entry_it = it;
+      oldest_last_use_time_ticks = entry.last_use_time_ticks_;
+    }
   }
-  return &(*entry_it);
+  DCHECK(oldest_entry_it != entries_.end());
+  Entry& oldest_entry = oldest_entry_it->second;
+  UMA_HISTOGRAM_LONG_TIMES("Net.HttpAuthCacheAddEvictedCreation",
+                           now_ticks - oldest_entry.creation_time_ticks_);
+  UMA_HISTOGRAM_LONG_TIMES("Net.HttpAuthCacheAddEvictedLastUse",
+                           now_ticks - oldest_entry.last_use_time_ticks_);
+  entries_.erase(oldest_entry_it);
 }
 
 }  // namespace net

net/http/http_auth_cache.h

@@ -8,6 +8,7 @@
 #include <stddef.h>
 
 #include <list>
+#include <map>
 #include <string>
 
 #include "base/gtest_prod_util.h"
@@ -118,7 +119,7 @@ class NET_EXPORT HttpAuthCache {
   // This also defines the worst-case lookup times (which grow linearly
   // with number of elements in the cache).
   enum { kMaxNumPathsPerRealmEntry = 10 };
-  enum { kMaxNumRealmEntries = 10 };
+  enum { kMaxNumRealmEntries = 20 };
 
   HttpAuthCache();
   ~HttpAuthCache();
@@ -203,17 +204,16 @@ class NET_EXPORT HttpAuthCache {
   void set_clock_for_testing(const base::Clock* clock) { clock_ = clock; }
 
  private:
-  typedef std::list<Entry> EntryList;
-  EntryList entries_;
+  using EntryMap = std::multimap<GURL, Entry>;
+  EntryMap entries_;
 
   const base::TickClock* tick_clock_ = base::DefaultTickClock::GetInstance();
   const base::Clock* clock_ = base::DefaultClock::GetInstance();
 
-  // Moves an entry towards the beginning of the entry list by one place, if it
-  // is not already at the beginning. This makes the more frequently accessed
-  // entries migrate towards the beginning of the list.
-  // Returns: the given entry, after possible move.
-  Entry* MoveEntryTowardsBeginning(EntryList::iterator entry_it);
+  EntryMap::iterator LookupEntryIt(const GURL& origin,
+                                   const std::string& realm,
+                                   HttpAuth::Scheme scheme);
+  void EvictLeastRecentlyUsedEntry();
 };
 
 // An authentication realm entry.

net/http/http_auth_cache_unittest.cc

@@ -83,6 +83,7 @@ AuthCredentials CreateASCIICredentials(const char* username,
 // Test adding and looking-up cache entries (both by realm and by path).
 TEST(HttpAuthCacheTest, Basic) {
   GURL origin("http://www.google.com");
+  GURL origin2("http://www.foobar.com");
   HttpAuthCache cache;
   HttpAuthCache::Entry* entry;
 
@@ -129,7 +130,18 @@ TEST(HttpAuthCacheTest, Basic) {
                                    "realm4-basic-password"),
             "/");
 
-  // There is no Realm5
+  std::unique_ptr<HttpAuthHandler> origin2_realm5_handler(new MockAuthHandler(
+      HttpAuth::AUTH_SCHEME_BASIC, kRealm5, HttpAuth::AUTH_SERVER));
+  cache.Add(origin2, origin2_realm5_handler->realm(),
+            origin2_realm5_handler->auth_scheme(), "Basic realm=Realm5",
+            CreateASCIICredentials("realm5-user", "realm5-password"), "/");
+  cache.Add(
+      origin2, realm3_basic_handler->realm(),
+      realm3_basic_handler->auth_scheme(), "Basic realm=Realm3",
+      CreateASCIICredentials("realm3-basic-user", "realm3-basic-password"),
+      std::string());
+
+  // There is no Realm5 in origin
   entry = cache.Lookup(origin, kRealm5, HttpAuth::AUTH_SCHEME_BASIC);
   EXPECT_TRUE(NULL == entry);
 
@@ -154,6 +166,12 @@ TEST(HttpAuthCacheTest, Basic) {
   EXPECT_EQ(ASCIIToUTF16("realm3-basic-password"),
             entry->credentials().password());
 
+  // Same realm, scheme with different origins
+  HttpAuthCache::Entry* entry2 = cache.Lookup(
+      GURL("http://www.foobar.com:80"), kRealm3, HttpAuth::AUTH_SCHEME_BASIC);
+  ASSERT_FALSE(NULL == entry2);
+  EXPECT_NE(entry, entry2);
+
   // Valid lookup by origin, realm, scheme when there's a duplicate
   // origin, realm in the cache
   entry = cache.Lookup(
@@ -698,22 +716,37 @@ class HttpAuthCacheEvictionTest : public testing::Test {
 
 // Add the maxinim number of realm entries to the cache. Each of these entries
 // must still be retrievable. Next add three more entries -- since the cache is
-// full this causes FIFO eviction of the first three entries.
+// full this causes FIFO eviction of the first three entries by time of last
+// use.
 TEST_F(HttpAuthCacheEvictionTest, RealmEntryEviction) {
-  for (int i = 0; i < kMaxRealms; ++i)
+  base::SimpleTestTickClock test_clock;
+  test_clock.SetNowTicks(base::TimeTicks::Now());
+  cache_.set_tick_clock_for_testing(&test_clock);
+
+  for (int i = 0; i < kMaxRealms; ++i) {
     AddRealm(i);
+    test_clock.Advance(base::TimeDelta::FromSeconds(1));
+  }
 
-  for (int i = 0; i < kMaxRealms; ++i)
+  for (int i = 0; i < kMaxRealms; ++i) {
     CheckRealmExistence(i, true);
+    test_clock.Advance(base::TimeDelta::FromSeconds(1));
+  }
 
-  for (int i = 0; i < 3; ++i)
+  for (int i = 0; i < 3; ++i) {
     AddRealm(i + kMaxRealms);
+    test_clock.Advance(base::TimeDelta::FromSeconds(1));
+  }
 
-  for (int i = 0; i < 3; ++i)
+  for (int i = 0; i < 3; ++i) {
     CheckRealmExistence(i, false);
+    test_clock.Advance(base::TimeDelta::FromSeconds(1));
+  }
 
-  for (int i = 0; i < kMaxRealms; ++i)
+  for (int i = 0; i < kMaxRealms; ++i) {
     CheckRealmExistence(i + 3, true);
+    test_clock.Advance(base::TimeDelta::FromSeconds(1));
+  }
 }
 
 // Add the maximum number of paths to a single realm entry. Each of these

tools/metrics/histograms/histograms.xml

@@ -54845,6 +54845,10 @@ uploading your change for review.
 </histogram>
 
 <histogram name="Net.HttpAuthCacheEntriesExaminedWhenNoMatch">
+  <obsolete>
+    Deprecated 10/2018 when changing the cache implementation such that the
+    number of entries examined no longer applies.
+  </obsolete>
   <owner>chlily@chromium.org</owner>
   <summary>
     The number of entries examined (equal to the cache size) when attempting to
@@ -54856,6 +54860,10 @@ uploading your change for review.
 </histogram>
 
 <histogram name="Net.HttpAuthCacheLookupByPathPosition">
+  <obsolete>
+    Deprecated 10/2018 when changing the cache implementation such that the
+    lookup position no longer applies.
+  </obsolete>
   <owner>asanka@chromium.org</owner>
   <summary>
     When looking up an HTTP auth cache entry by path, the position (1-indexed)
@@ -54864,6 +54872,10 @@ uploading your change for review.
 </histogram>
 
 <histogram name="Net.HttpAuthCacheLookupPosition">
+  <obsolete>
+    Deprecated 10/2018 when changing the cache implementation such that the
+    lookup position no longer applies.
+  </obsolete>
   <owner>asanka@chromium.org</owner>
   <summary>
     When looking up an HTTP auth cache entry by realm, the position (1-indexed)