Improve performance of registering font preferences

Improve performance of registering font preferences
    
Rather than using the preference registry to check which font preferences are already registered keep track locally of which font preferences have defaults and don't attempt to register these a second time.
    

Change-Id: Ib1f9bc3937f230f2250b2fecc2a0590dabb5e2ca

BUG=158818
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165449

[battre, 2012-10-30 12:17:47]

CC'ed mnissler

Hi.

I don't like this change because it opens the path for whole set of new
RegisterPrefs function (every time we add a new aspect, the number of
RegisterPrefs functions doubles.

A more proper way would be to introduce a HasRegisteredPref function, but even
better would be to prevent registering the same preference a second time. Is
there a reason you cannot do that? (I am not familiar with your code).

Best regards,
Dominic

[Mattias Nissler (ping if slow), 2012-10-30 12:47:35]

On 2012/10/30 12:17:47, battre wrote:
> CC'ed mnissler
> 
> Hi.
> 
> I don't like this change because it opens the path for whole set of new
> RegisterPrefs function (every time we add a new aspect, the number of
> RegisterPrefs functions doubles.
> 
> A more proper way would be to introduce a HasRegisteredPref function, but even
> better would be to prevent registering the same preference a second time. Is
> there a reason you cannot do that? (I am not familiar with your code).
> 
> Best regards,
> Dominic

Haven't looked at the code, but the fact that you need to check first whether
you already have registered the pref concerns me. Before we think how we can
make that faster, can please first explain to my why you cannot know whether you
have registered the pref or not? In general the advice is to have a single
well-defined place for pref registrations.

[aberent, 2012-10-30 13:50:48]

On 2012/10/30 12:47:35, Mattias Nissler wrote:
> On 2012/10/30 12:17:47, battre wrote:
> > CC'ed mnissler
> > 
> > Hi.
> > 
> > I don't like this change because it opens the path for whole set of new
> > RegisterPrefs function (every time we add a new aspect, the number of
> > RegisterPrefs functions doubles.
> > 
> > A more proper way would be to introduce a HasRegisteredPref function, but
even
> > better would be to prevent registering the same preference a second time. Is
> > there a reason you cannot do that? (I am not familiar with your code).
> > 
> > Best regards,
> > Dominic
> 
> Haven't looked at the code, but the fact that you need to check first whether
> you already have registered the pref concerns me. Before we think how we can
> make that faster, can please first explain to my why you cannot know whether
you
> have registered the pref or not? In general the advice is to have a single
> well-defined place for pref registrations.

Maybe I need to investigate this further. The performance bottleneck I am
dealing with here is that preferences for something over 1000 fonts are
registered by RegisterFontFamilyMap. I know a few of these (3 or 4) are
duplicates, but I am not clear why or which. The old code checked if each one
was already registered before registering them, as well as checking inside
RegisterPreference, but this was inefficient. 

Do any of you know why the font preferences are registered, or what they are
used for?

[Bernhard Bauer, 2012-10-30 14:34:15]

IIRC, Matt added the font preferences.

https://chromiumcodereview.appspot.com/11307005/diff/6/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_service.cc (left):

https://chromiumcodereview.appspot.com/11307005/diff/6/chrome/browser/prefs/p...
chrome/browser/prefs/pref_service.cc:791: NOTREACHED() << "Tried to register
duplicate pref " << path;
Seeing as this is just a NOTREACHED (which would imply an internal error if it
happens), could we just change it to a DCHECK? That would improve the
performance at least for a Release build.

[falken, 2012-10-30 15:11:07]

On 2012/10/30 14:34:15, Bernhard Bauer wrote:
> IIRC, Matt added the font preferences.

Yes, I added these in http://codereview.chromium.org/7606028/

The font preferences are for different languages (or "scripts"). There is a pref
for each script and for each generic CSS font family. An example is
"webprefs.webkit.fonts.serif.Grek" (serif font for Greek script). With 152
scripts * 7 generic families, that's 1064 prefs.

Each platform has default values for only a small number of font prefs. For
example, Chrome OS comes with a sans-serif Arabic font "Droid Arabic Naskh", so
when it registers webprefs.webkit.fonts.sansserif.Arab, it uses that value.

You can see this code in PrefsTabHelper::RegisterUserPrefs. First, it manually
registers prefs that have a default defined (hardcoded in kFontDefaults). Then,
it goes through all possible font pref strings by looping over scripts and
generic families. For each, if first checks if it was already registered
(meaning it had a default defined). If it hasn't, it registers it.

I imagine there are opportunities to speed this code up.

[Anthony Berent, 2012-10-31 19:57:48]

I am rewriting this and splitting this into two parts:

1) In PrefService::RegisterPreference make the check for the preference already
existing a DCHECK. See  https://codereview.chromium.org/11360014

2) Changes to font preference registration so that it doesn't have check whether
each preference is already registered. Still to be written.

[Anthony Berent, 2012-11-01 12:22:38]

Part 2 is now in the new version of this CL.

[battre, 2012-11-01 14:41:03]

LGTM but you may want to wait for somebody who is more familiar with the fonts
registration code.

Thanks for doing it this way. I think this is significantly cleaner than the
previous approach.

Best regards,
Dominic

https://codereview.chromium.org/11307005/diff/9001/chrome/browser/ui/prefs/pr...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/11307005/diff/9001/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper.cc:127: const std::set<std::string> &
fonts_with_defaults) {
nit: no space before &

[falken, 2012-11-01 15:26:49]

lgtm

http://codereview.chromium.org/11307005/diff/9001/chrome/browser/ui/prefs/pre...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

http://codereview.chromium.org/11307005/diff/9001/chrome/browser/ui/prefs/pre...
chrome/browser/ui/prefs/prefs_tab_helper.cc:135:
prefs->RegisterStringPref(pref_name, "", PrefService::UNSYNCABLE_PREF);
nit: Maybe it's better to put curly braces around this multiline if-body.

[Anthony Berent, 2012-11-01 16:47:49]

https://codereview.chromium.org/11307005/diff/9001/chrome/browser/ui/prefs/pr...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/11307005/diff/9001/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper.cc:127: const std::set<std::string> &
fonts_with_defaults) {
On 2012/11/01 14:41:03, battre wrote:
> nit: no space before &

Done.

https://codereview.chromium.org/11307005/diff/9001/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper.cc:135:
prefs->RegisterStringPref(pref_name, "", PrefService::UNSYNCABLE_PREF);
On 2012/11/01 15:26:49, falken wrote:
> nit: Maybe it's better to put curly braces around this multiline if-body.

Done.

[commit-bot: I haz the power, 2012-11-01 16:48:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/11307005/13001

[commit-bot: I haz the power, 2012-11-01 18:55:16]

Change committed as 165449