linux: Cache GetFontRenderParams() results.

ui/gfx/font_render_params_linux.cc

Index: ui/gfx/font_render_params_linux.cc
diff --git a/ui/gfx/font_render_params_linux.cc b/ui/gfx/font_render_params_linux.cc
index f7c1119c703ae80503450342931223673142ce67..fb0239a05e92dcf0bef4a5924d1671f1f13d2c04 100644
--- a/ui/gfx/font_render_params_linux.cc
+++ b/ui/gfx/font_render_params_linux.cc
@@ -5,7 +5,12 @@
 #include "ui/gfx/font_render_params.h"
 
 #include "base/command_line.h"
+#include "base/containers/mru_cache.h"
+#include "base/hash.h"
 #include "base/logging.h"
+#include "base/macros.h"
+#include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
 #include "ui/gfx/font.h"
 #include "ui/gfx/linux_font_delegate.h"
 #include "ui/gfx/switches.h"
@@ -16,6 +21,13 @@ namespace gfx {
 
 namespace {
 
+// Should cache entries by dropped by the next call to GetFontRenderParams()?
+// Used for tests.
+bool g_drop_cache = false;

[msw, 2014/08/05 00:55:10]

nit: g_drop_cache_for_test

[Daniel Erat, 2014/08/06 17:43:13]

done (well, g_clear_cache_for_test to match the function name)

+
+// Number of recent GetFontRenderParams() results to cache.
+const size_t kCacheSize = 10;

[Daniel Erat, 2014/08/04 23:44:43]

i pulled this number out of the air, but it seems to be about right to avoid
dropping any entries during startup and typing into the omnibox on linux.

[msw, 2014/08/05 00:55:10]

Acknowledged.

+
 // Converts Fontconfig FC_HINT_STYLE to FontRenderParams::Hinting.
 FontRenderParams::Hinting ConvertFontconfigHintStyle(int hint_style) {
   switch (hint_style) {
@@ -114,12 +126,36 @@ bool QueryFontconfig(const FontRenderParamsQuery& query,
   return true;
 }
 
+// Serialize |query| into a string and hash it to a value suitable for use as a
+// cache key.
+uint32 HashFontRenderParamsQuery(const FontRenderParamsQuery& query) {
+  return base::Hash(base::StringPrintf("%d|%d|%d|%d|%s",

[Daniel Erat, 2014/08/04 23:44:43]

this is ugly. i can't just pass the FontRenderParamsQuery bytes into
base::SuperFastHash() since it contains a vector of families. ideally there
would be a hasher somewhere that i can feed data to incrementally to avoid
building up a big string like this, but i couldn't find one.

[msw, 2014/08/05 00:55:10]

Acknowledged.

+      query.for_web_contents, query.pixel_size, query.point_size, query.style,
+      JoinString(query.families, ',').c_str()));
+}
+
 }  // namespace
 
 FontRenderParams GetFontRenderParams(const FontRenderParamsQuery& query,
                                      std::string* family_out) {
-  if (family_out)
+  typedef base::MRUCache<uint32, FontRenderParams> Cache;

[msw, 2014/08/05 00:55:10]

optional nit: key on FontRenderParamsQuery and ditch the hashing...

[Daniel Erat, 2014/08/06 17:43:13]

i tried this at first, but MRUCache uses std::map, so it needs operator<(). i
guess i could write this, but it also warns that it stores multiple copies of
the key, so hashing the struct to a uint32 seemed safer.

[msw, 2014/08/06 19:31:38]

Acknowledged.

+  CR_DEFINE_STATIC_LOCAL(Cache, cache, (kCacheSize));
+
+  if (g_drop_cache) {

[msw, 2014/08/05 00:55:10]

This pattern is slightly ugly, did you consider any alternatives?

[Daniel Erat, 2014/08/06 17:43:13]

yes, but let me know if you can think of anything else:

- i don't think i can use LazyInstance since MRUCache's c'tor takes an argument
- i can't use a global static instance since the cache is non-POD
- is it okay to have a global pointer and leak the object? i wasn't sure

[msw, 2014/08/06 19:31:38]

Shrug, I guess it's fine as-is.

+    cache.Clear();
+    g_drop_cache = false;
+  }
+
+  const uint32 hash = HashFontRenderParamsQuery(query);
+  if (!family_out) {
+    // The family returned by Fontconfig isn't part of FontRenderParams, so we
+    // can only return a value from the cache if it wasn't requested.
+    Cache::const_iterator it = cache.Get(hash);
+    if (it != cache.end())
+      return it->second;
+  } else {
     family_out->clear();
+  }
 
   // Start with the delegate's settings, but let Fontconfig have the final say.
   FontRenderParams params;
@@ -152,7 +188,12 @@ FontRenderParams GetFontRenderParams(const FontRenderParamsQuery& query,
   if (family_out && family_out->empty() && !query.families.empty())
     *family_out = query.families[0];
 
+  cache.Put(hash, params);
   return params;
 }
 
+void DropFontRenderParamsCacheForTest() {
+  g_drop_cache = true;
+}
+
 }  // namespace gfx