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)

defect
Not set
normal

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 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 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 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 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 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 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 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 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 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 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+
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.
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 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+
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 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 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 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 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 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 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 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 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
This is a nice set of changes. As well as the memory reductions, lots of code has been made nicer and duplication removed.
Keywords: leave-open
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 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+
Keywords: leave-open
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
https://hg.mozilla.org/mozilla-central/rev/0825cc9a1c78
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.