Tweaks to improve memory consumption by TopSites. The biggest culprit

seems to repeatedly querying history during this perf test. So, the
biggest gain is made by scaling this back. Here are other tweaks I've
made:

. decrease the page size/cache size of the two DBs. Now that favicons
  doesn't store thumbnails the db doesn't need to be so big.
. Set a cap to the max number of tmp thumnbails maintains.
. Force jpg data vector to be only as big as it needs to be.
. Tweak TopSitesCache so it doesn't end up duplicating so many
  GURLs. This is particularly helpful for long redirect lists.

BUG=61487
TEST=none

Review URL: http://codereview.chromium.org/4106014

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@64837 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 126 insertions, 52 deletions

diff --git a/chrome/browser/history/thumbnail_database.cc b/chrome/browser/history/thumbnail_database.cc
index e615876..e1054ac 100644
--- a/chrome/browser/history/thumbnail_database.cc
+++ b/chrome/browser/history/thumbnail_database.cc
@@ -104,21 +104,10 @@ sql::InitStatus ThumbnailDatabase::OpenDatabase(sql::Connection* db,
   // Set the exceptional sqlite error handler.
   db->set_error_delegate(GetErrorHandlerForThumbnailDb());
 
-  // Set the database page size to something  larger to give us
-  // better performance (we're typically seek rather than bandwidth limited).
-  // This only has an effect before any tables have been created, otherwise
-  // this is a NOP. Must be a power of 2 and a max of 8192. We use a bigger
-  // one because we're storing larger data (4-16K) in it, so we want a few
-  // blocks per element.
-  db->set_page_size(4096);
-
-  // The UI is generally designed to work well when the thumbnail database is
-  // slow, so we can tolerate much less caching. The file is also very large
-  // and so caching won't save a significant percentage of it for us,
-  // reducing the benefit of caching in the first place. With the default cache
-  // size of 2000 pages, it will take >8MB of memory, so reducing it can be a
-  // big savings.
-  db->set_cache_size(64);
+  // Thumbnails db now only stores favicons, so we don't need that big a page
+  // size or cache.
+  db->set_page_size(2048);
+  db->set_cache_size(32);
 
   // Run the database in exclusive mode. Nobody else should be accessing the
   // database while we're running, and this will give somewhat improved perf.
diff --git a/chrome/browser/history/top_sites.cc b/chrome/browser/history/top_sites.cc
index 1dacd3b..8bdfe34 100644
--- a/chrome/browser/history/top_sites.cc
+++ b/chrome/browser/history/top_sites.cc
@@ -39,6 +39,11 @@ namespace history {
 
 // How many top sites to store in the cache.
 static const size_t kTopSitesNumber = 20;
+
+// Max number of temporary images we'll cache. See comment above
+// temp_images_ for details.
+static const size_t kMaxTempTopImages = 8;
+
 static const size_t kTopSitesShown = 8;
 static const int kDaysOfHistory = 90;
 // Time from startup to first HistoryService query.
@@ -200,6 +205,9 @@ bool TopSites::SetPageThumbnail(const GURL& url,
     return false;
 
   if (add_temp_thumbnail) {
+    // Always remove the existing entry and then add it back. That way if we end
+    // up with too many temp thumbnails we'll prune the oldest first.
+    RemoveTemporaryThumbnailByURL(url);
     AddTemporaryThumbnail(url, thumbnail_data, score);
     return true;
   }
@@ -487,20 +495,42 @@ bool TopSites::EncodeBitmap(const SkBitmap& bitmap,
                             scoped_refptr<RefCountedBytes>* bytes) {
   *bytes = new RefCountedBytes();
   SkAutoLockPixels bitmap_lock(bitmap);
-  return gfx::JPEGCodec::Encode(
-      reinterpret_cast<unsigned char*>(bitmap.getAddr32(0, 0)),
-      gfx::JPEGCodec::FORMAT_BGRA, bitmap.width(),
-      bitmap.height(),
-      static_cast<int>(bitmap.rowBytes()), 90,
-      &((*bytes)->data));
+  std::vector<unsigned char> data;
+  if (!gfx::JPEGCodec::Encode(
+          reinterpret_cast<unsigned char*>(bitmap.getAddr32(0, 0)),
+          gfx::JPEGCodec::FORMAT_BGRA, bitmap.width(),
+          bitmap.height(),
+          static_cast<int>(bitmap.rowBytes()), 90,
+          &data)) {
+    return false;
+  }
+  // As we're going to cache this data, make sure the vector is only as big as
+  // it needs to be.
+  (*bytes)->data = data;
+  return true;
+}
+
+void TopSites::RemoveTemporaryThumbnailByURL(const GURL& url) {
+  for (TempImages::iterator i = temp_images_.begin(); i != temp_images_.end();
+       ++i) {
+    if (i->first == url) {
+      temp_images_.erase(i);
+      return;
+    }
+  }
 }
 
 void TopSites::AddTemporaryThumbnail(const GURL& url,
                                      const RefCountedBytes* thumbnail,
                                      const ThumbnailScore& score) {
-  Images& img = temp_thumbnails_map_[url];
-  img.thumbnail = const_cast<RefCountedBytes*>(thumbnail);
-  img.thumbnail_score = score;
+  if (temp_images_.size() == kMaxTempTopImages)
+    temp_images_.erase(temp_images_.begin());
+
+  TempImage image;
+  image.first = url;
+  image.second.thumbnail = const_cast<RefCountedBytes*>(thumbnail);
+  image.second.thumbnail_score = score;
+  temp_images_.push_back(image);
 }
 
 void TopSites::StartQueryForMostVisited() {
@@ -703,10 +733,9 @@ void TopSites::Observe(NotificationType type,
         return;
       const GURL& url = load_details->entry->url();
       if (!cache_->IsKnownURL(url) && HistoryService::CanAddURL(url)) {
-        // Ideally we would just invoke StartQueryForMostVisited, but at the
-        // time this is invoked history hasn't been updated, which means if we
-        // invoked StartQueryForMostVisited now we could get stale data.
-        RestartQueryForTopSitesTimer(base::TimeDelta::FromMilliseconds(1));
+        // To avoid slamming history we throttle requests when the url updates.
+        // To do otherwise negatively impacts perf tests.
+        RestartQueryForTopSitesTimer(GetUpdateDelay());
       }
     }
   }
@@ -731,21 +760,19 @@ void TopSites::SetTopSites(const MostVisitedURLList& new_top_sites) {
   cache_->SetTopSites(top_sites);
 
   // See if we have any tmp thumbnails for the new sites.
-  if (!temp_thumbnails_map_.empty()) {
+  if (!temp_images_.empty()) {
     for (size_t i = 0; i < top_sites.size(); ++i) {
       const MostVisitedURL& mv = top_sites[i];
       GURL canonical_url = cache_->GetCanonicalURL(mv.url);
-      for (std::map<GURL, Images>::iterator it = temp_thumbnails_map_.begin();
-           it != temp_thumbnails_map_.end(); ++it) {
-        // Must map all temp URLs to canonical ones.
-        // temp_thumbnails_map_ contains non-canonical URLs, because
-        // when we add a temp thumbnail, redirect chain is not known.
-        // This is slow, but temp_thumbnails_map_ should have very few URLs.
+      // At the time we get the thumbnail redirects aren't known, so we have to
+      // iterate through all the images.
+      for (TempImages::iterator it = temp_images_.begin();
+           it != temp_images_.end(); ++it) {
         if (canonical_url == cache_->GetCanonicalURL(it->first)) {
           SetPageThumbnailEncoded(mv.url,
                                   it->second.thumbnail,
                                   it->second.thumbnail_score);
-          temp_thumbnails_map_.erase(it);
+          temp_images_.erase(it);
           break;
         }
       }
@@ -753,7 +780,7 @@ void TopSites::SetTopSites(const MostVisitedURLList& new_top_sites) {
   }
 
   if (top_sites.size() >= kTopSitesNumber)
-    temp_thumbnails_map_.clear();
+    temp_images_.clear();
 
   ResetThreadSafeCache();
   ResetThreadSafeImageCache();
@@ -810,6 +837,12 @@ void TopSites::ResetThreadSafeImageCache() {
 }
 
 void TopSites::RestartQueryForTopSitesTimer(base::TimeDelta delta) {
+  if (timer_.IsRunning() && ((timer_start_time_ + timer_.GetCurrentDelay()) <
+                             (base::TimeTicks::Now() + delta))) {
+    return;
+  }
+
+  timer_start_time_ = base::TimeTicks::Now();
   timer_.Stop();
   timer_.Start(delta, this, &TopSites::StartQueryForMostVisited);
 }
@@ -858,7 +891,7 @@ void TopSites::OnGotMostVisitedThumbnails(
       MoveStateToLoaded();
     } else {
       top_sites_state_ = TOP_SITES_LOADED_WAITING_FOR_HISTORY;
-      // Ask for history just in case it hasn't been load yet. When history
+      // Ask for history just in case it hasn't been loaded yet. When history
       // finishes loading we'll do migration and/or move to loaded.
       profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
     }
diff --git a/chrome/browser/history/top_sites.h b/chrome/browser/history/top_sites.h
index cc70cc7..bd406f1 100644
--- a/chrome/browser/history/top_sites.h
+++ b/chrome/browser/history/top_sites.h
@@ -6,14 +6,14 @@
 #define CHROME_BROWSER_HISTORY_TOP_SITES_H_
 #pragma once
 
-#include <map>
+#include <list>
 #include <set>
 #include <string>
-#include <vector>
 
 #include "base/basictypes.h"
 #include "base/gtest_prod_util.h"
 #include "base/lock.h"
+#include "base/time.h"
 #include "base/timer.h"
 #include "base/ref_counted.h"
 #include "base/ref_counted_memory.h"
@@ -145,6 +145,9 @@ class TopSites
   friend class base::RefCountedThreadSafe<TopSites>;
   friend class TopSitesTest;
 
+  typedef std::pair<GURL, Images> TempImage;
+  typedef std::list<TempImage> TempImages;
+
   // Enumeration of the possible states history can be in.
   enum HistoryLoadState {
     // We're waiting for history to finish loading.
@@ -191,6 +194,10 @@ class TopSites
   static bool EncodeBitmap(const SkBitmap& bitmap,
                            scoped_refptr<RefCountedBytes>* bytes);
 
+  // Removes the cached thumbnail for url. Does nothing if |url| if not cached
+  // in |temp_images_|.
+  void RemoveTemporaryThumbnailByURL(const GURL& url);
+
   // Add a thumbnail for an unknown url. See temp_thumbnails_map_.
   void AddTemporaryThumbnail(const GURL& url,
                              const RefCountedBytes* thumbnail,
@@ -295,6 +302,9 @@ class TopSites
   // data stays in sync with history.
   base::OneShotTimer<TopSites> timer_;
 
+  // The time we started |timer_| at. Only valid if |timer_| is running.
+  base::TimeTicks timer_start_time_;
+
   NotificationRegistrar registrar_;
 
   // The number of URLs changed on the last update.
@@ -309,7 +319,7 @@ class TopSites
   // called, if we don't know about that URL yet and we don't have
   // enough Top Sites (new profile), we store it until the next
   // SetTopSites call.
-  URLToImagesMap temp_thumbnails_map_;
+  TempImages temp_images_;
 
   // Blacklisted and pinned URLs are stored in Preferences.
 
diff --git a/chrome/browser/history/top_sites_cache.cc b/chrome/browser/history/top_sites_cache.cc
index 9bae928..828b7014 100644
--- a/chrome/browser/history/top_sites_cache.cc
+++ b/chrome/browser/history/top_sites_cache.cc
@@ -48,17 +48,17 @@ bool TopSitesCache::GetPageThumbnail(const GURL& url,
 }
 
 GURL TopSitesCache::GetCanonicalURL(const GURL& url) {
-  std::map<GURL, size_t>::const_iterator i = canonical_urls_.find(url);
-  return i == canonical_urls_.end() ? url : top_sites_[i->second].url;
+  CanonicalURLs::iterator i = TopSitesCache::GetCanonicalURLsIterator(url);
+  return i == canonical_urls_.end() ? url : i->first.first->url;
 }
 
 bool TopSitesCache::IsKnownURL(const GURL& url) {
-  return canonical_urls_.find(url) != canonical_urls_.end();
+  return GetCanonicalURLsIterator(url) != canonical_urls_.end();
 }
 
 size_t TopSitesCache::GetURLIndex(const GURL& url) {
   DCHECK(IsKnownURL(url));
-  return canonical_urls_[url];
+  return GetCanonicalURLsIterator(url)->second;
 }
 
 void TopSitesCache::RemoveUnreferencedThumbnails() {
@@ -88,9 +88,23 @@ void TopSitesCache::StoreRedirectChain(const RedirectList& redirects,
   // Map all the redirected URLs to the destination.
   for (size_t i = 0; i < redirects.size(); i++) {
     // If this redirect is already known, don't replace it with a new one.
-    if (canonical_urls_.find(redirects[i]) == canonical_urls_.end())
-      canonical_urls_[redirects[i]] = destination;
+    if (!IsKnownURL(redirects[i])) {
+      CanonicalURLEntry entry;
+      entry.first = &(top_sites_[destination]);
+      entry.second = i;
+      canonical_urls_[entry] = destination;
+    }
   }
 }
 
+TopSitesCache::CanonicalURLs::iterator TopSitesCache::GetCanonicalURLsIterator(
+    const GURL& url) {
+  MostVisitedURL most_visited_url;
+  most_visited_url.redirects.push_back(url);
+  CanonicalURLEntry entry;
+  entry.first = &most_visited_url;
+  entry.second = 0u;
+  return canonical_urls_.find(entry);
+}
+
 }  // namespace history
diff --git a/chrome/browser/history/top_sites_cache.h b/chrome/browser/history/top_sites_cache.h
index 7cedd9f..4c5d79a 100644
--- a/chrome/browser/history/top_sites_cache.h
+++ b/chrome/browser/history/top_sites_cache.h
@@ -6,7 +6,9 @@
 #define CHROME_BROWSER_HISTORY_TOP_SITES_CACHE_H_
 #pragma once
 
+#include <algorithm>
 #include <map>
+#include <string>
 
 #include "base/ref_counted.h"
 #include "chrome/browser/history/history_types.h"
@@ -56,21 +58,47 @@ class TopSitesCache {
   void RemoveUnreferencedThumbnails();
 
  private:
+  // The entries in CanonicalURLs, see CanonicalURLs for details. The second
+  // argument gives the index of the URL into MostVisitedURLs redirects.
+  typedef std::pair<MostVisitedURL*, size_t> CanonicalURLEntry;
+
+  // Comparator used for CanonicalURLs.
+  class CanonicalURLComparator {
+   public:
+    bool operator()(const CanonicalURLEntry& e1,
+                    const CanonicalURLEntry& e2) const {
+      return e1.first->redirects[e1.second] < e2.first->redirects[e2.second];
+    }
+  };
+
+  // This is used to map from redirect url to the MostVisitedURL the redirect is
+  // from. Ideally this would be map<GURL, size_t> (second param indexing into
+  // top_sites_), but this results in duplicating all redirect urls. As some
+  // sites have a lot of redirects, we instead use the MostVisitedURL* and the
+  // index of the redirect as the key, and the index into top_sites_ as the
+  // value. This way we aren't duplicating GURLs. CanonicalURLComparator
+  // enforces the ordering as if we were using GURLs.
+  typedef std::map<CanonicalURLEntry, size_t,
+                   CanonicalURLComparator> CanonicalURLs;
+
   // Generates the set of canonical urls from |top_sites_|.
   void GenerateCanonicalURLs();
 
   // Stores a set of redirects. This is used by GenerateCanonicalURLs.
   void StoreRedirectChain(const RedirectList& redirects, size_t destination);
 
+  // Returns the iterator into canconical_urls_ for the specified url.
+  CanonicalURLs::iterator GetCanonicalURLsIterator(const GURL& url);
+
   // The top sites.
   MostVisitedURLList top_sites_;
 
   // The images. These map from canonical url to image.
   URLToImagesMap images_;
 
-  // Generated from the redirects to and from the most visited pages, this maps
-  // the redirects to the index into top_sites_ that contains it.
-  std::map<GURL, size_t> canonical_urls_;
+  // Generated from the redirects to and from the most visited pages. See
+  // description above typedef for details.
+  CanonicalURLs canonical_urls_;
 
   DISALLOW_COPY_AND_ASSIGN(TopSitesCache);
 };
diff --git a/chrome/browser/history/top_sites_database.cc b/chrome/browser/history/top_sites_database.cc
index 4c751bd..1058d22 100644
--- a/chrome/browser/history/top_sites_database.cc
+++ b/chrome/browser/history/top_sites_database.cc
@@ -132,7 +132,7 @@ std::string TopSitesDatabase::GetRedirects(const MostVisitedURL& url) {
 
 // static
 void TopSitesDatabase::SetRedirects(const std::string& redirects,
-                                        MostVisitedURL* url) {
+                                    MostVisitedURL* url) {
   std::vector<std::string> redirects_vector;
   SplitStringAlongWhitespace(redirects, &redirects_vector);
   for (size_t i = 0; i < redirects_vector.size(); i++)
@@ -366,7 +366,7 @@ sql::Connection* TopSitesDatabase::CreateDB(const FilePath& db_name) {
   // Settings copied from ThumbnailDatabase.
   db->set_error_delegate(GetErrorHandlerForThumbnailDb());
   db->set_page_size(4096);
-  db->set_cache_size(64);
+  db->set_cache_size(32);
 
   if (!db->Open(db_name)) {
     LOG(ERROR) << db->GetErrorMessage();