Closed Bug 1569526 Opened 5 years ago Closed 5 years ago

Remove CacheData

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [MemShrink:P2][overhead:20-32K])

Attachments

(7 files)

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

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().

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

It's unused.

Depends on D39650

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

Depends on D39651

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

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

Keywords: leave-open
Pushed by nnethercote@mozilla.com: 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
Whiteboard: [MemShrink][overhead:20-32K]
Depends on: 1570212
Depends on: 1571544
Attachment #9081489 - Attachment description: Bug 1569526 - Remove CacheData. r=kmag → 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 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

Whiteboard: [MemShrink][overhead:20-32K] → [MemShrink:P2][overhead:20-32K]
Pushed by nnethercote@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/14cef049c72d Don't use default values as fallbacks for VarCache prefs. r=froydnj
Keywords: leave-open
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: