Various optimizations to reduce the number of temporary allocations.

chrome/browser/extensions/api/font_settings/font_settings_api.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.
 
 // Font Settings Extension API implementation.
 
 #include "chrome/browser/extensions/api/font_settings/font_settings_api.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 43 matching lines @@
 
 // Gets the font name preference path for |generic_family| and |script|. If
 // |script| is NULL, uses prefs::kWebKitCommonScript.
 std::string GetFontNamePrefPath(fonts::GenericFamily generic_family_enum,
                                 fonts::ScriptCode script_enum) {
   std::string script = fonts::ToString(script_enum);
   if (script.empty())
     script = prefs::kWebKitCommonScript;
   std::string generic_family = fonts::ToString(generic_family_enum);
   return base::StringPrintf(kWebKitFontPrefFormat,
-                            generic_family.c_str(),
-                            script.c_str());
+                            generic_family,
+                            script);
 }
 
 // Returns the localized name of a font so that it can be matched within the
 // list of system fonts. On Windows, the list of system fonts has names only
 // for the system locale, but the pref value may be in the English name.
 std::string MaybeGetLocalizedFontName(const std::string& font_name) {
 #if defined(OS_WIN)
   if (!font_name.empty()) {
     gfx::Font font(font_name, 12);  // dummy font size
     return static_cast<gfx::PlatformFontWin*>(font.platform_font())->
         GetLocalizedFontName();
   }
 #endif
   return font_name;
 }
 
 // Registers |obs| to observe per-script font prefs under the path |map_name|.
 void RegisterFontFamilyMapObserver(
     PrefChangeRegistrar* registrar,
     const char* map_name,
-    const PrefChangeRegistrar::NamedChangeCallback& callback) {
+    const PrefChangeRegistrar::NamedChangeCallback& callback,
+    std::string* pref_name) {
   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);
-    registrar->Add(pref_name.c_str(), callback);
+    pref_name->assign(map_name);
+    pref_name->append(1, '.');
+    pref_name->append(script);

[Nico, 2014/12/01 17:05:15]

This kind of change does not lgtm. The previous code is much more readable.

If this is actually a performance issue, maybe we can improve StringPrintf(), or
port the google-internal StrCat() thing which I believe is pretty good about
allocations.

[Peter Kasting, 2014/12/01 23:09:17]

FWIW, I actually find the new code significantly more readable, because prtinf
format strings have never made sense to me (I use them so rarely).

What about this, how's its perf?  This seems more readable than either, to me:

    *pref_name = map_name + '.' + script;

Or if that's not fast enough how about writing what this patch does but with
operators (which are better than the named functions for readability IMO):

    *pref_name = map_name;
    *pref_name += '.';
    *pref_name += script;

Nico, WDYT?

[Nico, 2014/12/01 23:19:06]

Huh, this is very surprising to me. I thought printf strings are so prevalent in
all languages that they make sense to everyone, but I do remember finding them
pretty hard to read when they were new to me.

…cs.chromium.org finds oodles of uses of it just below chrome/ though, so I
guess at least quite a few people like and understand them?
This one does one allocation per operator+, and since a new object is returned
for every + the allocation can't be amortized away – if you have N +s, this will
do O(n^2) character copies behind the scenes (and n allocations).
That's better than + since now the string can grow 2x when it needs to grow and
allocations get amortized. Personally, I can read printf strings much faster
than any manual concatenation, but I admit I kind of just assumed that that's
universally true.

(A printf should in theory be implementable with a single allocation internally,
which is fewer allocations than a few +=s, but I'm not sure if our
implementation gets this right.)

[Peter Kasting, 2014/12/01 23:35:30]

We have quite a few in our codebase, but I think in part it's because a lot of
people don't really know any other way to e.g. concat an int to a string, or
assume that StringPrintF is always going to be much faster than the equivalent
functionality using just the string methods.  I go out of my way to avoid them
in my own code because they just look like line noise to me :P
I was hoping that one of the new objects (the result of the whole RHS) would get
optimized away, so we'd just have one new object for "map_name + .", but yeah,
it's not as good as the way this patch is written.

Separately, I really hate the way this patch passes in |pref_name| when it's
neither an in-param nor an out-param.  I think the function should still take 3
args and should instead define pref_name as a local outside the loop if it wants
to avoid allocing and freeing as much.  This results in more allocations than
the way the patch currently stands, but fewer than the number the original code
did, and doesn't make the API misleading.

That said, since this is file-local, it's better than if this was in a .h
somewhere.

[cmumford, 2014/12/03 17:28:29]

I wonder if creating a variadic template function to concat strings (and
possibly other types) would meet the goal of reducing allocations and also
improve readability. Something like
http://stackoverflow.com/questions/21806561/concatenating-strings-and-numbers...

+    registrar->Add(*pref_name, callback);
   }
 }
 
 }  // namespace
 
 FontSettingsEventRouter::FontSettingsEventRouter(
     Profile* profile) : profile_(profile) {
   registrar_.Init(profile_->GetPrefs());
 
   AddPrefToObserve(prefs::kWebKitDefaultFixedFontSize,
                    fonts::OnDefaultFixedFontSizeChanged::kEventName,
                    kPixelSizeKey);
   AddPrefToObserve(prefs::kWebKitDefaultFontSize,
                    fonts::OnDefaultFontSizeChanged::kEventName,
                    kPixelSizeKey);
   AddPrefToObserve(prefs::kWebKitMinimumFontSize,
                    fonts::OnMinimumFontSizeChanged::kEventName,
                    kPixelSizeKey);
 
   PrefChangeRegistrar::NamedChangeCallback callback =
       base::Bind(&FontSettingsEventRouter::OnFontFamilyMapPrefChanged,
                  base::Unretained(this));
-  RegisterFontFamilyMapObserver(&registrar_,
-                                prefs::kWebKitStandardFontFamilyMap, callback);
-  RegisterFontFamilyMapObserver(&registrar_,
-                                prefs::kWebKitSerifFontFamilyMap, callback);
-  RegisterFontFamilyMapObserver(&registrar_,
-                                prefs::kWebKitSansSerifFontFamilyMap, callback);
-  RegisterFontFamilyMapObserver(&registrar_,
-                                prefs::kWebKitFixedFontFamilyMap, callback);
-  RegisterFontFamilyMapObserver(&registrar_,
-                                prefs::kWebKitCursiveFontFamilyMap, callback);
-  RegisterFontFamilyMapObserver(&registrar_,
-                                prefs::kWebKitFantasyFontFamilyMap, callback);
-  RegisterFontFamilyMapObserver(&registrar_,
-                                prefs::kWebKitPictographFontFamilyMap,
-                                callback);
+
+  std::string pref_name;
+  pref_name.reserve(500);
+  RegisterFontFamilyMapObserver(
+      &registrar_, prefs::kWebKitStandardFontFamilyMap, callback, &pref_name);
+  RegisterFontFamilyMapObserver(&registrar_, prefs::kWebKitSerifFontFamilyMap,
+                                callback, &pref_name);
+  RegisterFontFamilyMapObserver(
+      &registrar_, prefs::kWebKitSansSerifFontFamilyMap, callback, &pref_name);
+  RegisterFontFamilyMapObserver(&registrar_, prefs::kWebKitFixedFontFamilyMap,
+                                callback, &pref_name);
+  RegisterFontFamilyMapObserver(&registrar_, prefs::kWebKitCursiveFontFamilyMap,
+                                callback, &pref_name);
+  RegisterFontFamilyMapObserver(&registrar_, prefs::kWebKitFantasyFontFamilyMap,
+                                callback, &pref_name);
+  RegisterFontFamilyMapObserver(
+      &registrar_, prefs::kWebKitPictographFontFamilyMap, callback, &pref_name);
 }
 
 FontSettingsEventRouter::~FontSettingsEventRouter() {}
 
 void FontSettingsEventRouter::AddPrefToObserve(const char* pref_name,
                                                const char* event_name,
                                                const char* key) {
   registrar_.Add(pref_name,
                  base::Bind(&FontSettingsEventRouter::OnFontPrefChanged,
                             base::Unretained(this),
@@ skipping 11 matching lines @@
   }
 
   NOTREACHED();
 }
 
 void FontSettingsEventRouter::OnFontNamePrefChanged(
     const std::string& pref_name,
     const std::string& generic_family,
     const std::string& script) {
   const PrefService::Preference* pref = registrar_.prefs()->FindPreference(
-      pref_name.c_str());
+      pref_name);
   CHECK(pref);
 
   std::string font_name;
   if (!pref->GetValue()->GetAsString(&font_name)) {
     NOTREACHED();
     return;
   }
   font_name = MaybeGetLocalizedFontName(font_name);
 
   base::ListValue args;
@@ skipping 10 matching lines @@
       APIPermission::kFontSettings,
       false,
       pref_name);
 }
 
 void FontSettingsEventRouter::OnFontPrefChanged(
     const std::string& event_name,
     const std::string& key,
     const std::string& pref_name) {
   const PrefService::Preference* pref = registrar_.prefs()->FindPreference(
-      pref_name.c_str());
+      pref_name);
   CHECK(pref);
 
   base::ListValue args;
   base::DictionaryValue* dict = new base::DictionaryValue();
   args.Append(dict);
   dict->Set(key, pref->GetValue()->DeepCopy());
 
   extensions::preference_helpers::DispatchEventToExtensions(
       profile_,
       event_name,
@@ skipping 27 matching lines @@
 
   scoped_ptr<fonts::ClearFont::Params> params(
       fonts::ClearFont::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
 
   std::string pref_path = GetFontNamePrefPath(params->details.generic_family,
                                               params->details.script);
 
   // Ensure |pref_path| really is for a registered per-script font pref.
   EXTENSION_FUNCTION_VALIDATE(
-      GetProfile()->GetPrefs()->FindPreference(pref_path.c_str()));
+      GetProfile()->GetPrefs()->FindPreference(pref_path));
 
   PreferenceAPI::Get(GetProfile())->RemoveExtensionControlledPref(
-      extension_id(), pref_path.c_str(), kExtensionPrefsScopeRegular);
+      extension_id(), pref_path, kExtensionPrefsScopeRegular);
   return true;
 }
 
 bool FontSettingsGetFontFunction::RunSync() {
   scoped_ptr<fonts::GetFont::Params> params(
       fonts::GetFont::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
 
   std::string pref_path = GetFontNamePrefPath(params->details.generic_family,
                                               params->details.script);
 
   PrefService* prefs = GetProfile()->GetPrefs();
   const PrefService::Preference* pref =
-      prefs->FindPreference(pref_path.c_str());
+      prefs->FindPreference(pref_path);
 
   std::string font_name;
   EXTENSION_FUNCTION_VALIDATE(
       pref && pref->GetValue()->GetAsString(&font_name));
   font_name = MaybeGetLocalizedFontName(font_name);
 
   // We don't support incognito-specific font prefs, so don't consider them when
   // getting level of control.
   const bool kIncognito = false;
   std::string level_of_control =
@@ skipping 15 matching lines @@
 
   scoped_ptr<fonts::SetFont::Params> params(
       fonts::SetFont::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
 
   std::string pref_path = GetFontNamePrefPath(params->details.generic_family,
                                               params->details.script);
 
   // Ensure |pref_path| really is for a registered font pref.
   EXTENSION_FUNCTION_VALIDATE(
-      GetProfile()->GetPrefs()->FindPreference(pref_path.c_str()));
+      GetProfile()->GetPrefs()->FindPreference(pref_path));
 
   PreferenceAPI::Get(GetProfile())->SetExtensionControlledPref(
       extension_id(),
-      pref_path.c_str(),
+      pref_path,
       kExtensionPrefsScopeRegular,
       new base::StringValue(params->details.font_id));
   return true;
 }
 
 bool FontSettingsGetFontListFunction::RunAsync() {
   content::GetFontListAsync(
       Bind(&FontSettingsGetFontListFunction::FontListHasLoaded, this));
   return true;
 }
@@ skipping 142 matching lines @@
 
 const char* FontSettingsSetMinimumFontSizeFunction::GetPrefName() {
   return prefs::kWebKitMinimumFontSize;
 }
 
 const char* FontSettingsSetMinimumFontSizeFunction::GetKey() {
   return kPixelSizeKey;
 }
 
 }  // namespace extensions