1569526 - Remove CacheData

Status: RESOLVED FIXED

(Core :: Preferences: Backend, task, P3)

Description

[Nicholas Nethercote [inactive]]

libpref's callbacks involve a type called CacheData, which can be removed. This will simplify things and also reduce memory usage.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1569526 - Add more thread/process assertions to libpref. r=KrisWright

Lots of operations in Preferences.cpp can only occur in the parent process
and/or on the main thread. It has a bunch of assertions to enforce/document
this. This commit adds some more, because they're really useful for
understanding the code.

The commit also removes an unnecessary XRE_IsParentProcess() check in
pref_SetPref() (because that condition is always true, as the added assertion
indicates), and renames a parameter in InitVarCachePrefs().

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Bug 1569526 - Rename `SetPref_*()` as `InitPref_*()`. r=KrisWright

This makes it clear that these run when prefs are initialized (like
InitVarCache()) rather than being vanilla set calls that happen at any
point during runtime.

Depends on D39649

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Bug 1569526 - Remove PrefsIter::Remove. r=KrisWright

It's unused.

Depends on D39650

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Bug 1569526 - Rename `PreferencesInternalMethods`. r=KrisWright

It's an annoyingly long name that causes lots of line breaking, and it's not
exposed outside of libpref.

Depends on D39651

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Bug 1569526 - Remove return values from `Add*VarCache()`. r=KrisWright

They're infallible in practice and always NS_OK. (This stems from
AddVarCacheNoAssignment() always returning NS_OK.)

As a result, the commit removes lots of unnecessary checks.

Depends on D39803

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Bug 1569526 - Don't use default values as fallbacks for VarCache prefs. r=froydnj

A CacheData object holds two things: a VarCache/mirror variable address, and
a default value. The default value is only used if a pref value changes and
then the callback fails to find its value, which can only happen if a pref is
deleted. We can change things so that the mirror variable is untouched in that
case. (In fact, due to bug 343600, callbacks aren't notified upon pref deletion
and so this failure case currently cannot occur!)

With that change in place, CacheData only holds an address, so there's no
need for a distinct heap object for it, and we can eliminate CacheData
entirely and just use the mirror variable address (a void*) directly,
avoiding the need for lots of heap objects.

The removal of the CacheData objects removes one reason for gCacheData to
exist (which is to have an owner for those objects). The other reason is to
detect if two or more prefs get VarCached onto a single variable. But given
that VarCaches are on the way out in favour of static prefs (bug 1448219) this
checking isn't that important. So the commit removes gCacheData as well.
Removing it saves 20-32 KiB per process on 64-bit platforms.

The patch also removes gCacheDataDesc, a diagnostic thing from bug 1276488
that isn't relevant with gCacheData removed. This means the return type of
InitInitialObjects can be simplified.

Finally, the commit renames a few things, taking a step along the path of
renaming VarCache prefs as mirrored prefs, which I think is a much better name
for them.

Depends on D39804

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af046d24fb87
Add more thread/process assertions to libpref. r=KrisWright
https://hg.mozilla.org/integration/autoland/rev/e92f24287067
Rename `SetPref_*()` as `InitPref_*()`. r=KrisWright
https://hg.mozilla.org/integration/autoland/rev/f627f8a8db5d
Remove PrefsIter::Remove. r=KrisWright
https://hg.mozilla.org/integration/autoland/rev/daab458e4910
Rename `PreferencesInternalMethods`. r=KrisWright
https://hg.mozilla.org/integration/autoland/rev/7e970d37369a
Remove return values from `Add*VarCache()`. r=KrisWright

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/af046d24fb87
https://hg.mozilla.org/mozilla-central/rev/e92f24287067
https://hg.mozilla.org/mozilla-central/rev/f627f8a8db5d
https://hg.mozilla.org/mozilla-central/rev/daab458e4910
https://hg.mozilla.org/mozilla-central/rev/7e970d37369a

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: Bug 1569526 - Remove CacheData. r=froydnj

A CacheData object holds two things: a VarCache/mirror variable address, and
a default value. The previous patch removed the use of the default value.
Therefore, CacheData now only holds an address, so there's no need for a
distinct heap object for it, and we can eliminate CacheData entirely and just
use the mirror variable address (a void*) directly.

The removal of the CacheData objects removes one reason for gCacheData to
exist (which is to have an owner for those objects). The other reason is to
detect if two or more prefs get VarCached onto a single variable. But given
that VarCaches are on the way out in favour of static prefs (bug 1448219) this
checking is no longer important. So the commit removes gCacheData as well.

The above changes save 20-32 KiB per process on 64-bit platforms.

The patch also removes gCacheDataDesc, a diagnostic thing from bug 1276488
that isn't relevant with gCacheData removed. This means the return type of
InitInitialObjects can be simplified.

Finally, the commit renames a few things, taking another step along the path of
renaming VarCache prefs as mirrored prefs, a much better name.

Depends on D39805

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14cef049c72d
Don't use default values as fallbacks for VarCache prefs. r=froydnj

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/334875e4ddf4
Remove CacheData. r=froydnj

Comment 12

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/14cef049c72d

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/334875e4ddf4