Skip per-script font preferences registration and filling on Android.

Skip per-script font preferences registration and filling on Android.

Per-script font preferences have no default values on Android, nor is there a
way to set them.

By disabling the registration and filling of these preferences we save ~40ms on
each renderer startup and ~10ms on each browser start.

http://crbug.com/308095 was spawned to track optimizing this code path for
platforms that use per-script font preferences.

BUG=308033
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229356

[ppi, 2013-10-16 16:09:21]

Could you guys have a look?

[Yaron, 2013-10-16 16:24:13]

https://codereview.chromium.org/27527003/diff/3001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/27527003/diff/3001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:2098: #if !defined(OS_ANDROID)
Theres' some related code in chrome/browser/ui/prefs/prefs_tab_helper.cc to
actually register all of these prefs. Happens once on startup but takes over
10ms last I recall. If we're removing the read of these prefs, can we also
remove the registration?

[tony, 2013-10-16 16:37:48]

If these are rarely set, we should fix the code to be fast on all platforms. 
Instead of using StringPrintf + PrefService::GetString(), we can use
PrefService::GetPreferenceValues to get a root node of the preferences tree and
walk the DictionaryValue.  This should be fast pretty much all the time.

[ppi, 2013-10-16 18:02:35]

Tony, I agree. I spawned
https://code.google.com/p/chromium/issues/detail?id=308095 to track that, but
I'd prefer to address this asynchronously, as we have some (speculative tab
restore) patches blocked on that.

Yaron, please see the reply inline.

https://codereview.chromium.org/27527003/diff/3001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/27527003/diff/3001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:2098: #if !defined(OS_ANDROID)
On 2013/10/16 16:24:13, Yaron wrote:
> Theres' some related code in chrome/browser/ui/prefs/prefs_tab_helper.cc to
> actually register all of these prefs. Happens once on startup but takes over
> 10ms last I recall. If we're removing the read of these prefs, can we also
> remove the registration?

Thanks for the pointer! Actually, it looks like the registration of observers
for these preferences was already disabled for Android by Marcus:
https://chromiumcodereview.appspot.com/12529014 .

[Yaron, 2013-10-16 18:08:53]

https://codereview.chromium.org/27527003/diff/3001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/27527003/diff/3001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:2098: #if !defined(OS_ANDROID)
On 2013/10/16 18:02:36, ppi wrote:
> On 2013/10/16 16:24:13, Yaron wrote:
> > Theres' some related code in chrome/browser/ui/prefs/prefs_tab_helper.cc to
> > actually register all of these prefs. Happens once on startup but takes over
> > 10ms last I recall. If we're removing the read of these prefs, can we also
> > remove the registration?
> 
> Thanks for the pointer! Actually, it looks like the registration of observers
> for these preferences was already disabled for Android by Marcus:
> https://chromiumcodereview.appspot.com/12529014 .

Ya, I measured after the observers were removed. We still register >1k prefs

[tony, 2013-10-16 18:26:49]

On 2013/10/16 18:02:35, ppi wrote:
> Tony, I agree. I spawned
> https://code.google.com/p/chromium/issues/detail?id=308095 to track that, but
> I'd prefer to address this asynchronously, as we have some (speculative tab
> restore) patches blocked on that.

It shouldn't be that much code. I doubt it would take more than an hour to do
properly.  Here, I'll write it for you (not tested):
base::DictionaryValue* pref_map = prefs->GetDictionary(map_name);
base::DictionaryValue::Iterator it(pref_map);
while (!it.IsAtEnd()) {
  std::string script = it.key();
  if (!it.value().empty()) {
    for (size_t i = 0; i < prefs::kWebKitScriptsForFontFamilyMapsLength; ++i) {
      if (script == prefs::kWebKitScriptsForFontFamilyMaps[i]) {
        (*map)[prefs::kWebKitScriptsForFontFamilyMaps[i]] =
UTF8ToUTF16(it.value());
      }
    }
  }
  it.Advance();    
}

Please don't add maintenance burdens for the project if the fix is easy.

[falken, 2013-10-17 08:37:52]

+bauerb, jshin

Since the per-script font prefs are not in a dictionary (each is an independent
key) we can'd do something like
  prefs->GetDictionary(standard_font_family_map);
Instead we'd have to do Tony's first comment of
  prefs->GetPreferenceValues()
to get the entire effective pref store.

If we use GetPreferenceValues(), we probably should do it once in
OverrideWebPrefs and pass to FillFontFamilyMap.

I originally considered putting all the prefs in a dictionary, which likely
would have sped up registration and reading. But the disadvantage was that
changing the default values later on (e.g., adding scripts or changing default
fonts) would not have any affect for users who set one of the prefs. The
discussion of that is here:
https://codereview.chromium.org/7606028

I'm okay with
1. Removing the per-script font pref registration code for Android. If we ever
do add default per-script fonts on Android, we can selectively register only
those prefs.
2. Speeding up FillFontFamilyMap by using GetPreferenceValues (assuming that is
actually fast). Doing (1) first may speed up (2) since the dictionaries would
have just one entry for the common script.

[Bernhard Bauer, 2013-10-17 09:25:26]

On 2013/10/17 08:37:52, falken wrote:
> +bauerb, jshin
> 
> Since the per-script font prefs are not in a dictionary (each is an
independent
> key) we can'd do something like
>   prefs->GetDictionary(standard_font_family_map);
> Instead we'd have to do Tony's first comment of
>   prefs->GetPreferenceValues()
> to get the entire effective pref store.

Technically, there's nothing keeping you from registering an additional
preference with the key kWebKitStandardFontFamilyMap. I'm not sure if that or
GetPreferenceValues() would be the nicer solution.

> If we use GetPreferenceValues(), we probably should do it once in
> OverrideWebPrefs and pass to FillFontFamilyMap.
> 
> I originally considered putting all the prefs in a dictionary, which likely
> would have sped up registration and reading. But the disadvantage was that
> changing the default values later on (e.g., adding scripts or changing default
> fonts) would not have any affect for users who set one of the prefs. The
> discussion of that is here:
> https://codereview.chromium.org/7606028
> 
> I'm okay with
> 1. Removing the per-script font pref registration code for Android. If we ever
> do add default per-script fonts on Android, we can selectively register only
> those prefs.
> 2. Speeding up FillFontFamilyMap by using GetPreferenceValues (assuming that
is
> actually fast). Doing (1) first may speed up (2) since the dictionaries would
> have just one entry for the common script.

[ppi, 2013-10-17 12:07:45]

Thanks, everyone!

In patch set 2 I disabled registration of the preferences altogether on Android.

As to the fix for other platforms, walking the entire preference dictionary
looking for matching preferences (using text matching of the (FONT_FAMILY).
prefix?) doesn't seem appealing - ideally we would keep each family map in
individual dictionary.

In the meantime, we need these 40ms / renderer startup savings - could you guys
have another look at the patch?

[Bernhard Bauer, 2013-10-17 12:12:23]

On 2013/10/17 12:07:45, ppi wrote:
> Thanks, everyone!
> 
> In patch set 2 I disabled registration of the preferences altogether on
Android.
> 
> As to the fix for other platforms, walking the entire preference dictionary
> looking for matching preferences (using text matching of the (FONT_FAMILY).
> prefix?) doesn't seem appealing - ideally we would keep each family map in
> individual dictionary.

Well, you don't need to walk the *entire* pref dictionary. Due to the nested
structure you can simply GetDictionary() with the prefix to get a dictionary
with all the scripts as keys.

> In the meantime, we need these 40ms / renderer startup savings - could you
guys
> have another look at the patch?

Not sure if you need if from me, but LGTM.

[falken, 2013-10-17 12:23:28]

lgtm

https://codereview.chromium.org/27527003/diff/17001/chrome/browser/ui/prefs/p...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/27527003/diff/17001/chrome/browser/ui/prefs/p...
chrome/browser/ui/prefs/prefs_tab_helper.cc:149: // way to set them, so we can
avoid registering them altogether.
Maybe mention that the way to set them on other platforms is through the
extension API, something like:
"On Android, the prefs have no default values and as there are no extensions the
Font Settings API cannot be used to set them, so we can avoid registering them
altogether."

[tony, 2013-10-17 16:43:12]

On 2013/10/17 12:12:23, Bernhard Bauer wrote:
> On 2013/10/17 12:07:45, ppi wrote:
> > Thanks, everyone!
> > 
> > In patch set 2 I disabled registration of the preferences altogether on
> Android.
> > 
> > As to the fix for other platforms, walking the entire preference dictionary
> > looking for matching preferences (using text matching of the (FONT_FAMILY).
> > prefix?) doesn't seem appealing - ideally we would keep each family map in
> > individual dictionary.
> 
> Well, you don't need to walk the *entire* pref dictionary. Due to the nested
> structure you can simply GetDictionary() with the prefix to get a dictionary
> with all the scripts as keys.

Right, that's what my code snippet in comment #6 does.  Also, note that the
reason the existing code is slow is probably because of the prefs->GetString()
call in the loop.  This call has to split the string by periods and then walk
the json object eatch time.  By calling GetDictionary with the prefix, you avoid
all the redundant string splitting and tree walking.

falken, I'm going to assign bug 308095 to you because I don't want us to carry
this #if def forever.  I bet we can somehow get the DictionaryValue for the
default prefs as well.

[ppi, 2013-10-17 16:49:03]

Thanks, everyone!

Tony, sorry I did not get your comments right - I am not familiar with
pref_service.

https://codereview.chromium.org/27527003/diff/17001/chrome/browser/ui/prefs/p...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/27527003/diff/17001/chrome/browser/ui/prefs/p...
chrome/browser/ui/prefs/prefs_tab_helper.cc:149: // way to set them, so we can
avoid registering them altogether.
On 2013/10/17 12:23:29, falken wrote:
> Maybe mention that the way to set them on other platforms is through the
> extension API, something like:
> "On Android, the prefs have no default values and as there are no extensions
the
> Font Settings API cannot be used to set them, so we can avoid registering them
> altogether."

Done.

[commit-bot: I haz the power, 2013-10-17 16:57:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ppi@chromium.org/27527003/26001

[commit-bot: I haz the power, 2013-10-18 01:44:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ppi@chromium.org/27527003/26001

[commit-bot: I haz the power, 2013-10-18 11:18:51]

Change committed as 229356