1414188 - Remove ValueObserver

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Description

[Nicholas Nethercote [inactive]]

libpref's callback notification system has two layers, which is one too many.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1414188 - Remove ValueObserver. .

libpref has callbacks. You can register a callback function against a
particular pref (or pref prefix), and it'll be called when any matching pref
changes.

This is implemented in two layers. The lower layer is the list of CallbackNode
objects, pointed to by gFirstCallback.

The upper layer involves gObserverTable, which is a hash table of ValueObserver
objects. It is used for callbacks registered via
Preferences::RegisterCallback() and Preferences::Add*VarCache(), but not for
observers registered with Preferences::Add{Weak,Strong}Observer(). If multiple
callbacks with identical prefnames, callback functions and MatchKinds occur,
they are commoned up into a single ValueObserver, which is then wrapped by a
single CallbackNode. (The callbacks within a single ValueObserver can have
different void* aClosure args; ValueObserver keeps those in an nsTArray.)

Note also that gObserverTable is very inelegant, with duplication of data
between the keys and the values due it being a
nsRefPtrHashtable<ValueObserverHashKey, ValueObserver> and ValueObserver being
a subclass of ValueObserverHashKey(!)

This extra layer might make sense if there were a lot of commoning up
happening, but there's not. Across all process kinds there is an average of
between 1.2 and 1.7 closures per ValueObserver. The vast majority have 1
closure, and there are a handful that have multiple; the highest number I've
seen is 71.

(Note that this two-layer design probably seemed more natural back when libpref
was spread across multiple files.)

This patch removes the ValueObserver layer. The only tricky part is that there
is a notion of a MatchKind -- ExactMatch or PrefixMatch -- which exists in the
ValueObserverLayer but not in the CallbackNode layer. So the patch moves the
MatchKind into the CallbackNode layer, which is straightforward.

On Linux64 this reduces memory usage by about 40 KiB in the parent process and
about 30 KiB in each content processes.

The performance implications are minor.

- The list of CallbackNodes is a bit longer, so when a pref is changed we must
  check more nodes. But we avoid possibly doing prefname matching twice.

- Callback registration is much faster, just a simple list prepend, without any
  hash table insertion required.

- Callback unregistration is probably not much different. There is a longer
  list to traverse, but no hash table removal.

- Pref lookup is by far the hottest libpref operation, and it's unchanged.

For perf to be genuinely affected would require (a) a *lot* more
almost-duplicate callbacks that would have been commoned up, and (b) frequent
changing of the pref those callbacks are observing. This seems highly unlikely.

Review commit: https://reviewboard.mozilla.org/r/196128/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196128/

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8924872 [details]
Bug 1414188 - Remove ValueObserver. .

https://reviewboard.mozilla.org/r/196128/#review201642

Comment 3

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/891b6831d29bdfc80fc14434185bfd7c975e45dd
Bug 1414188 - Remove ValueObserver. r=glandium.

Comment 4

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/891b6831d29b