Closed
Bug 1473631
Opened 6 years ago
Closed 6 years ago
Reduce the number of preference observers defined in normal sessions
Categories
(Core :: Preferences: Backend, defect)
Core
Preferences: Backend
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: kmag, Assigned: kmag)
References
(Blocks 1 open bug)
Details
(Whiteboard: [overhead:4k])
Attachments
(17 files)
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
gfritzsche
:
review+
|
Details |
I'm not sure this is the best component for this, but I can't think of a better one. Preference observers are expensive. Each one requires, at a minimum, 64 bytes for its PrefCallback (plus whatever extra space that takes up in the hashtable) plus 40 bytes for the PrefCallback's CallbackNode. For C++ callers, the obvious and easy solution is migrating to callbacks rather than observers. For JS callers, there are two solutions: 1) We probably need a native helper for defining pref cache properties. I'll file a separate bug about that. 2) For callers that need more than a simple var cache and need to observe multiple prefs, they should define a single prefix observer and check the changed preference name against a map of preferences they care about. Most of them already do this, but define multiple observers anyway. In a fresh session with an about:blank content process, here's our current pref observer usage: ─25,664 B (00.02%) -- observer ├───4,864 B (00.00%) ── nsXPTCStubBase [76] (_startWatchingPrefs@TelemetryEnvironment.jsm:1133) ├───1,472 B (00.00%) ── 0x154250798ce0 [23] ├───1,024 B (00.00%) ── mozilla::net::nsHttpHandler [16] ├─────960 B (00.00%) ── nsCacheProfilePrefObserver [15] ├─────768 B (00.00%) ── nsDNSService [12] ├─────704 B (00.00%) ── 0x1542542b66a8 [11] ├─────576 B (00.00%) ── nsXPTCStubBase [9] (init@chrome://browser/content/browser-feeds.js) ├─────576 B (00.00%) ── mozilla::net::nsSocketTransportService [9] ├─────448 B (00.00%) ── nsCookieService [7] ├─────384 B (00.00%) ── nsXPTCStubBase [6] (startup@AddonManager.jsm) ├─────384 B (00.00%) ── mozilla::net::nsIOService [6] ├─────320 B (00.00%) ── nsXPTCStubBase [5] ├─────256 B (00.00%) ── nsXPTCStubBase [4] ├─────256 B (00.00%) ── nsXPTCStubBase [4] ├─────256 B (00.00%) ── 0x154253daa1c8 [4] ├─────256 B (00.00%) ── nsPrefetchService [4] ├─────256 B (00.00%) ── nsFocusManager [4] ├─────256 B (00.00%) ── mozilla::nsRFPService [4] ├─────256 B (00.00%) ── nsIDNService [4] ├─────256 B (00.00%) ── WatchdogManager [4] ├─────256 B (00.00%) ── mozilla::dom::U2FPrefManager [4] ├─────192 B (00.00%) ── nsXPTCStubBase [3] ├─────192 B (00.00%) ── FontPrefsObserver [3] ... ─4,864 B (00.02%) -- observer ├──1,024 B (00.00%) ── mozilla::net::nsHttpHandler [16] ├────576 B (00.00%) ── mozilla::net::nsSocketTransportService [9] ├────384 B (00.00%) ── mozilla::net::nsIOService [6] ├────256 B (00.00%) ── nsFocusManager [4] ├────256 B (00.00%) ── mozilla::nsRFPService [4] ├────256 B (00.00%) ── nsIDNService [4] ├────256 B (00.00%) ── WatchdogManager [4] ├────192 B (00.00%) ── FontPrefsObserver [3] ├────192 B (00.00%) ── gfxFontListPrefObserver [3] ├────192 B (00.00%) ── nsScriptSecurityManager [3] ├────128 B (00.00%) ── nsSHistoryObserver [2] ├────128 B (00.00%) ── nsNameSpaceManager [2] ├─────64 B (00.00%) ── SRGBOverrideObserver ├─────64 B (00.00%) ── nsXPTCStubBase ├─────64 B (00.00%) ── nsXPTCStubBase ├─────64 B (00.00%) ── nsXPTCStubBase ├─────64 B (00.00%) ── nsPluginHost ├─────64 B (00.00%) ── nsXPTCStubBase ├─────64 B (00.00%) ── nsXPTCStubBase ├─────64 B (00.00%) ── nsXPTCStubBase ├─────64 B (00.00%) ── nsXPTCStubBase ├─────64 B (00.00%) ── nsXPTCStubBase ├─────64 B (00.00%) ── XPCLocaleObserver ├─────64 B (00.00%) ── mozilla::LogModulePrefWatcher ├─────64 B (00.00%) ── mozilla::DataStorage ├─────64 B (00.00%) ── mozilla::DataStorage ├─────64 B (00.00%) ── mozilla::DataStorage └─────64 B (00.00%) ── mozilla::DataStorage
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•6 years ago
|
||
mozreview-review |
Comment on attachment 8992127 [details] Bug 1473631: Part 1 - Replace pref observers with callbacks in nsHttpHandler. https://reviewboard.mozilla.org/r/257012/#review263854 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: netwerk/protocol/http/nsHttpHandler.cpp:1689 (Diff revision 1) > static_cast<int32_t>(mThrottleEnabled)); > } > } > > if (PREF_CHANGED(HTTP_PREF("throttle.version"))) { > - rv = prefs->GetIntPref(HTTP_PREF("throttle.version"), &val); > + rv = Preferences::GetInt(HTTP_PREF("throttle.version"), &val); Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores] rv = Preferences::GetInt(HTTP_PREF("throttle.version"), &val); ^
Comment 19•6 years ago
|
||
mozreview-review |
Comment on attachment 8992125 [details] Bug 1473631: Part 0a - Make preference callbacks typesafe. https://reviewboard.mozilla.org/r/257008/#review263910 +1 for less use of `void*`.
Attachment #8992125 -
Flags: review?(n.nethercote) → review+
Comment 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8992126 [details] Bug 1473631: Part 0b - Add helper for registering instance methods as pref callbacks. https://reviewboard.mozilla.org/r/257010/#review263912 ::: commit-message-9f9fc:24 (Diff revision 1) > + > +The main difficulty is that, until C++ 17, there's no way match a value as a > +template parameter unless you know its complete type, or can at least compute > +its complete type based on earlier template parameters. That means that we > +need a macro to extract the type of the method, and construct the template > +with the full set of explicit parameters. This bit about C++17 is useful info. Should it be in one of the comments below?
Attachment #8992126 -
Flags: review?(n.nethercote) → review+
Comment 21•6 years ago
|
||
mozreview-review |
Comment on attachment 8992127 [details] Bug 1473631: Part 1 - Replace pref observers with callbacks in nsHttpHandler. https://reviewboard.mozilla.org/r/257012/#review263914
Attachment #8992127 -
Flags: review?(n.nethercote) → review+
Comment 22•6 years ago
|
||
mozreview-review |
Comment on attachment 8992128 [details] Bug 1473631: Part 2 - Replace pref observers with callbacks in STS. https://reviewboard.mozilla.org/r/257014/#review263916 ::: netwerk/base/nsSocketTransportService2.cpp (Diff revision 1) > mThread = nullptr; > } > > - nsCOMPtr<nsIPrefBranch> tmpPrefService = do_GetService(NS_PREFSERVICE_CONTRACTID); > + Preferences::UnregisterCallbacks(PrefCallback, gCallbackPrefs, this); > - if (tmpPrefService) > - tmpPrefService->RemoveObserver(SEND_BUFFER_PREF, this); So the old code was failing to unregister most of the observers? Huh.
Attachment #8992128 -
Flags: review?(n.nethercote) → review+
Comment 23•6 years ago
|
||
mozreview-review |
Comment on attachment 8992129 [details] Bug 1473631: Part 3 - Replace pref observers with callbacks in IOService. https://reviewboard.mozilla.org/r/257016/#review263918
Attachment #8992129 -
Flags: review?(n.nethercote) → review+
Comment 24•6 years ago
|
||
mozreview-review |
Comment on attachment 8992128 [details] Bug 1473631: Part 2 - Replace pref observers with callbacks in STS. https://reviewboard.mozilla.org/r/257014/#review263920 Hmm, looks like `nsSocketTransportService::UpdatePrefs()` could be updated to use `Preferences::Get*()` instead of going via `nsIPrefBranch`, if you want to go the extra mile.
Comment 25•6 years ago
|
||
mozreview-review |
Comment on attachment 8992130 [details] Bug 1473631: Part 4 - Replace pref observers with callbacks in FocusManager. https://reviewboard.mozilla.org/r/257018/#review263922
Attachment #8992130 -
Flags: review?(n.nethercote) → review+
Comment 26•6 years ago
|
||
mozreview-review |
Comment on attachment 8992131 [details] Bug 1473631: Part 5 - Replace pref observers with callbacks in RFPService. https://reviewboard.mozilla.org/r/257020/#review263924
Attachment #8992131 -
Flags: review?(n.nethercote) → review+
Comment 27•6 years ago
|
||
mozreview-review |
Comment on attachment 8992133 [details] Bug 1473631: Part 7 - Replace pref observers with callbacks in WatchdogManager. https://reviewboard.mozilla.org/r/257024/#review263928
Attachment #8992133 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 28•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8992126 [details] Bug 1473631: Part 0b - Add helper for registering instance methods as pref callbacks. https://reviewboard.mozilla.org/r/257010/#review263912 > This bit about C++17 is useful info. Should it be in one of the comments below? Oh, yeah. I thought I already had a comment mentioning it.
Assignee | ||
Comment 29•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8992128 [details] Bug 1473631: Part 2 - Replace pref observers with callbacks in STS. https://reviewboard.mozilla.org/r/257014/#review263916 > So the old code was failing to unregister most of the observers? Huh. Yup. Fortunately, we only have one STS instance per session, though, so it didn't wind up making much difference.
Comment 30•6 years ago
|
||
mozreview-review |
Comment on attachment 8992132 [details] Bug 1473631: Part 6 - Replace pref observers with callbacks in IDNService. https://reviewboard.mozilla.org/r/257022/#review263926 ::: netwerk/dns/nsIDNService.cpp:79 (Diff revision 1) > > nsCOMPtr<nsIPrefService> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID)); > if (prefs) > prefs->GetBranch(NS_NET_PREF_IDNWHITELIST, getter_AddRefs(mIDNWhitelistPrefBranch)); > > - nsCOMPtr<nsIPrefBranch> prefInternal(do_QueryInterface(prefs)); > + Preferences::RegisterPrefixCallbacks(PrefChanged, gCallbackPrefs, this); Some patches use `RegisterCallbacks()` and some use `RegisterPrefixCallbacks()`. How did you decide which one to use in each patch?
Attachment #8992132 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 31•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8992132 [details] Bug 1473631: Part 6 - Replace pref observers with callbacks in IDNService. https://reviewboard.mozilla.org/r/257022/#review263926 > Some patches use `RegisterCallbacks()` and some use `RegisterPrefixCallbacks()`. How did you decide which one to use in each patch? I checked all of the callers to make sure I knew whether they actually wanted prefixes (which is all that observers know about) or only cared about exact matches. For the ones that had a mix, I mostly just stuck with prefixes, since that's what the existing code was getting anyway, and it made things simpler.
Comment 32•6 years ago
|
||
mozreview-review |
Comment on attachment 8992134 [details] Bug 1473631: Part 8 - Replace pref observers with callbacks in gfxPlatform. https://reviewboard.mozilla.org/r/257026/#review263940
Attachment #8992134 -
Flags: review?(n.nethercote) → review+
Comment 33•6 years ago
|
||
mozreview-review |
Comment on attachment 8992135 [details] Bug 1473631: Part 9 - Replace pref observers with callbacks in gfxPlatformFontList. https://reviewboard.mozilla.org/r/257028/#review263942
Attachment #8992135 -
Flags: review?(n.nethercote) → review+
Comment 34•6 years ago
|
||
mozreview-review |
Comment on attachment 8992136 [details] Bug 1473631: Part 10 - Replace pref observers with callbacks in ScriptSecurityManager. https://reviewboard.mozilla.org/r/257030/#review263944
Attachment #8992136 -
Flags: review?(n.nethercote) → review+
Comment 35•6 years ago
|
||
mozreview-review |
Comment on attachment 8992137 [details] Bug 1473631: Part 11 - Replace pref observers with callbacks in nsSHistory. https://reviewboard.mozilla.org/r/257032/#review263946
Attachment #8992137 -
Flags: review?(n.nethercote) → review+
Comment 36•6 years ago
|
||
mozreview-review |
Comment on attachment 8992138 [details] Bug 1473631: Part 12 - Replace pref observers with callbacks in nsNameSpaceManager. https://reviewboard.mozilla.org/r/257034/#review263948 ::: dom/base/nsNameSpaceManager.cpp:68 (Diff revision 1) > rv = AddDisabledNameSpace(dont_AddRef(uri), id); \ > NS_ENSURE_SUCCESS(rv, false) > > - mozilla::Preferences::AddStrongObservers(this, kObservedPrefs); > - mMathMLDisabled = mozilla::Preferences::GetBool(kPrefMathMLDisabled); > - mSVGDisabled = mozilla::Preferences::GetBool(kPrefSVGDisabled); > + mozilla::Preferences::RegisterCallbacks( > + PREF_CHANGE_METHOD(nsNameSpaceManager::PrefChanged), > + kObservedPrefs, this); +1 for removing this code duplication.
Attachment #8992138 -
Flags: review?(n.nethercote) → review+
Comment 37•6 years ago
|
||
mozreview-review |
Comment on attachment 8992139 [details] Bug 1473631: Part 13 - Replace pref observers with callbacks in DataStorage. https://reviewboard.mozilla.org/r/257036/#review263950
Attachment #8992139 -
Flags: review?(n.nethercote) → review+
Comment 38•6 years ago
|
||
mozreview-review |
Comment on attachment 8992140 [details] Bug 1473631: Part 14 - Replace pref observers with callbacks in nsCacheService. https://reviewboard.mozilla.org/r/257038/#review263952
Attachment #8992140 -
Flags: review?(n.nethercote) → review+
Comment 39•6 years ago
|
||
mozreview-review |
Comment on attachment 8992141 [details] Bug 1473631: Part 15 - Use a single pref observer for all telemetry environmernt preferences. https://reviewboard.mozilla.org/r/257040/#review263954 ::: commit-message-c74f5:1 (Diff revision 1) > +Bug 1473631: Part 15 - Use a single pref observer for all telemetry environmernt preferences. r?gfritzsche Typo: environment
Comment 40•6 years ago
|
||
This is a nice set of changes. As well as the memory reductions, lots of code has been made nicer and duplication removed.
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Assignee | ||
Comment 41•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf6b1a5f392d5ba60ef4d36487a3fcdb90a7d04a Bug 1473631: Part 0a - Make preference callbacks typesafe. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/bf732e81b820724b8a9a099c7b260ebfa90dc977 Bug 1473631: Part 0b - Add helper for registering instance methods as pref callbacks. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/1049c14941e451ff5921a5c229144ff4cc3d3c06 Bug 1473631: Part 1 - Replace pref observers with callbacks in nsHttpHandler. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/c214e32922d1737ab334c5ee892f82471019f496 Bug 1473631: Part 2 - Replace pref observers with callbacks in STS. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/0e097c8c11ce5b65fa6c90f19d8db43ea753704f Bug 1473631: Part 3 - Replace pref observers with callbacks in IOService. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/ecdda45a59f885cb1e364a80e3a684a64bc7b33a Bug 1473631: Part 4 - Replace pref observers with callbacks in FocusManager. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/d521f084e9918df2c748b35ad85d13309278f7ae Bug 1473631: Part 5 - Replace pref observers with callbacks in RFPService. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/cae48c105b0c6e218e856d3f9044386f43968099 Bug 1473631: Part 6 - Replace pref observers with callbacks in IDNService. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/6ad1956d8e153c9726cb35c838eb0fcefdda3866 Bug 1473631: Part 7 - Replace pref observers with callbacks in WatchdogManager. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/d00a4dc2cc98ce4e08e462d8cdfb957c97ed3dc7 Bug 1473631: Part 8 - Replace pref observers with callbacks in gfxPlatform. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/107fd1544ea99e84ed45d840aeeee6144062716a Bug 1473631: Part 9 - Replace pref observers with callbacks in gfxPlatformFontList. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/576783cbe9cb6d2d726a46213430b93225ed6e58 Bug 1473631: Part 10 - Replace pref observers with callbacks in ScriptSecurityManager. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/e5496d93de3d54e5503406e35e5069a146fe657f Bug 1473631: Part 11 - Replace pref observers with callbacks in nsSHistory. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/59534a1c5946aefbc845437efa8925a53fa289e5 Bug 1473631: Part 12 - Replace pref observers with callbacks in nsNameSpaceManager. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa09d94e4fb19297cd3e80fcc6d14856c898ee8 Bug 1473631: Part 13 - Replace pref observers with callbacks in DataStorage. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/25ab77df83364c1cf96b5ae047cf747e88090b29 Bug 1473631: Part 14 - Replace pref observers with callbacks in nsCacheService. r=njn
Comment 42•6 years ago
|
||
mozreview-review |
Comment on attachment 8992141 [details] Bug 1473631: Part 15 - Use a single pref observer for all telemetry environmernt preferences. https://reviewboard.mozilla.org/r/257040/#review264394 Looks good to me, cheers.
Attachment #8992141 -
Flags: review?(gfritzsche) → review+
Comment 43•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bf6b1a5f392d https://hg.mozilla.org/mozilla-central/rev/bf732e81b820 https://hg.mozilla.org/mozilla-central/rev/1049c14941e4 https://hg.mozilla.org/mozilla-central/rev/c214e32922d1 https://hg.mozilla.org/mozilla-central/rev/0e097c8c11ce https://hg.mozilla.org/mozilla-central/rev/ecdda45a59f8 https://hg.mozilla.org/mozilla-central/rev/d521f084e991 https://hg.mozilla.org/mozilla-central/rev/cae48c105b0c https://hg.mozilla.org/mozilla-central/rev/6ad1956d8e15 https://hg.mozilla.org/mozilla-central/rev/d00a4dc2cc98 https://hg.mozilla.org/mozilla-central/rev/107fd1544ea9 https://hg.mozilla.org/mozilla-central/rev/576783cbe9cb https://hg.mozilla.org/mozilla-central/rev/e5496d93de3d https://hg.mozilla.org/mozilla-central/rev/59534a1c5946 https://hg.mozilla.org/mozilla-central/rev/1aa09d94e4fb https://hg.mozilla.org/mozilla-central/rev/25ab77df8336
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Assignee | ||
Comment 44•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0825cc9a1c786bc538e134fd98346dd6975d719a Bug 1473631: Part 15 - Use a single pref observer for all telemetry environment preferences. r=gfritzsche
Comment 45•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0825cc9a1c78
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in
before you can comment on or make changes to this bug.
Description
•