1462138 - Add a memory reporter for nsStringBundleService

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement, P3)

Description

[Eric Rahm [:erahm]]

As noted in bug 1451568 there's a fair amount of memory allocated for string bundles that isn't reported. We'd like to add a reporter so that this shows up in about:memory reports [0]. There are a couple of steps involved:

Part 1 - Implement and register a reporter

#1 - Make nsStringBundleService implement nsIMemoryReporter
#2 - Add a stubbed out CollectReports method
#3 - Register as a memory reporter in the nsStringBundleService::Init method, unregister in the dtor

Part 2 - Add SizeOfIncludingThis/SizeOfExcludingThis methods for the objects we care about. In this case we care about `mBundleMap` [1] and the values it contains. Here are the classes we'll need to update to add SizeOf methods:
  - bundleCacheEntry_t
    - nsIStringBundle [2], this is generated so we'll have to update the IDL file
    - nsStringBundle [3]
      - nsIPersistentProperties [4], again we need to update the IDL file
      - nsPersistentProperties [5] 

`nsPersistentProperties` is the main piece we need to measure including it's table and it's arena.

Part 3 - Actually report the memory in `CollectReports`

This should just be measuring the memory in nsStringBundleService and reporting it as 'explicit/string-bundles'.

[0] https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Memory_reporting
[1] https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/intl/strres/nsStringBundleService.h#49
[2] https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/intl/strres/nsIStringBundle.idl#22
[3] https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/intl/strres/nsStringBundle.h#18
[4] https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/xpcom/ds/nsIPersistentProperties2.idl#19
[5] https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/xpcom/ds/nsPersistentProperties.h#19

Comment 1

[Eric Rahm [:erahm]]

Kris, can you take a look at this?

Comment 2

[Andrew McCreight [:mccr8]]

Once you've done parts 1 - 3 you should also run a DMD build to verify that you aren't double counting anything, and also that you didn't miss any important sources of memory for the bundle service.

Comment 3

[Eric Rahm [:erahm]]

For the IDL methods we can do something like what nsITimer does [1], which is just adding a virtual method for C++ only.

[1] https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/xpcom/threads/nsITimer.idl#259-261

Comment 4

[Kris Wright :KrisWright]

Attachment: Part 1: Add a memory reporter for nsStringBundleService

Added memory reporter in nsStringBundleService. Added SizeOfIncluding/ExcludingThis functions in IStringBundle, StringBundle, ExtensibleStringBundle, PersistentProperties, StringBundleTextOverride.

Comment 5

[Kris Wright :KrisWright]

Attachment: Part 2: Create a reporter for nsContentUtils string bundles

Created new class 'nsContentUtilsReporter' which reports 'content-utils-string-bundles' allocations that are not a part of the reported StringBundleService bundles

Comment 6

[Eric Rahm [:erahm]]

Comment on attachment 8979314 [details] [diff] [review]
Part 1: Add a memory reporter for nsStringBundleService

Review of attachment 8979314 [details] [diff] [review]:
-----------------------------------------------------------------

Logically looks good, just some code style cleanups and a little simplification.

::: intl/strres/nsIStringBundle.idl
@@ +65,5 @@
>    void asyncPreload();
> +
> +%{C++
> +  virtual size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const = 0;
> +  virtual size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const = 0;

Something we should probably update in the memory reporting doc, we usually don't add *both* functions unless they're needed. For example, if `SizeOfExcludingThis` is never called directly you can just fold it into `SizeOfIncludingThis`. I think we want to do that for all of these.

::: intl/strres/nsStringBundle.cpp
@@ +86,5 @@
> +
> +size_t
> +nsStringBundle::SizeOfIncludingThisIfUnshared(MallocSizeOf aMallocSizeOf) const
> +{
> +  if (mRefCnt == 1) return SizeOfIncludingThis(aMallocSizeOf);

nit: formatting, please use k&r style bracing. Search for control structures in the coding style doc [1]. Please cleanup other instances.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

@@ +419,5 @@
> +size_t
> +nsExtensibleStringBundle::SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const
> +{
> +  size_t n = 0;
> +  const uint32_t sz = mBundles.Count();

nit: please fix the indentation in this function. It might be helpful to add/configure an auto-indenter in your IDE. For example atom has 'atom-beautify' package [1].

There's also the |mach clang-format| command that will cleanup local changes. That's not 100% perfect right now so you'll still need to look over the changes.

[1] https://atom.io/packages/atom-beautify

@@ +555,2 @@
>    flushBundleCache();
> +

nit: empty line

@@ +587,5 @@
> +size_t
> +nsStringBundleService::SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
> +{
> +  size_t n = mBundleMap.ShallowSizeOfExcludingThis(aMallocSizeOf);
> +  for(auto iter = mBundleMap.ConstIter(); !iter.Done(); iter.Next()){

nit: various bits of indentation. Space after `for`, space before `{`, indentation of the following line. See 'control structures' in the coding style doc.

@@ +588,5 @@
> +nsStringBundleService::SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
> +{
> +  size_t n = mBundleMap.ShallowSizeOfExcludingThis(aMallocSizeOf);
> +  for(auto iter = mBundleMap.ConstIter(); !iter.Done(); iter.Next()){
> +  bundleCacheEntry_t  *elem = mBundleMap.Get(iter.Key());

You can just use `iter->Data()`

@@ +589,5 @@
> +{
> +  size_t n = mBundleMap.ShallowSizeOfExcludingThis(aMallocSizeOf);
> +  for(auto iter = mBundleMap.ConstIter(); !iter.Done(); iter.Next()){
> +  bundleCacheEntry_t  *elem = mBundleMap.Get(iter.Key());
> +      n += elem->mBundle->SizeOfIncludingThis(aMallocSizeOf);

Can you also include `mHashKey->SizeOfExcludingThisIfUnshared`

::: intl/strres/nsStringBundleService.h
@@ +42,5 @@
> +  {
> +    size_t amt = SizeOfExcludingThis(MallocSizeOf);
> +
> +    MOZ_COLLECT_REPORT(
> +      "explicit/string-bundles", KIND_HEAP, UNITS_BYTES,

Lets actually call this 'string-bundle-service', since we found more unreported string bundles elsewhere.

@@ +44,5 @@
> +
> +    MOZ_COLLECT_REPORT(
> +      "explicit/string-bundles", KIND_HEAP, UNITS_BYTES,
> +      amt,
> +      "Memory used for string-bundles");

and change the wording here.

::: intl/strres/nsStringBundleTextOverride.h
@@ +32,4 @@
>      NS_DECL_ISUPPORTS
>      NS_DECL_NSISTRINGBUNDLEOVERRIDE
>  
> +    virtual size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const override;

You can remove the virtual prefix when specifying override. Search for 'virtual' in the coding style guides [1].

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

::: xpcom/ds/nsPersistentProperties.cpp
@@ +454,5 @@
> +size_t
> +nsPersistentProperties::SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
> +{
> +  // We still need to measure:
> +  // mIn

You can note that we don't measure this because it's almost always null.

@@ +455,5 @@
> +nsPersistentProperties::SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
> +{
> +  // We still need to measure:
> +  // mIn
> +  // mTable

And remove this since you measure it now.

@@ +458,5 @@
> +  // mIn
> +  // mTable
> +  size_t n = 0;
> +  n += mArena.SizeOfExcludingThis(aMallocSizeOf);
> +  n += mTable.ShallowSizeOfExcludingThis(aMallocSizeOf);

Note that we don't measure the entries themselves since they're allocated out of the arena.

Comment 7

[Eric Rahm [:erahm]]

Comment on attachment 8979315 [details] [diff] [review]
Part 2: Create a reporter for nsContentUtils string bundles

Review of attachment 8979315 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just some minor changes.

::: dom/base/nsContentUtils.cpp
@@ +539,5 @@
> +                 nsISupports* aData, bool aAnonymize) override
> +  {
> +    int64_t amt = 0;
> +    int32_t i;
> +    for (i = 0; i < PropertiesFile_COUNT; ++i){

nit: formatting, space before '{'

@@ +540,5 @@
> +  {
> +    int64_t amt = 0;
> +    int32_t i;
> +    for (i = 0; i < PropertiesFile_COUNT; ++i){
> +      amt += sStringBundles[i]->SizeOfIncludingThisIfUnshared(MallocSizeOf);

Lets null check the string bundle entry just in case.

@@ +545,5 @@
> +    }
> +
> +    MOZ_COLLECT_REPORT("explicit/dom/content-utils-string-bundles", KIND_HEAP, UNITS_BYTES,
> +    amt,
> +    "string-bundles in ContentUtils");

nit: formatting, can you align this with the '('?

::: dom/base/nsContentUtils.h
@@ +3387,4 @@
>  
>    static nsIStringBundleService* sStringBundleService;
>    static nsIStringBundle* sStringBundles[PropertiesFile_COUNT];
> +  class nsContentUtilsReporter;

Is this necessary?

Comment 8

[Kris Wright :KrisWright]

Attachment: Part 1: Add a memory reporter for nsStringBundleService

Added memory reporter in nsStringBundleService. Added SizeOfIncluding/ExcludingThis functions in IStringBundle, StringBundle, ExtensibleStringBundle, PersistentProperties, StringBundleTextOverride.

Comment 9

[Kris Wright :KrisWright]

Attachment: Part 2: Create a reporter for nsContentUtils string bundles

Created new class 'nsContentUtilsReporter' which reports 'content-utils-string-bundles' allocations that are not a part of the reported StringBundleService bundles

Comment 10

[Kris Wright :KrisWright]

Comment on attachment 8979647 [details] [diff] [review]
Part 1: Add a memory reporter for nsStringBundleService

Try results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=882a7b22d5d6f87f38518e876623b55050a413e9

Comment 11

[Kris Wright :KrisWright]

Comment on attachment 8979648 [details] [diff] [review]
Part 2: Create a reporter for nsContentUtils string bundles

Try results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=882a7b22d5d6f87f38518e876623b55050a413e9

Comment 12

[Eric Rahm [:erahm]]

Oops sorry, I r+'d over IRC but just noticed an issue.

Comment 13

[Eric Rahm [:erahm]]

Comment on attachment 8979647 [details] [diff] [review]
Part 1: Add a memory reporter for nsStringBundleService

Review of attachment 8979647 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few more tweaks on this one

::: intl/strres/nsStringBundle.cpp
@@ +79,5 @@
> +
> +size_t
> +nsStringBundle::SizeOfIncludingThisIfUnshared(MallocSizeOf aMallocSizeOf) const
> +{
> +  if (mRefCnt == 1) return SizeOfIncludingThis(aMallocSizeOf);

This still needs to be cleaned up.

::: xpcom/ds/nsPersistentProperties.cpp
@@ +447,5 @@
> +size_t
> +nsPersistentProperties::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
> +{
> +  size_t n = 0;
> +  n += mArena.SizeOfExcludingThis(aMallocSizeOf);

Please still add a comment here that the memory used by the entries in mTable are accounted for in the arena.

Comment 14

[Eric Rahm [:erahm]]

Comment on attachment 8979648 [details] [diff] [review]
Part 2: Create a reporter for nsContentUtils string bundles

Review of attachment 8979648 [details] [diff] [review]:
-----------------------------------------------------------------

Okay looks good, but lets get a dom peer to sign off.

Comment 15

[Kris Wright :KrisWright]

Attachment: Part 1: Add a memory reporter for nsStringBundleService

Added memory reporter in nsStringBundleService. Added SizeOfIncluding/ExcludingThis functions in IStringBundle, StringBundle, ExtensibleStringBundle, PersistentProperties, StringBundleTextOverride.

Comment 16

[Andrew McCreight [:mccr8]]

Comment on attachment 8979648 [details] [diff] [review]
Part 2: Create a reporter for nsContentUtils string bundles

Review of attachment 8979648 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!

::: dom/base/nsContentUtils.cpp
@@ +539,5 @@
> +                 nsISupports* aData, bool aAnonymize) override
> +  {
> +    int64_t amt = 0;
> +    int32_t i;
> +    for (i = 0; i < PropertiesFile_COUNT; ++i) {

It might be a little nicer if you put the declaration of |i| inside the for (ie "for (int32_t i = 0")

@@ +547,5 @@
> +    }
> +
> +    MOZ_COLLECT_REPORT("explicit/dom/content-utils-string-bundles", KIND_HEAP, UNITS_BYTES,
> +                       amt,
> +                       "string-bundles in ContentUtils");

nit: I think the string should be on the same line as |amt|.

::: dom/base/nsContentUtils.h
@@ +3387,4 @@
>  
>    static nsIStringBundleService* sStringBundleService;
>    static nsIStringBundle* sStringBundles[PropertiesFile_COUNT];
> +  class nsContentUtilsReporter;

Ah, I see, you need this even though it is unused, because this is actually inside nsContentUtils, because it has to access a private static of nsContentUtils.

Comment 17

[Kris Wright :KrisWright]

Attachment: Part 2: Create a reporter for nsContentUtils string bundles

Created new class 'nsContentUtilsReporter' which reports 'content-utils-string-bundles' allocations that are not a part of the reported StringBundleService bundles

Comment 18

[Eric Rahm [:erahm]]

Comment on attachment 8979660 [details] [diff] [review]
Part 1: Add a memory reporter for nsStringBundleService

Review of attachment 8979660 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks for updating.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef08139f2c65
Part 1: Add a memory reporter for nsStringBundleService. r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ae824c33c15
Part 2: Create a reporter for nsContentUtils string bundles. r=mccr8

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/ef08139f2c65
https://hg.mozilla.org/mozilla-central/rev/0ae824c33c15

Comment 21

[Ionuț Goldan [:igoldan]]

Perf wins!

== Change summary for alert #13424 (as of Thu, 24 May 2018 22:35:59 GMT) ==

Improvements:

 11%  Base Content Heap Unclassified windows10-64 opt stylo     1,867,524.00 -> 1,657,277.33
 11%  Base Content Heap Unclassified windows10-64 pgo stylo     1,897,623.33 -> 1,687,202.67
  3%  Base Content Heap Unclassified linux64-qr opt stylo       5,564,696.00 -> 5,374,996.67
  3%  Base Content Heap Unclassified linux64 opt stylo          5,568,362.00 -> 5,380,948.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=13424