Kris Maglione [:kmag] Assignee
	Description • 6 years ago

We currently duplicate the domain string for every instance of every preference callback. This adds up fast, especially since things like the PresShell add many callbacks for every instance. In an empty content process, the overhead is closed to 100K just for names.

nsCString makes it easy to avoid this duplication. For literal strings, it makes it so that we don't need to do any dynamic allocation at all.

	Nicholas Nethercote [inactive]
	Comment 5 • 6 years ago
mozreview-review

Comment on attachment 8989064 [details]
Bug 1472523: Part 1 - Avoid string copies in preference callbacks.

https://reviewboard.mozilla.org/r/254134/#review260860

::: gfx/thebes/gfxPrefs.cpp:285
(Diff revision 1)
>  
>  void gfxPrefs::WatchChanges(const char* aPrefname, Pref* aPref)
>  {
>    MOZ_ASSERT(IsPrefsServiceAvailable());
> -  Preferences::RegisterCallback(OnGfxPrefChanged, aPrefname, aPref);
> +  nsCString name;
> +  name.AssignLiteral(aPrefname, strlen(aPrefname));

I think there's a variant of `AssignLiteral` that doesn't require the length. (Assuming that's right, there are numerous instances in this patch series that can be modified.)

::: xpcom/base/nsDumpUtils.h:113
(Diff revision 1)
>  {
>  public:
>    /**
>     * The name of the preference used to enable/disable the FifoWatcher.
>     */
> -  static const char* const kPrefName;
> +  static const char kPrefName[38];

Please find a way to avoid this magic number.

	Nicholas Nethercote [inactive]
	Comment 6 • 6 years ago
mozreview-review

Comment on attachment 8989065 [details]
Bug 1472523: Part 2 - Avoid unnecessary string copies in preference caches.

https://reviewboard.mozilla.org/r/254136/#review260864

::: modules/libpref/test/gtest/CallbackAndVarCacheOrder.cpp:169
(Diff revision 1)
>    ASSERT_TRUE(closure2.mCalled);
>  }
>  
>  TEST(CallbackAndVarCacheOrder, Bool)
>  {
> -  RunTest<bool>("test_pref.bool.1", "test_pref.bool.2", false, true);
> +  RunTest<bool>(NS_LITERAL_CSTRING("test_pref.bool.1"), NS_LITERAL_CSTRING("test_pref.bool.2"), false, true);

These lines exceed 80 chars now. I have been running `./mach clang-format -p modules/libpref/` on my libpref changes for a while now. Can you please run it on each of your patches? (I see there are already a couple of minor formatting discrepancies on mozilla-central, feel free to include those in the first patch's changes.)

	Nicholas Nethercote [inactive]
	Comment 7 • 6 years ago
mozreview-review

Comment on attachment 8989066 [details]
Bug 1472523: Part 3 - Use the same nsCString for pref callback/observer objects.

https://reviewboard.mozilla.org/r/254138/#review260866

::: modules/libpref/Preferences.cpp:1640
(Diff revision 1)
> +    {
> +      CStringMatcher(nsACString& aStr)
> +        : mStr(aStr)
> +      {}
> +
> +      nsACString& mStr;

It took me a long time to understand how this worked because I overlooked the fact that `mStr` is a reference (which is unusual for a class member). Please add a brief comment about this.

::: modules/libpref/Preferences.cpp:1658
(Diff revision 1)
>      {
>        static PtrMatcher m;
>        return match(m);
>      }
>  
> +    void Get(nsACString& aStr) const

Having `get()` and `Get()` methods in a single class is a bit weird. Maybe the new one should be `get()` as well?

::: modules/libpref/Preferences.cpp:2395
(Diff revision 1)
>  
>    NS_ENSURE_ARG(aDomain);
>    NS_ENSURE_ARG(aObserver);
>  
> +  nsCString domain;
> +  GetPrefName(aDomain).Get(domain);

There are some pre-existing problems with the terminology distinguishing between partial pref paths and full pref paths. Having said that, using "domain" in two different ways in a single line is confusing! In other places the value returned by `GetPrefName()` is called `pref` or `prefName`... please use one of those.

::: modules/libpref/Preferences.cpp:2459
(Diff revision 1)
>  
>    // Remove the relevant PrefCallback from mObservers and get an owning pointer
>    // to it. Unregister the callback first, and then let the owning pointer go
>    // out of scope and destroy the callback.
> -  PrefCallback key(aDomain, aObserver, this);
> +  nsCString domain;
> +  GetPrefName(aDomain).Get(domain);

ditto

	Nicholas Nethercote [inactive]
	Comment 8 • 6 years ago
mozreview-review

Comment on attachment 8989067 [details]
Bug 1472523: Part 4 - Avoid unnecessary domain string duplication in preference observers.

https://reviewboard.mozilla.org/r/254140/#review260868

::: modules/libpref/Preferences.h:263
(Diff revision 1)
> +  }
>  
>    // Adds/Removes two or more observers for the root pref branch. Pass to
>    // aPrefs an array of const char* whose last item is nullptr.
> +  // Note: All preference strings *must* be statically-allocated string
> +  // literals.

I guess there's no way to enforce this?

::: toolkit/components/reputationservice/LoginReputation.cpp:276
(Diff revision 1)
>    LR_LOG(("Enable login reputation service"));
>  
>    nsresult rv = mLoginWhitelist->Init();
>    Unused << NS_WARN_IF(NS_FAILED(rv));
>  
> -  for (const char* pref : kObservedPrefs) {
> +  Preferences::AddStrongObservers(this, kObservedPrefs);

I don't think this change is valid. `AddStrongObservers()` requires the final array element be a null terminator, but `kObservedPrefs` doesn't have that.

::: toolkit/components/reputationservice/LoginReputation.cpp:295
(Diff revision 1)
>  
>    mQueryRequests.Clear();
>  
>    nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
>    if (prefs) {
> -    for (const char* pref : kObservedPrefs) {
> +    Preferences::RemoveObservers(this, kObservedPrefs);

ditto

	Nicholas Nethercote [inactive]
	Comment 9 • 6 years ago

Overall, this looks good. I have a bunch of minor comments above, and I'd like to see the patches again once they are addressed.

Also, I'd like to see before and after numbers from about:memory to get a clear idea of how much memory has been saved. Thanks!

	Kris Maglione [:kmag] Assignee
	Comment 10 • 6 years ago

(In reply to Nicholas Nethercote [:njn] from comment #9)
> Overall, this looks good. I have a bunch of minor comments above, and I'd
> like to see the patches again once they are addressed.
> 
> Also, I'd like to see before and after numbers from about:memory to get a
> clear idea of how much memory has been saved. Thanks!

For all processes, in a fresh session with two about:blank tabs:

Before:

├──────656,072 B (00.30%) -- preferences
│      ├──163,488 B (00.07%) -- callbacks
│      │  ├───86,944 B (00.04%) ── domains
│      │  └───76,544 B (00.03%) ── objects

├─────483,336 B (02.18%) -- preferences
│     ├───71,856 B (00.32%) -- callbacks
│     │   ├──39,312 B (00.18%) ── domains
│     │   └──32,544 B (00.15%) ── objects

├─────485,432 B (02.03%) -- preferences
│     ├───73,856 B (00.31%) -- callbacks
│     │   ├──40,288 B (00.17%) ── domains
│     │   └──33,568 B (00.14%) ── objects

├─────472,208 B (02.70%) -- preferences
│     ├───62,680 B (00.36%) -- callbacks
│     │   ├──34,296 B (00.20%) ── domains
│     │   └──28,384 B (00.16%) ── objects

After:

├──────605,080 B (00.34%) -- preferences
│      ├──113,392 B (00.06%) -- callbacks
│      │  ├──112,944 B (00.06%) ── objects
│      │  └──────448 B (00.00%) ── domains

├─────462,072 B (01.94%) -- preferences
│     ├───50,464 B (00.21%) -- callbacks
│     │   ├──50,016 B (00.21%) ── objects
│     │   └─────448 B (00.00%) ── domains

├─────462,712 B (01.85%) -- preferences
│     ├───51,136 B (00.20%) -- callbacks
│     │   ├──50,688 B (00.20%) ── objects
│     │   └─────448 B (00.00%) ── domains

├─────452,040 B (02.33%) -- preferences
│     ├───42,576 B (00.22%) -- callbacks
│     │   ├──42,576 B (00.22%) ── objects
│     │   └───────0 B (00.00%) ── domains


Though this varies a lot by session, and the numbers go way up as we add more instances of things that add separate observers per instance (e.g., PresShell).

As of now, it seems to save about a third, because the extra 4 bytes of string flag fields bump CallbackNode into the next sized malloc bucket. We could probably save another 15-20% with a custom allocator, or by switching to an array.

	Kris Maglione [:kmag] Assignee
	Comment 11 • 6 years ago
mozreview-review-reply

Comment on attachment 8989067 [details]
Bug 1472523: Part 4 - Avoid unnecessary domain string duplication in preference observers.

https://reviewboard.mozilla.org/r/254140/#review260868

> I guess there's no way to enforce this?

I've been thinking about it. The best I can come up with is to add an assertion that checks that the value isn't in a jemalloc arena in debug builds. I'm not sure it's a good solution, though.

> I don't think this change is valid. `AddStrongObservers()` requires the final array element be a null terminator, but `kObservedPrefs` doesn't have that.

Huh. Right, it works by accident.

	Kris Maglione [:kmag] Assignee
	Comment 12 • 6 years ago
mozreview-review-reply

Comment on attachment 8989064 [details]
Bug 1472523: Part 1 - Avoid string copies in preference callbacks.

https://reviewboard.mozilla.org/r/254134/#review260860

> I think there's a variant of `AssignLiteral` that doesn't require the length. (Assuming that's right, there are numerous instances in this patch series that can be modified.)

There is, but it only accepts string literals/string arrays with a known length. For `char*`, it requires that you pass it a length.

> Please find a way to avoid this magic number.

It's enforced at compile time, since the declaration in nsDumpUtils.cpp has an implicit size from the string constant that needs to match this value, which seemed good enough. I can add a comment to that effect, or I guess I can just make it a macro.

	Kris Maglione [:kmag] Assignee
	Comment 13 • 6 years ago

Just since it was easy enough to check, here's with a custom allocator and free list:

├──────586,616 B (00.30%) -- preferences
│      ├───95,008 B (00.05%) -- callbacks
│      │   ├──94,560 B (00.05%) ── objects
│      │   └─────448 B (00.00%) ── domains

├─────451,992 B (02.04%) -- preferences
│     ├───40,640 B (00.18%) -- callbacks
│     │   ├──40,640 B (00.18%) ── objects
│     │   └───────0 B (00.00%) ── domains

├─────453,368 B (01.90%) -- preferences
│     ├───41,920 B (00.18%) -- callbacks
│     │   ├──41,920 B (00.18%) ── objects
│     │   └───────0 B (00.00%) ── domains

├─────444,944 B (02.52%) -- preferences
│     ├───35,480 B (00.20%) -- callbacks
│     │   ├──35,480 B (00.20%) ── objects
│     │   └───────0 B (00.00%) ── domains

Probably worth doing for an extra 10K per base content process, but seems better as a different bug.

	Kris Maglione [:kmag] Assignee
	Comment 14 • 6 years ago
mozreview-review-reply

Comment on attachment 8989067 [details]
Bug 1472523: Part 4 - Avoid unnecessary domain string duplication in preference observers.

https://reviewboard.mozilla.org/r/254140/#review260868

> I've been thinking about it. The best I can come up with is to add an assertion that checks that the value isn't in a jemalloc arena in debug builds. I'm not sure it's a good solution, though.

I added a debug-only check that the strings are not known to jemalloc.

	Nicholas Nethercote [inactive]
	Comment 19 • 6 years ago
mozreview-review

Comment on attachment 8989067 [details]
Bug 1472523: Part 4 - Avoid unnecessary domain string duplication in preference observers.

https://reviewboard.mozilla.org/r/254140/#review261698

Thank you for the fixes.

	Kris Maglione [:kmag] Assignee
	Comment 20 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/aedd8cf3e783b0d982ed92d01c6996327a71f1cb
Bug 1472523: Part 1 - Avoid string copies in preference callbacks. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e2ddd1d83f11fd222f26a64c6b7c7eea9e6ff93
Bug 1472523: Part 2 - Avoid unnecessary string copies in preference caches. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/506cb0a8e4e278ef54f6e741ca226b8e32d86af9
Bug 1472523: Part 3 - Use the same nsCString for pref callback/observer objects. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/c430c64b47d64406108819996da3e1e00da91cf4
Bug 1472523: Part 4 - Avoid unnecessary domain string duplication in preference observers. r=njn

	Andrei Ciure[:aciure]
	Comment 21 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/aedd8cf3e783
https://hg.mozilla.org/mozilla-central/rev/4e2ddd1d83f1
https://hg.mozilla.org/mozilla-central/rev/506cb0a8e4e2
https://hg.mozilla.org/mozilla-central/rev/c430c64b47d6