Add a cache for FillFontFamilyMap.

The function is called ~20,000 times on a fresh launch of Chrome on my MacBook
Pro. Adding a dumb cache to reduce redundant fetches reduced cpu cycles in the
function by 75% (profiled by DTrace).

The class FontFamilyCache performs the caching, and the observation of the
PrefService to ensure that the cache does not become stale.

The Profile owns the FontFamilyCache to ensure that the cache does not
outlive the PrefService.

BUG=308095

Review URL: https://codereview.chromium.org/439913002

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

chrome/browser/chrome_content_browser_client.cc

@@ -34,6 +34,7 @@
 #include "chrome/browser/devtools/chrome_devtools_manager_delegate.h"
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/extensions/chrome_content_browser_client_extensions_part.h"
+#include "chrome/browser/font_family_cache.h"
 #include "chrome/browser/geolocation/chrome_access_token_store.h"
 #include "chrome/browser/geolocation/geolocation_permission_context.h"
 #include "chrome/browser/geolocation/geolocation_permission_context_factory.h"
@@ -424,23 +425,7 @@ bool CertMatchesFilter(const net::X509Certificate& cert,
   return false;
 }
 
-#if !defined(OS_ANDROID)
-// Fills |map| with the per-script font prefs under path |map_name|.
-void FillFontFamilyMap(const PrefService* prefs,
-                       const char* map_name,
-                       content::ScriptFontFamilyMap* map) {
-  // TODO(falken): Get rid of the brute-force scan over possible
-  // (font family / script) combinations - see http://crbug.com/308095.
-  for (size_t i = 0; i < prefs::kWebKitScriptsForFontFamilyMapsLength; ++i) {
-    const char* script = prefs::kWebKitScriptsForFontFamilyMaps[i];
-    std::string pref_name = base::StringPrintf("%s.%s", map_name, script);
-    std::string font_family = prefs->GetString(pref_name.c_str());
-    if (!font_family.empty())
-      (*map)[script] = base::UTF8ToUTF16(font_family);
-  }
-}
-
-#if defined(OS_POSIX) && !defined(OS_MACOSX)
+#if defined(OS_POSIX) && !defined(OS_ANDROID) && !defined(OS_MACOSX)
 breakpad::CrashHandlerHostLinux* CreateCrashHandlerHost(
     const std::string& process_type) {
   base::FilePath dumps_path;
@@ -497,8 +482,7 @@ int GetCrashSignalFD(const CommandLine& command_line) {
 
   return -1;
 }
-#endif  // defined(OS_POSIX) && !defined(OS_MACOSX)
-#endif  // !defined(OS_ANDROID)
+#endif  // defined(OS_POSIX) && !defined(OS_ANDROID) && !defined(OS_MACOSX)
 
 #if !defined(OS_CHROMEOS)
 GURL GetEffectiveURLForSignin(const GURL& url) {
@@ -2094,20 +2078,27 @@ void ChromeContentBrowserClient::OverrideWebkitPrefs(
   // Fill per-script font preferences. These are not registered on Android
   // - http://crbug.com/308033.
 #if !defined(OS_ANDROID)
-  FillFontFamilyMap(prefs, prefs::kWebKitStandardFontFamilyMap,
-                    &web_prefs->standard_font_family_map);
-  FillFontFamilyMap(prefs, prefs::kWebKitFixedFontFamilyMap,
-                    &web_prefs->fixed_font_family_map);
-  FillFontFamilyMap(prefs, prefs::kWebKitSerifFontFamilyMap,
-                    &web_prefs->serif_font_family_map);
-  FillFontFamilyMap(prefs, prefs::kWebKitSansSerifFontFamilyMap,
-                    &web_prefs->sans_serif_font_family_map);
-  FillFontFamilyMap(prefs, prefs::kWebKitCursiveFontFamilyMap,
-                    &web_prefs->cursive_font_family_map);
-  FillFontFamilyMap(prefs, prefs::kWebKitFantasyFontFamilyMap,
-                    &web_prefs->fantasy_font_family_map);
-  FillFontFamilyMap(prefs, prefs::kWebKitPictographFontFamilyMap,
-                    &web_prefs->pictograph_font_family_map);
+  FontFamilyCache::FillFontFamilyMap(profile,
+                                     prefs::kWebKitStandardFontFamilyMap,
+                                     &web_prefs->standard_font_family_map);
+  FontFamilyCache::FillFontFamilyMap(profile,
+                                     prefs::kWebKitFixedFontFamilyMap,
+                                     &web_prefs->fixed_font_family_map);
+  FontFamilyCache::FillFontFamilyMap(profile,
+                                     prefs::kWebKitSerifFontFamilyMap,
+                                     &web_prefs->serif_font_family_map);
+  FontFamilyCache::FillFontFamilyMap(profile,
+                                     prefs::kWebKitSansSerifFontFamilyMap,
+                                     &web_prefs->sans_serif_font_family_map);
+  FontFamilyCache::FillFontFamilyMap(profile,
+                                     prefs::kWebKitCursiveFontFamilyMap,
+                                     &web_prefs->cursive_font_family_map);
+  FontFamilyCache::FillFontFamilyMap(profile,
+                                     prefs::kWebKitFantasyFontFamilyMap,
+                                     &web_prefs->fantasy_font_family_map);
+  FontFamilyCache::FillFontFamilyMap(profile,
+                                     prefs::kWebKitPictographFontFamilyMap,
+                                     &web_prefs->pictograph_font_family_map);
 #endif
 
   web_prefs->default_font_size =

chrome/browser/font_family_cache.cc

+// Copyright 2014 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 "chrome/browser/font_family_cache.h"
+
+#include <map>
+
+#include "base/prefs/pref_service.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/pref_font_webkit_names.h"
+#include "chrome/common/pref_names.h"
+#include "content/public/browser/notification_source.h"
+
+// Identifies the user data on the profile.
+const char kFontFamilyCacheKey[] = "FontFamilyCacheKey";
+
+FontFamilyCache::FontFamilyCache(Profile* profile)
+    : prefs_(profile->GetPrefs()) {
+  profile_pref_registrar_.Init(profile->GetPrefs());
+  notification_registrar_.Add(this,
+                              chrome::NOTIFICATION_PROFILE_DESTROYED,
+                              content::Source<Profile>(profile));
+}
+
+FontFamilyCache::~FontFamilyCache() {
+}
+
+void FontFamilyCache::FillFontFamilyMap(Profile* profile,
+                                        const char* map_name,
+                                        content::ScriptFontFamilyMap* map) {
+  FontFamilyCache* cache =
+      static_cast<FontFamilyCache*>(profile->GetUserData(&kFontFamilyCacheKey));
+  if (!cache) {
+    cache = new FontFamilyCache(profile);
+    // The profile takes ownership of |cache|.
+    profile->SetUserData(&kFontFamilyCacheKey, cache);
+  }
+
+  cache->FillFontFamilyMap(map_name, map);
+}
+
+void FontFamilyCache::FillFontFamilyMap(const char* map_name,
+                                        content::ScriptFontFamilyMap* map) {
+  // TODO(falken): Get rid of the brute-force scan over possible
+  // (font family / script) combinations - see http://crbug.com/308095.
+  for (size_t i = 0; i < prefs::kWebKitScriptsForFontFamilyMapsLength; ++i) {
+    const char* script = prefs::kWebKitScriptsForFontFamilyMaps[i];
+    base::string16 result = FetchAndCacheFont(script, map_name);
+    if (!result.empty())
+      (*map)[script] = result;
+  }
+}
+
+base::string16 FontFamilyCache::FetchFont(const char* script,
+                                          const char* map_name) {
+  std::string pref_name = base::StringPrintf("%s.%s", map_name, script);
+  std::string font = prefs_->GetString(pref_name.c_str());
+  base::string16 font16 = base::UTF8ToUTF16(font);
+
+  // Lazily constructs the map if it doesn't already exist.
+  ScriptFontMap& map = font_family_map_[map_name];
+  map[script] = font16;
+
+  // Register for profile preference changes.
+  profile_pref_registrar_.Add(
+      pref_name.c_str(),
+      base::Bind(&FontFamilyCache::OnPrefsChanged, base::Unretained(this)));
+  return font16;
+}
+
+base::string16 FontFamilyCache::FetchAndCacheFont(const char* script,
+                                                  const char* map_name) {
+  FontFamilyMap::const_iterator it = font_family_map_.find(map_name);
+  if (it != font_family_map_.end()) {
+    ScriptFontMap::const_iterator it2 = it->second.find(script);
+    if (it2 != it->second.end())
+      return it2->second;
+  }
+
+  return FetchFont(script, map_name);
+}
+
+// There are ~1000 entries in the cache. Avoid unnecessary object construction,
+// including std::string.
+void FontFamilyCache::OnPrefsChanged(const std::string& pref_name) {
+  const size_t delimiter_length = 1;
+  const char delimiter = '.';
+  for (FontFamilyMap::iterator it = font_family_map_.begin();
+       it != font_family_map_.end();
+       ++it) {
+    const char* map_name = it->first;
+    size_t map_name_length = strlen(map_name);
+
+    // If the map name doesn't match, move on.
+    if (pref_name.compare(0, map_name_length, map_name) != 0)
+      continue;
+
+    ScriptFontMap& map = it->second;
+    for (ScriptFontMap::iterator it2 = map.begin(); it2 != map.end(); ++it2) {
+      const char* script = it2->first;
+      size_t script_length = strlen(script);
+
+      // If the length doesn't match, move on.
+      if (pref_name.size() !=
+          map_name_length + script_length + delimiter_length)
+        continue;
+
+      // If the script doesn't match, move on.
+      if (pref_name.compare(
+              map_name_length + delimiter_length, script_length, script) != 0)
+        continue;
+
+      // If the delimiter doesn't match, move on.
+      if (pref_name[map_name_length] != delimiter)
+        continue;
+
+      // Clear the cache and the observer.
+      map.erase(it2);
+      profile_pref_registrar_.Remove(pref_name.c_str());
+      break;
+    }
+  }
+}
+
+void FontFamilyCache::Observe(int type,
+                              const content::NotificationSource& source,
+                              const content::NotificationDetails& details) {
+  DCHECK_EQ(chrome::NOTIFICATION_PROFILE_DESTROYED, type);
+  profile_pref_registrar_.RemoveAll();
+}

chrome/browser/font_family_cache.h

+// Copyright 2014 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.
+
+#ifndef CHROME_BROWSER_FONT_FAMILY_CACHE_H_
+#define CHROME_BROWSER_FONT_FAMILY_CACHE_H_
+
+#include "base/containers/hash_tables.h"
+#include "base/gtest_prod_util.h"
+#include "base/prefs/pref_change_registrar.h"
+#include "base/strings/string16.h"
+#include "base/supports_user_data.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+#include "content/public/common/web_preferences.h"
+
+class PrefService;
+class Profile;
+
+FORWARD_DECLARE_TEST(FontFamilyCacheTest, Caching);
+
+// Caches font family preferences associated with a PrefService. This class
+// relies on the assumption that each concatenation of map_name + '.' + script
+// is a unique string. It also relies on the assumption that the (const char*)
+// keys used in both inner and outer hash_maps are compile time constants.
+class FontFamilyCache : public base::SupportsUserData::Data,
+                        public content::NotificationObserver {
+ public:
+  explicit FontFamilyCache(Profile* profile);
+  virtual ~FontFamilyCache();
+
+  // Gets or creates the relevant FontFamilyCache, and then fills |map|.
+  static void FillFontFamilyMap(Profile* profile,
+                                const char* map_name,
+                                content::ScriptFontFamilyMap* map);
+
+  // Fills |map| with font family preferences.
+  void FillFontFamilyMap(const char* map_name,
+                         content::ScriptFontFamilyMap* map);
+
+ protected:
+  // Exposed and virtual for testing.
+  // Fetches the font without checking the cache.
+  virtual base::string16 FetchFont(const char* script, const char* map_name);
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(::FontFamilyCacheTest, Caching);
+
+  // Map from script to font.
+  // Key comparison uses pointer equality.
+  typedef base::hash_map<const char*, base::string16> ScriptFontMap;
+
+  // Map from font family to ScriptFontMap.
+  // Key comparison uses pointer equality.
+  typedef base::hash_map<const char*, ScriptFontMap> FontFamilyMap;
+
+  // Checks the cache for the font. If not present, fetches the font and stores
+  // the result in the cache.
+  // This method needs to be very fast, because it's called ~20,000 times on a
+  // fresh launch with an empty profile. It's important to avoid unnecessary
+  // object construction, hence the heavy use of const char* and the minimal use
+  // of std::string.
+  // |script| and |map_name| must be compile time constants. Two behaviors rely
+  // on this: key comparison uses pointer equality, and keys must outlive the
+  // hash_maps.
+  base::string16 FetchAndCacheFont(const char* script, const char* map_name);
+
+  // Called when font family preferences changed.
+  // Invalidates the cached entry, and removes the relevant observer.
+  // Note: It is safe to remove the observer from the pref change callback.
+  void OnPrefsChanged(const std::string& pref_name);
+
+  // content::NotificationObserver override.
+  // Called when the profile is being destructed.
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
+  // Cache of font family preferences.
+  FontFamilyMap font_family_map_;
+
+  // Weak reference.
+  // Note: The lifetime of this object is tied to the lifetime of the
+  // PrefService, so there is no worry about an invalid pointer.
+  const PrefService* prefs_;
+
+  // Reacts to profile changes.
+  PrefChangeRegistrar profile_pref_registrar_;
+
+  // Listens for profile destruction.
+  content::NotificationRegistrar notification_registrar_;
+
+  DISALLOW_COPY_AND_ASSIGN(FontFamilyCache);
+};
+
+#endif  // CHROME_BROWSER_FONT_FAMILY_CACHE_H_

chrome/browser/font_family_cache_unittest.cc

+// Copyright 2014 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 "chrome/browser/font_family_cache.h"
+
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/test/base/testing_pref_service_syncable.h"
+#include "chrome/test/base/testing_profile.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+class TestingFontFamilyCache : public FontFamilyCache {
+ public:
+  explicit TestingFontFamilyCache(Profile* profile)
+      : FontFamilyCache(profile), fetch_font_count_(0) {}
+  virtual ~TestingFontFamilyCache() {}
+  virtual base::string16 FetchFont(const char* script,
+                                   const char* map_name) OVERRIDE {
+    ++fetch_font_count_;
+    return FontFamilyCache::FetchFont(script, map_name);
+  }
+
+  int fetch_font_count_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestingFontFamilyCache);
+};
+
+}  // namespace
+
+// Tests that the cache is correctly set and cleared.
+TEST(FontFamilyCacheTest, Caching) {
+  TestingProfile profile;
+  TestingFontFamilyCache cache(&profile);
+  TestingPrefServiceSyncable* prefs = profile.GetTestingPrefService();
+
+  std::string font1("font 1");
+  std::string font2("font 2");
+  std::string map_name("webkit.webprefs.fonts.pictograph");
+  std::string script("Zzyxca");
+  std::string pref_name(map_name + '.' + script);
+  std::string pref_name2(map_name + '.' + "adsf");
+
+  // Registers 2 preferences, and sets the first one.
+  prefs->registry()->RegisterStringPref(
+      pref_name.c_str(),
+      std::string(),
+      user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
+  prefs->registry()->RegisterStringPref(
+      pref_name2.c_str(),
+      std::string(),
+      user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
+  prefs->SetString(pref_name.c_str(), font1.c_str());
+
+  // Check that the right preference is returned.
+  std::string result = base::UTF16ToUTF8(
+      cache.FetchAndCacheFont(script.c_str(), map_name.c_str()));
+  EXPECT_EQ(font1, result);
+  EXPECT_EQ(1, cache.fetch_font_count_);
+
+  // Check that the second access uses the cache.
+  result = base::UTF16ToUTF8(
+      cache.FetchAndCacheFont(script.c_str(), map_name.c_str()));
+  EXPECT_EQ(font1, result);
+  EXPECT_EQ(1, cache.fetch_font_count_);
+
+  // Changing another preference should have no effect.
+  prefs->SetString(pref_name2.c_str(), "katy perry");
+  result = base::UTF16ToUTF8(
+      cache.FetchAndCacheFont(script.c_str(), map_name.c_str()));
+  EXPECT_EQ(font1, result);
+  EXPECT_EQ(1, cache.fetch_font_count_);
+
+  // Changing the preferences invalidates the cache.
+  prefs->SetString(pref_name.c_str(), font2.c_str());
+  result = base::UTF16ToUTF8(
+      cache.FetchAndCacheFont(script.c_str(), map_name.c_str()));
+  EXPECT_EQ(font2, result);
+  EXPECT_EQ(2, cache.fetch_font_count_);
+}

chrome/chrome_browser.gypi

@@ -2066,6 +2066,8 @@
       'browser/first_run/upgrade_util_mac.cc',
       'browser/first_run/upgrade_util_win.cc',
       'browser/first_run/upgrade_util_win.h',
+      'browser/font_family_cache.cc',
+      'browser/font_family_cache.h',
       'browser/idle.cc',
       'browser/importer/external_process_importer_client.cc',
       'browser/importer/external_process_importer_client.h',

chrome/chrome_tests_unit.gypi

@@ -983,6 +983,7 @@
         'browser/favicon/favicon_handler_unittest.cc',
         'browser/file_select_helper_unittest.cc',
         'browser/first_run/first_run_unittest.cc',
+        'browser/font_family_cache_unittest.cc',
         'browser/geolocation/geolocation_permission_context_unittest.cc',
         'browser/global_keyboard_shortcuts_mac_unittest.mm',
         'browser/google/google_search_counter_android_unittest.cc',
@@ -2538,6 +2539,7 @@
             'browser/download/download_shelf_unittest.cc',
             'browser/extensions/extension_message_bubble_controller_unittest.cc',
             'browser/extensions/extension_test_message_listener_unittest.cc',
+            'browser/font_family_cache_unittest.cc',
             'browser/policy/policy_path_parser_unittest.cc',
             'browser/process_singleton_posix_unittest.cc',
             'browser/profiles/off_the_record_profile_impl_unittest.cc',