Closed
Bug 1470771
Opened 6 years ago
Closed 6 years ago
Don't flush cached string bundles with more than one reference
Categories
(Core :: Internationalization, enhancement)
Core
Internationalization
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: kmag, Assigned: kmag)
Details
(Whiteboard: [Memshrink:P2])
Attachments
(2 files)
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) |
Comment hidden (mozreview-request) |
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.
Updated•6 years ago
|
Whiteboard: [memshrink] → [Memshrink:P2]
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+
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-
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...
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 hidden (mozreview-request) |
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+
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
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
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
You need to log in
before you can comment on or make changes to this bug.
Description
•