Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
440773003:
linux: Cache GetFontRenderParams() results. (Closed)
|
Created:
6 years, 4 months ago by Daniel Erat Modified:
6 years, 4 months ago Reviewers:
msw CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
Descriptionlinux: Cache GetFontRenderParams() results.
Make the Linux implementation of GetFontRenderParams() cache
the ten most recent results to greatly reduce the number of
Fontconfig queries that are performed.
BUG=376077, 391830
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287977
Patch Set 1 #
Total comments: 18
Patch Set 2 : apply review feedback #Patch Set 3 : remove ifdef from header and add todo #Patch Set 4 : whoops, rename function calls in tests #
Messages
Total messages: 16 (0 generated)
for your consideration. i suspect that i'd also need to update the blink test harness for this change. https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params.h File ui/gfx/font_render_params.h (right): https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params.h#... ui/gfx/font_render_params.h:96: GFX_EXPORT void DropFontRenderParamsCacheForTest(); i'm getting "undeclared identifier" errors that i don't understand when i hide this behind "#if defined(OS_LINUX)" -- presumably that indicates that OS_LINUX isn't defined when font_render_params_linux_unittest.cc includes this file, which doesn't make sense to me. i could spend more time trying to figure out what's going on there, or add no-op implementations for non-linux platforms, or add a comment mentioning that this is only defined for linux. https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params_li... File ui/gfx/font_render_params_linux.cc (right): https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params_li... ui/gfx/font_render_params_linux.cc:29: const size_t kCacheSize = 10; 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. https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params_li... ui/gfx/font_render_params_linux.cc:132: return base::Hash(base::StringPrintf("%d|%d|%d|%d|%s", 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.
https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params.h File ui/gfx/font_render_params.h (right): https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params.h#... ui/gfx/font_render_params.h:96: GFX_EXPORT void DropFontRenderParamsCacheForTest(); nit: s/Drop/Clear/ https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params.h#... ui/gfx/font_render_params.h:96: GFX_EXPORT void DropFontRenderParamsCacheForTest(); On 2014/08/04 23:44:43, Daniel Erat wrote: > i'm getting "undeclared identifier" errors that i don't understand when i hide > this behind "#if defined(OS_LINUX)" -- presumably that indicates that OS_LINUX > isn't defined when font_render_params_linux_unittest.cc includes this file, > which doesn't make sense to me. i could spend more time trying to figure out > what's going on there, or add no-op implementations for non-linux platforms, or > add a comment mentioning that this is only defined for linux. That seems very odd to me as well... would you mind uploading a patch set with the preprocessor checks you wish to add and see if the trybots or I can help at all? Otherwise, just adding a comment for now seems fine. https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params_li... File ui/gfx/font_render_params_linux.cc (right): https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params_li... ui/gfx/font_render_params_linux.cc:26: bool g_drop_cache = false; nit: g_drop_cache_for_test https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params_li... ui/gfx/font_render_params_linux.cc:29: const size_t kCacheSize = 10; On 2014/08/04 23:44:43, Daniel Erat wrote: > 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. Acknowledged. https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params_li... ui/gfx/font_render_params_linux.cc:132: return base::Hash(base::StringPrintf("%d|%d|%d|%d|%s", On 2014/08/04 23:44:43, Daniel Erat wrote: > 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. Acknowledged. https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params_li... ui/gfx/font_render_params_linux.cc:141: typedef base::MRUCache<uint32, FontRenderParams> Cache; optional nit: key on FontRenderParamsQuery and ditch the hashing... https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params_li... ui/gfx/font_render_params_linux.cc:144: if (g_drop_cache) { This pattern is slightly ugly, did you consider any alternatives?
You may also want to do some measurements, to see the number of calls that hit/miss the cache, the number of overall calls, and how long they really take.
i added some logging earlier; here are more-detailed numbers: during startup: - params computed 15 times (only once due to family_out being non-NULL) - cached params returned 22 times while typing "www.google.com" into the omnibox: - params computed 0 times - a single copy of cached params returned 350 times (!) in a release build on my z620, hashing a FontRenderParamsQuery object takes 5-75 usec (usually skewed toward the lower end) and filling a new FontRenderParams object takes 830 usec to 2.2 ms (!!) https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params.h File ui/gfx/font_render_params.h (right): https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params.h#... ui/gfx/font_render_params.h:96: GFX_EXPORT void DropFontRenderParamsCacheForTest(); On 2014/08/05 00:55:10, msw wrote: > nit: s/Drop/Clear/ Done. https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params.h#... ui/gfx/font_render_params.h:96: GFX_EXPORT void DropFontRenderParamsCacheForTest(); On 2014/08/05 00:55:10, msw wrote: > On 2014/08/04 23:44:43, Daniel Erat wrote: > > i'm getting "undeclared identifier" errors that i don't understand when i hide > > this behind "#if defined(OS_LINUX)" -- presumably that indicates that OS_LINUX > > isn't defined when font_render_params_linux_unittest.cc includes this file, > > which doesn't make sense to me. i could spend more time trying to figure out > > what's going on there, or add no-op implementations for non-linux platforms, > or > > add a comment mentioning that this is only defined for linux. > > That seems very odd to me as well... would you mind uploading a patch set with > the preprocessor checks you wish to add and see if the trybots or I can help at > all? Otherwise, just adding a comment for now seems fine. sure, uploaded. and i can see with an #error that OS_LINUX is in fact not defined here. https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params_li... File ui/gfx/font_render_params_linux.cc (right): https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params_li... ui/gfx/font_render_params_linux.cc:26: bool g_drop_cache = false; On 2014/08/05 00:55:10, msw wrote: > nit: g_drop_cache_for_test done (well, g_clear_cache_for_test to match the function name) https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params_li... ui/gfx/font_render_params_linux.cc:141: typedef base::MRUCache<uint32, FontRenderParams> Cache; On 2014/08/05 00:55:10, msw wrote: > optional nit: key on FontRenderParamsQuery and ditch the hashing... 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. https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params_li... ui/gfx/font_render_params_linux.cc:144: if (g_drop_cache) { On 2014/08/05 00:55:10, msw wrote: > This pattern is slightly ugly, did you consider any alternatives? 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
On 2014/08/06 17:43:13, Daniel Erat wrote: > i added some logging earlier; here are more-detailed numbers: > > during startup: > - params computed 15 times (only once due to family_out being non-NULL) > - cached params returned 22 times > > while typing "www.google.com" into the omnibox: > - params computed 0 times > - a single copy of cached params returned 350 times (!) LGTM. Yikes, we really need to audit some the text rendering stuff. (I really think the omnibox popup is doing something wrong...) https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params.h File ui/gfx/font_render_params.h (right): https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params.h#... ui/gfx/font_render_params.h:96: GFX_EXPORT void DropFontRenderParamsCacheForTest(); On 2014/08/06 17:43:11, Daniel Erat wrote: > On 2014/08/05 00:55:10, msw wrote: > > On 2014/08/04 23:44:43, Daniel Erat wrote: > > > i'm getting "undeclared identifier" errors that i don't understand when i > hide > > > this behind "#if defined(OS_LINUX)" -- presumably that indicates that > OS_LINUX > > > isn't defined when font_render_params_linux_unittest.cc includes this file, > > > which doesn't make sense to me. i could spend more time trying to figure out > > > what's going on there, or add no-op implementations for non-linux platforms, > > or > > > add a comment mentioning that this is only defined for linux. > > > > That seems very odd to me as well... would you mind uploading a patch set with > > the preprocessor checks you wish to add and see if the trybots or I can help > at > > all? Otherwise, just adding a comment for now seems fine. > > sure, uploaded. and i can see with an #error that OS_LINUX is in fact not > defined here. Acknowledged; the comment is fine. https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params_li... File ui/gfx/font_render_params_linux.cc (right): https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params_li... ui/gfx/font_render_params_linux.cc:141: typedef base::MRUCache<uint32, FontRenderParams> Cache; On 2014/08/06 17:43:13, Daniel Erat wrote: > On 2014/08/05 00:55:10, msw wrote: > > optional nit: key on FontRenderParamsQuery and ditch the hashing... > > 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. Acknowledged. https://codereview.chromium.org/440773003/diff/1/ui/gfx/font_render_params_li... ui/gfx/font_render_params_linux.cc:144: if (g_drop_cache) { On 2014/08/06 17:43:13, Daniel Erat wrote: > On 2014/08/05 00:55:10, msw wrote: > > This pattern is slightly ugly, did you consider any alternatives? > > 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 Shrug, I guess it's fine as-is.
The CQ bit was checked by derat@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/derat@chromium.org/440773003/40001
The CQ bit was checked by derat@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/derat@chromium.org/440773003/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
The CQ bit was checked by derat@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/derat@chromium.org/440773003/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
Message was sent while issue was closed.
Change committed as 287977 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
