Don't flush cached string bundles with more than one reference

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

unspecified
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [Memshrink:P2])

Attachments

(2 attachments)

Assignee

Description

11 months ago
The string bundle cache currently flushes cached bundles on memory-pressure, or when entries need to be evicted so we can add new ones. Unfortunately, it flushes bundles even when there are external references which will keep them alive. Which means that if those bundles are requested again, we wind up with multiple copies.

Since just about all consumers of the string bundle service keep long-lived references to the bundles they load, this winds up happening a lot. Particularly for bundles cached by nsContentUtils, which are never flushed, and often loaded by other consumers (e.g., new browser windows).

We should solve this by only evicting bundles when they're only being held alive by the bundle cache.

(Note: Bug 1470365 improves this situation somewhat by never evicting shared memory bundles, since their memory is held alive by other processes, and needs to remain mapped for the lifetime of each process once its data has been accessed anyway)
Comment hidden (mozreview-request)
Assignee

Comment 3

11 months ago
Memory reports after changes:

Parent:

127,888 B - string-bundles
├─69,792 B ─ nsStringBundle(url="chrome://browser/locale/browser.properties", shared=true, refCount=8, heapSize=69792)
├──8,352 B ─ nsStringBundle(url="chrome://browser/locale/customizableui/customizableWidgets.properties", shared=true, refCount=4, heapSize=8352)
├──6,304 B ─ nsStringBundle(url="chrome://pocket/locale/pocket.properties", shared=true, refCount=7, heapSize=6304)
├──5,280 B ─ nsStringBundle(url="chrome://global/locale/extensions.properties", shared=true, refCount=4, heapSize=5280)
├──3,728 B ─ service
├──3,232 B ─ nsStringBundle(url="chrome://browser/locale/search.properties", shared=true, refCount=4, heapSize=3232)
├──3,232 B ─ nsStringBundle(url="chrome://browser/locale/tabbrowser.properties", shared=true, refCount=7, heapSize=3232)
├──3,232 B ─ nsStringBundle(url="chrome://devtools-startup/locale/key-shortcuts.properties", shared=true, refCount=4, heapSize=3232)
├──3,232 B ─ nsStringBundle(url="chrome://devtools-startup/locale/startup.properties", shared=true, refCount=4, heapSize=3232)
├──3,232 B ─ nsStringBundle(url="chrome://global-platform/locale/platformKeys.properties", shared=true, refCount=4, heapSize=3232)
├──3,232 B ─ nsStringBundle(url="chrome://global/locale/aboutReader.properties", shared=true, refCount=4, heapSize=3232)
├──3,232 B ─ nsStringBundle(url="chrome://global/locale/autocomplete.properties", shared=true, refCount=6, heapSize=3232)
├──3,232 B ─ nsStringBundle(url="chrome://global/locale/plugins.properties", shared=true, refCount=4, heapSize=3232)
├──3,232 B ─ nsStringBundle(url="chrome://weave/locale/sync.properties", shared=true, refCount=4, heapSize=3232)
├──3,232 B ─ nsStringBundle(url="chrome://webcompat-reporter/locale/webcompat.properties", shared=true, refCount=4, heapSize=3232)
├────128 B ─ SharedStringBundle(url="chrome://branding/locale/brand.properties", shared=true, refCount=5, heapSize=128, sharedMemorySize=260)
├────128 B ─ SharedStringBundle(url="chrome://global/locale/commonDialogs.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=1894)
├────128 B ─ SharedStringBundle(url="chrome://global/locale/css.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=22402)
├────128 B ─ SharedStringBundle(url="chrome://global/locale/dom/dom.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=51802)
├────128 B ─ SharedStringBundle(url="chrome://global/locale/intl.properties", shared=false, refCount=2, heapSize=128, sharedMemorySize=336)
├────128 B ─ SharedStringBundle(url="chrome://global/locale/layout_errors.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=5318)
├────128 B ─ SharedStringBundle(url="chrome://global/locale/layout/HtmlForm.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=2236)
├────128 B ─ SharedStringBundle(url="chrome://global/locale/layout/htmlparser.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=21308)
├────128 B ─ SharedStringBundle(url="chrome://global/locale/mathml/mathml.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=1756)
├────128 B ─ SharedStringBundle(url="chrome://global/locale/printing.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=2732)
├────128 B ─ SharedStringBundle(url="chrome://global/locale/security/security.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=15730)
├────128 B ─ SharedStringBundle(url="chrome://global/locale/svg/svg.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=154)
├────128 B ─ SharedStringBundle(url="chrome://global/locale/xbl.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=1550)
├────128 B ─ SharedStringBundle(url="chrome://global/locale/xul.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=1104)
├────128 B ─ SharedStringBundle(url="chrome://necko/locale/necko.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=3392)
├─────64 B ─ nsStringBundle(url="chrome://browser/locale/sitePermissions.properties", shared=true, refCount=4, heapSize=64)
├─────64 B ─ nsStringBundle(url="chrome://pipnss/locale/nsserrors.properties", shared=true, refCount=3, heapSize=64)
└─────64 B ─ nsStringBundle(url="chrome://pipnss/locale/pipnss.properties", shared=true, refCount=3, heapSize=64)


131,974 B (100.0%) -- shared-string-bundles
├───51,802 B (39.25%) ── SharedStringBundle(url="chrome://global/locale/dom/dom.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=51802)
├───22,402 B (16.97%) ── SharedStringBundle(url="chrome://global/locale/css.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=22402)
├───21,308 B (16.15%) ── SharedStringBundle(url="chrome://global/locale/layout/htmlparser.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=21308)
├───15,730 B (11.92%) ── SharedStringBundle(url="chrome://global/locale/security/security.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=15730)
├────5,318 B (04.03%) ── SharedStringBundle(url="chrome://global/locale/layout_errors.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=5318)
├────3,392 B (02.57%) ── SharedStringBundle(url="chrome://necko/locale/necko.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=3392)
├────2,732 B (02.07%) ── SharedStringBundle(url="chrome://global/locale/printing.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=2732)
├────2,236 B (01.69%) ── SharedStringBundle(url="chrome://global/locale/layout/HtmlForm.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=2236)
├────1,894 B (01.44%) ── SharedStringBundle(url="chrome://global/locale/commonDialogs.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=1894)
├────1,756 B (01.33%) ── SharedStringBundle(url="chrome://global/locale/mathml/mathml.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=1756)
├────1,550 B (01.17%) ── SharedStringBundle(url="chrome://global/locale/xbl.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=1550)
├────1,104 B (00.84%) ── SharedStringBundle(url="chrome://global/locale/xul.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=1104)
├──────336 B (00.25%) ── SharedStringBundle(url="chrome://global/locale/intl.properties", shared=false, refCount=2, heapSize=128, sharedMemorySize=336)
├──────260 B (00.20%) ── SharedStringBundle(url="chrome://branding/locale/brand.properties", shared=true, refCount=5, heapSize=128, sharedMemorySize=260)
└──────154 B (00.12%) ── SharedStringBundle(url="chrome://global/locale/svg/svg.properties", shared=true, refCount=3, heapSize=128, sharedMemorySize=154)


Web child:

19,824 B - string-bundles
├─16,544 B ─ nsStringBundle(url="chrome://onboarding/locale/onboarding.properties", shared=false, refCount=2, heapSize=16544)
├──1,936 B ─ service
├────128 B ─ SharedStringBundle(url="chrome://branding/locale/brand.properties", shared=true, refCount=4, heapSize=128)
├────128 B ─ SharedStringBundle(url="chrome://global/locale/intl.properties", shared=false, refCount=2, heapSize=128)
├────128 B ─ SharedStringBundle(url="chrome://necko/locale/necko.properties", shared=false, refCount=2, heapSize=128)
├─────80 B ─ SharedStringBundle(url="chrome://global/locale/commonDialogs.properties", shared=false, refCount=2, heapSize=80)
├─────80 B ─ SharedStringBundle(url="chrome://global/locale/css.properties", shared=false, refCount=2, heapSize=80)
├─────80 B ─ SharedStringBundle(url="chrome://global/locale/dom/dom.properties", shared=false, refCount=2, heapSize=80)
├─────80 B ─ SharedStringBundle(url="chrome://global/locale/layout_errors.properties", shared=false, refCount=2, heapSize=80)
├─────80 B ─ SharedStringBundle(url="chrome://global/locale/layout/HtmlForm.properties", shared=false, refCount=2, heapSize=80)
├─────80 B ─ SharedStringBundle(url="chrome://global/locale/layout/htmlparser.properties", shared=false, refCount=2, heapSize=80)
├─────80 B ─ SharedStringBundle(url="chrome://global/locale/mathml/mathml.properties", shared=false, refCount=2, heapSize=80)
├─────80 B ─ SharedStringBundle(url="chrome://global/locale/printing.properties", shared=false, refCount=2, heapSize=80)
├─────80 B ─ SharedStringBundle(url="chrome://global/locale/security/security.properties", shared=false, refCount=2, heapSize=80)
├─────80 B ─ SharedStringBundle(url="chrome://global/locale/svg/svg.properties", shared=false, refCount=2, heapSize=80)
├─────80 B ─ SharedStringBundle(url="chrome://global/locale/xbl.properties", shared=false, refCount=2, heapSize=80)
└─────80 B ─ SharedStringBundle(url="chrome://global/locale/xul.properties", shared=false, refCount=2, heapSize=80)

That onboarding bundle is pretty annoying, but will go away after bug 1469072.


Before changes:

Parent:

│    ├────188,448 B (00.03%) ── content-utils-string-bundles
├──────151,040 B (00.03%) ── string-bundle-service


Web child:

│        ├──191,680 B (00.01%) ── content-utils-string-bundles
├─────────44,672 B (00.00%) ── string-bundle-service


It's interesting to note that the shared memory bundles use about a third less
memories than the standard implementation, despite its use of the arena
allocator to try to avoid fragmentation.

Updated

11 months ago
Whiteboard: [memshrink] → [Memshrink:P2]

Comment 4

11 months ago
mozreview-review
Comment on attachment 8987385 [details]
Bug 1470771: Part 1 - Don't evict shared bundles from the bundle cache.

https://reviewboard.mozilla.org/r/252612/#review260164

Looks good, just some minor changes.

::: intl/strres/nsStringBundle.cpp:655
(Diff revision 1)
>                    nsIMemoryReporter)
>  
>  nsStringBundleService::~nsStringBundleService()
>  {
>    UnregisterWeakMemoryReporter(this);
> -  flushBundleCache();
> +  flushBundleCache(false);

nit: `/*whatever =*/ false` here and below.

::: intl/strres/nsStringBundle.cpp:849
(Diff revision 1)
>  nsStringBundleService::insertIntoCache(already_AddRefed<nsIStringBundle> aBundle,
>                                         const nsCString &aHashKey)
>  {
> -  bundleCacheEntry_t *cacheEntry;
> +  UniquePtr<bundleCacheEntry_t> cacheEntry;
>  
> -  if (mBundleMap.Count() < MAX_CACHED_BUNDLES ||
> +  if (mBundleMap.Count() >= MAX_CACHED_BUNDLES) {

This check is kinda busted now that we have `SharedStringBundle`s. I guess I should have caught that in the other review. I suppose we could add a field to track the size of the cache.
Attachment #8987385 - Flags: review?(erahm) → review+

Comment 5

11 months ago
mozreview-review
Comment on attachment 8987386 [details]
Bug 1470771: Part 2 - Update StringBundle memory reporters to account for sharing.

https://reviewboard.mozilla.org/r/252614/#review260726

Looks good, thanks for taking the time to add reporting for all bundles. Just a few minor issues.

::: intl/strres/nsStringBundle.cpp:130
(Diff revision 1)
> -    return mTarget->SizeOfIncludingThis(aMallocSizeOf) + aMallocSizeOf(this);
> +    return aMallocSizeOf(this);
>    }
>  
>    size_t SizeOfIncludingThisIfUnshared(mozilla::MallocSizeOf aMallocSizeOf) const override
>    {
> -    size_t size = mTarget->SizeOfIncludingThisIfUnshared(aMallocSizeOf);
> +    return mRefCnt > 1 ? SizeOfIncludingThis(aMallocSizeOf) : 0;

This check is reversed.

::: intl/strres/nsStringBundle.cpp:382
(Diff revision 1)
> +  path.AppendLiteral("(url=\"");
> +  path.Append(escapedURL);
> +
> +  // We have an extra ref from the memory reporter service, at this point.
> +  path.AppendLiteral("\", shared=");
> +  path.AppendASCII(mRefCnt > 2 ? "true" : "false");

You used RegisterWeak so there shouldn't be an extra ref.

::: intl/strres/nsStringBundle.cpp:388
(Diff revision 1)
> +
> +  path.AppendLiteral(", refCount=");
> +  path.AppendInt(mRefCnt);
> +
> +  path.AppendLiteral(", heapSize=");
> +  path.AppendInt(heapSize);

We're already reporting the heap size below, maybe leave this off?
Attachment #8987386 - Flags: review?(erahm) → review-
Assignee

Comment 6

11 months ago
mozreview-review-reply
Comment on attachment 8987385 [details]
Bug 1470771: Part 1 - Don't evict shared bundles from the bundle cache.

https://reviewboard.mozilla.org/r/252612/#review260164

> This check is kinda busted now that we have `SharedStringBundle`s. I guess I should have caught that in the other review. I suppose we could add a field to track the size of the cache.

Yeah, I considered that. It's why I added the `isEmpty` check in the earlier patch.

I think counting the shared bundles in the max cache size is probably fine. It matters in the parent process at least. Probably less in the content, but we really don't want unshared string bundles in the content process if we can help it...
Assignee

Comment 7

11 months ago
mozreview-review-reply
Comment on attachment 8987386 [details]
Bug 1470771: Part 2 - Update StringBundle memory reporters to account for sharing.

https://reviewboard.mozilla.org/r/252614/#review260726

> This check is reversed.

Yeah, I fixed this when you made the same comment on the patch that added this method.

> You used RegisterWeak so there shouldn't be an extra ref.

The service holds an extra ref while it's calling memory reporter functions

> We're already reporting the heap size below, maybe leave this off?

Yeah, I'm not sure it's super helpful. I added it so the heap size would also show up in the separate shared bundles list where we report mmapped size, but I don't know how much it matters.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

11 months ago
mozreview-review
Comment on attachment 8987386 [details]
Bug 1470771: Part 2 - Update StringBundle memory reporters to account for sharing.

https://reviewboard.mozilla.org/r/252614/#review260806
Attachment #8987386 - Flags: review?(erahm) → review+
Assignee

Comment 11

11 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/967409251f5cfdb7acc7616f6da2774677854aeb
Bug 1470771: Part 1 - Don't evict shared bundles from the bundle cache. r=erahm

https://hg.mozilla.org/integration/mozilla-inbound/rev/34f927a06700696cb78789e9e77770054556212c
Bug 1470771: Part 2 - Update StringBundle memory reporters to account for sharing. r=erahm

Comment 13

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/967409251f5c
https://hg.mozilla.org/mozilla-central/rev/34f927a06700
https://hg.mozilla.org/mozilla-central/rev/c8dd2790ee1d
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.