Closed Bug 1436863 Opened 7 years ago Closed 7 years ago

Only send changed prefs to content processes

Categories

(Core :: Preferences: Backend, enhancement)

enhancement
Not set
normal

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.
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.
Because of the issue in comment 2, the conservative thing would be to only apply this optimization to the prefs sent via IPC.
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…
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.
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.
(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.
Blocks: 1436911
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 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+
> "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.
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Assignee: nobody → n.nethercote
Blocks: 1451986
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: