Remove CacheData
Categories
(Core :: Preferences: Backend, task, P3)
Tracking
()
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)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
libpref's callbacks involve a type called CacheData
, which can be removed. This will simplify things and also reduce memory usage.
Assignee | ||
Comment 1•5 years ago
|
||
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()
.
Assignee | ||
Comment 2•5 years ago
|
||
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
Assignee | ||
Comment 3•5 years ago
|
||
It's unused.
Depends on D39650
Assignee | ||
Comment 4•5 years ago
|
||
It's an annoyingly long name that causes lots of line breaking, and it's not
exposed outside of libpref.
Depends on D39651
Assignee | ||
Comment 5•5 years ago
|
||
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
Assignee | ||
Comment 6•5 years ago
|
||
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
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Comment 8•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Assignee | ||
Comment 9•5 years ago
|
||
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
Updated•5 years ago
|
Comment 10•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Comment 11•5 years ago
|
||
Comment 12•5 years ago
|
||
bugherder |
Comment 13•5 years ago
|
||
bugherder |
Description
•