PrefsTabHelper watches too many fonts |
||
Issue descriptionBug 452693 made this slightly less bad by watching globally one per profile. But this code still watches 1064 font prefs. This means a PrefObserver with a std::map of 1064 strings to 1064 closures. It also means that the PrefNotifierImpl has 1064 strings mapping to 1064::ObserverList objects containing 1064 vectors of one element each. We should consider adding support for watching all subkeys of a given dictionary. I'm thinking if the key to watch ends in a '.' then all subkeys should be watched. The The PrefNotifierImpl can keep a separate mapping of these (we would expect relatively few) and do a binary search for the changing key. I think we would expect all prefixes of them given key to immediately preceed this search (but I have to think about it more).
,
Sep 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/21cf626af38bb568d740cd8a78efbdc9aaac8222 commit 21cf626af38bb568d740cd8a78efbdc9aaac8222 Author: Brett Wilson <brettw@chromium.org> Date: Thu Sep 07 00:30:20 2017 Add a separate way to notify about font changes. This allows code that needs to observe changes to font family preferences an efficient and convenient way to register for all font change preferences without registering for each one individually. This lowers the total number of preference change observers from ~1200 to 189 for a new profile. Previously this was very inefficient. For all ~1100 font pref names, there were three services that registered for each: the font extensions API, the font family cache, and the prefs tab helper. Each of these had a PrefRegistrar with a map of 1100 entries mapping strings to callabacks. The global notifier has a hash map mapping 1100 strings to unique_ptrs to ObserverList<PrefObserver> containing a vector of 3 pointers. The net allocation savings with this patch is about 500KB, with all of it occurring during startup. There were other designs considered. The most general would be to have a general prefix matching ability for observing changes. But this is difficult to make but efficient and support reentrant changes like the normal observers. Since there are less than 200 registered observers other than fonts, prefix matching seems not to be a common problem so this approach was avoided. Instead, this patch adds a single font changed observer (so that the prefix matching need only be done once globally for each pref change) that interested components registers with. This registers a new type of global low-level notification with the pref service. This could have been made more safe by adding a new registrar type in the prefs service for this purpose, but that would require an additional indirection and extra complexity for each preference change. Bug: 760255 Change-Id: I4629a7e5c95261ad8b15addf663f20c68332d644 Reviewed-on: https://chromium-review.googlesource.com/648617 Commit-Queue: Brett Wilson <brettw@chromium.org> Reviewed-by: Erik Chen <erikchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#500155} [modify] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/base/observer_list.h [modify] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/chrome/browser/BUILD.gn [modify] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/chrome/browser/extensions/api/font_settings/font_settings_api.cc [modify] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/chrome/browser/extensions/api/font_settings/font_settings_api.h [modify] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/chrome/browser/font_family_cache.cc [modify] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/chrome/browser/font_family_cache.h [add] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/chrome/browser/font_pref_change_notifier.cc [add] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/chrome/browser/font_pref_change_notifier.h [add] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/chrome/browser/font_pref_change_notifier_factory.cc [add] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/chrome/browser/font_pref_change_notifier_factory.h [add] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/chrome/browser/font_pref_change_notifier_unittest.cc [modify] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/chrome/browser/ui/prefs/prefs_tab_helper.cc [modify] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/chrome/browser/ui/prefs/prefs_tab_helper.h [modify] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/chrome/common/pref_names_util.h [modify] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/chrome/test/BUILD.gn [modify] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/components/prefs/pref_change_registrar.h [modify] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/components/prefs/pref_notifier_impl.cc [modify] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/components/prefs/pref_notifier_impl.h [modify] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/components/prefs/pref_service.cc [modify] https://crrev.com/21cf626af38bb568d740cd8a78efbdc9aaac8222/components/prefs/pref_service.h
,
Sep 7 2017
|
||
►
Sign in to add a comment |
||
Comment 1 by brettw@chromium.org
, Aug 29 2017