Closed
Bug 1436863
Opened 7 years ago
Closed 7 years ago
Only send changed prefs to content processes
Categories
(Core :: Preferences: Backend, enhancement)
Core
Preferences: Backend
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
Currently, when a content process is started up, it gets pref values in three different ways:
(1) It gets initial values from data files: default prefs from the omnijar, and user prefs from the user's profile.
(2) The parent process sends the current value of a subset prefs via the command-line argument; these are available immediately, and so are called "early" prefs.
(3) The parent process sends the default and user values of all prefs via IPC. These aren't available until the first IPC message is sent, so these are called "late" prefs.
The key observation is that the pref values sent via (2) and (3) are mostly the same as those obtained by (1), because most prefs don't get changed. If we only send prefs whose value has changed since initialization, we can reduce the number of values sent via (2) from ~300 to ~1, and the number sent via (3) from ~3100 to ~70.
Assignee | ||
Comment 1•7 years ago
|
||
incorrect-see-comment-5 |
One potential problem here: maintaining the list of early prefs is non-trivial. E.g. see bug 1432979 for a case where some prefs were needed on this list, but only used in obscure circumstances. The change suggested by this bug would increase this problem, because we would only detect such cases in debug builds where the relevant pref has changed from the initial value.
Assignee | ||
Comment 2•7 years ago
|
||
Because of the issue in comment 2, the conservative thing would be to only apply this optimization to the prefs sent via IPC.
Comment 3•7 years ago
|
||
Perhaps split this bug into "only send early prefs that have changed since disk" and "only send late prefs that have changed since disk/early"?
I suspect that the two have quite different levels of risk and of benefit; perhaps the 300->1 transition gives a big improvement in content process startup time, and so it might be worth doing the work…
Comment 4•7 years ago
|
||
How about sending all changed prefs at startup? ~300 to ~71 is still improvement, and we don't have to maintain the fragile list anymore.
Assignee | ||
Comment 5•7 years ago
|
||
Comment 1 is wrong: the early pref checking occurs when prefs are *used* not when they are *received* from the parent process. So the fact that fewer pref values are received doesn't matter.
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #4)
> How about sending all changed prefs at startup? ~300 to ~71 is still
> improvement, and we don't have to maintain the fragile list anymore.
I've been contemplating that. It would be *great* to remove the early/late split. See bug 1436911 for more discussion.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8950466 [details]
Bug 1436863 - Factor out several calls to ContentPrefs::GetEarlyPref(i).
https://reviewboard.mozilla.org/r/219726/#review225512
::: dom/ipc/ContentParent.cpp:2009
(Diff revision 1)
> size_t prefsLen;
> ContentPrefs::GetEarlyPrefs(&prefsLen);
>
> for (unsigned int i = 0; i < prefsLen; i++) {
> - MOZ_ASSERT(i == 0 || strcmp(ContentPrefs::GetEarlyPref(i), ContentPrefs::GetEarlyPref(i - 1)) > 0);
> - switch (Preferences::GetType(ContentPrefs::GetEarlyPref(i))) {
> + const char* prefName = ContentPrefs::GetEarlyPref(i);
> + MOZ_ASSERT(i == 0 || strcmp(prefName, ContentPrefs::GetEarlyPref(i - 1)) > 0);
You could possibly keep the value from the previous iteration and check against that instead of using GetEarlyPref(i - 1)
Attachment #8950466 -
Flags: review?(mh+mozilla) → review+
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8950467 [details]
Bug 1436863 - Only send possibly-changed prefs to content processes. .
https://reviewboard.mozilla.org/r/219728/#review226364
::: modules/libpref/Preferences.cpp:350
(Diff revision 1)
> + }
>
> bool HasDefaultValue() const { return mHasDefaultValue; }
> bool HasUserValue() const { return mHasUserValue; }
>
> + // When a content process is created we could pass it info about every pref.
FWIW, I was about to write there must be some word misspelled or misplaced or missing because I couldn't make sense of this sentence, but after reading it a number of times, it sank in. "pass it info" is what somehow confused me.
Attachment #8950467 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 12•7 years ago
|
||
> "pass it info" is what somehow confused me.
I'll change it to this: "When a content process is created we could tell it about every pref."
The use of "tell" matches the next sentence.
Assignee | ||
Comment 13•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/25ce4e94746b43c0cb7998bd9a56364714577c69
Bug 1436863 - Factor out several calls to ContentPrefs::GetEarlyPref(i). r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f9f219db59c3e1d421324699aac0a71eb1b58c8
Bug 1436863 - Only send possibly-changed prefs to content processes. r=glandium.
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/25ce4e94746b
https://hg.mozilla.org/mozilla-central/rev/2f9f219db59c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•7 years ago
|
Assignee: nobody → n.nethercote
You need to log in
before you can comment on or make changes to this bug.
Description
•