Various optimizations to reduce the number of temporary allocations.

chrome/browser/extensions/api/font_settings/font_settings_api.cc

Index: chrome/browser/extensions/api/font_settings/font_settings_api.cc
diff --git a/chrome/browser/extensions/api/font_settings/font_settings_api.cc b/chrome/browser/extensions/api/font_settings/font_settings_api.cc
index cf01a958e33df9ba0d82949357627cf7e80edca3..66176ef85b5e0b5213ce506f1d7522218f211aa7 100644
--- a/chrome/browser/extensions/api/font_settings/font_settings_api.cc
+++ b/chrome/browser/extensions/api/font_settings/font_settings_api.cc
@@ -61,8 +61,8 @@ std::string GetFontNamePrefPath(fonts::GenericFamily generic_family_enum,
     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
@@ -83,11 +83,14 @@ std::string MaybeGetLocalizedFontName(const std::string& font_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);
   }
 }
 
@@ -110,21 +113,23 @@ FontSettingsEventRouter::FontSettingsEventRouter(
   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() {}
@@ -156,7 +161,7 @@ void FontSettingsEventRouter::OnFontNamePrefChanged(
     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;
@@ -187,7 +192,7 @@ void FontSettingsEventRouter::OnFontPrefChanged(
     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;
@@ -235,10 +240,10 @@ bool FontSettingsClearFontFunction::RunSync() {
 
   // 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;
 }
 
@@ -252,7 +257,7 @@ bool FontSettingsGetFontFunction::RunSync() {
 
   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(
@@ -288,11 +293,11 @@ bool FontSettingsSetFontFunction::RunSync() {
 
   // 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;