Closed Bug 1569526 Opened 1 year ago Closed 1 year ago

Remove CacheData

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: njn, Assigned: njn)

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 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

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]
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
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.