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

Assignee

Description

•

6 years 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)

Review commit: https://reviewboard.mozilla.org/r/252612/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252612/

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/252614/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252614/

Kris Maglione [:kmag]

Assignee

Comment 3

•

6 years 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.

Eric Rahm [:erahm]

Updated

•

6 years ago

Whiteboard: [memshrink] → [Memshrink:P2]

Eric Rahm [:erahm]

Comment 4

•

6 years 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+

Eric Rahm [:erahm]

Comment 5

•

6 years 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-

Kris Maglione [:kmag]

Assignee

Comment 6

•

6 years 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...

Kris Maglione [:kmag]

Assignee

Comment 7

•

6 years 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 on attachment 8987385 [details]
Bug 1470771: Part 1 - Don't evict shared bundles from the bundle cache.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252612/diff/1-2/

Comment hidden (mozreview-request)

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252614/diff/1-2/

Eric Rahm [:erahm]

Comment 10

•

6 years 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+

Kris Maglione [:kmag]

Assignee

Comment 11

•

6 years 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

Kris Maglione [:kmag]

Assignee

Comment 12

•

6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8dd2790ee1d6f87dcb381ec44abdf8a594ed828
Bug 1470771: Follow-up: Fix null deref in Windows GPU process. r=me

Raul Gurzau (:RaulG)

Comment 13

•

6 years 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

Closed: 6 years ago

status-firefox63: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla63

Bug 1470771: Part 1 - Don't evict shared bundles from the bundle cache. 6 years ago Kris Maglione [:kmag] 59 bytes, text/x-review-board-request	erahm : review+	Details
Bug 1470771: Part 2 - Update StringBundle memory reporters to account for sharing. 6 years ago Kris Maglione [:kmag] 59 bytes, text/x-review-board-request	erahm : review+	Details

Bugzilla

Quick Search

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

Categories

(Core :: Internationalization, enhancement)

Tracking

()

People

(Reporter: kmag, Assigned: kmag)

References

Details

(Whiteboard: [Memshrink:P2])

Crash Data

Security

(public)

User Story

Attachments

(2 files)

Description

Comment 1

Comment 2

Comment 3

Updated

Comment 4

Comment 5

Comment 6

Comment 7

Comment 8

Comment 9

Comment 10

Comment 11

Comment 12

Comment 13

Attachment

General

Description

File Name

Content Type