Closed Bug 1498316 Opened 1 year ago Closed 1 year ago

Limit the number of nsFontMetrics objects cached by each nsDeviceContext

Categories

(Core :: Graphics: Text, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

Attachments

(1 file, 1 obsolete file)

We cache nsFontMetrics objects (which in turn hold font instances via gfxFontGroup) in the nsFontCache in each device context. This uses a simple array to track the most recently used nsFontMetrics instances for quick retrieval when they are used repeatedly.

Unfortunately, in cases such as animated variation fonts or font sizes, we end up putting thousands -- or even tens of thousands -- of nsFontMetrics instances into this cache, for fractionally different font styles. The cache is only flushed when the device context is destroyed, or on a memory-pressure notification.

This is not useful because (a) it takes up memory and holds fontgroup and font instances alive when they may no longer be needed, and (b) lookups for font metrics that are not actually present (or were not recently used) get slower as the array of cached entries -- which we search linearly -- gets larger.

In practice, most lookups find the nsFontMetrics they want within the last (most recent) few entries in the cache. And once the array exceeds a couple of hundred entries, it's likely (according to instrumentation in my build) to take longer to scan the array for an entry that isn't there, or is far from the end, than to just create a new nsFontMetrics.

So: let's cap the size of the array, and simply discard the oldest entries once it gets too big.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Attachment #9016399 - Flags: review?(lsalzman) → review+
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57e52347a69b
Limit the number of nsFontMetrics entries cached by each device context, to avoid excessive growth of this cache in examples such as animated font variations or sizes. r=lsalzman
Backed out changeset 57e52347a69b (bug 1498316) for leaks in SharedFontList on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/f30aa69dd3b1290de0b8808f44f7c69bdcb6861b

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed,busted,exception&selectedJob=205019804&revision=57e52347a69b17244922a9a2b60be27db3279689

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=205022184&repo=mozilla-inbound&lineNumber=9183

Log snippet: 

[task 2018-10-12T09:26:29.908Z] 09:26:29     INFO - nsTraceRefcnt::DumpStatistics: 1377 entries
[task 2018-10-12T09:26:29.909Z] 09:26:29     INFO - TEST-INFO | leakcheck | tab process: leaked 23 SharedFontList
[task 2018-10-12T09:26:29.911Z] 09:26:29     INFO - TEST-INFO | leakcheck | tab process: leaked 21 ThreadSafeWeakReference<UnscaledFont>
[task 2018-10-12T09:26:29.912Z] 09:26:29     INFO - TEST-INFO | leakcheck | tab process: leaked 8 UnscaledFontFontconfig
[task 2018-10-12T09:26:29.913Z] 09:26:29     INFO - TEST-INFO | leakcheck | tab process: leaked 118 gfxFont
[task 2018-10-12T09:26:29.914Z] 09:26:29     INFO - TEST-INFO | leakcheck | tab process: leaked 60 gfxFontEntry
[task 2018-10-12T09:26:29.915Z] 09:26:29     INFO - TEST-INFO | leakcheck | tab process: leaked 7 gfxFontFamily
[task 2018-10-12T09:26:29.916Z] 09:26:29     INFO - TEST-INFO | leakcheck | tab process: leaked 2496 gfxTextRunFactory
[task 2018-10-12T09:26:29.918Z] 09:26:29     INFO - TEST-INFO | leakcheck | tab process: leaked 2496 nsFontMetrics
[task 2018-10-12T09:26:29.919Z] 09:26:29     INFO - TEST-INFO | leakcheck | tab process: leaked 93 nsStringBuffer
[task 2018-10-12T09:26:29.920Z] 09:26:29     INFO - TEST-INFO | leakcheck | tab process: leaked 18293 nsTArray_base
[task 2018-10-12T09:26:29.921Z] 09:26:29    ERROR - TEST-UNEXPECTED-FAIL | leakcheck | tab process: 569532 bytes leaked (SharedFontList, ThreadSafeWeakReference<UnscaledFont>, UnscaledFontFontconfig, gfxFont, gfxFontEntry, ...)
[task 2018-10-12T09:26:29.923Z] 09:26:29     INFO - 
[task 2018-10-12T09:26:29.925Z] 09:26:29     INFO - == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, tab process 1630
[task 2018-10-12T09:26:29.926Z] 09:26:29     INFO - 
[task 2018-10-12T09:26:29.927Z] 09:26:29     INFO -      |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
[task 2018-10-12T09:26:29.928Z] 09:26:29     INFO -      |                                      | Per-Inst   Leaked|   Total      Rem|
[task 2018-10-12T09:26:29.930Z] 09:26:29     INFO -    0 |TOTAL                                 |       51        0|   29522        0|
[task 2018-10-12T09:26:29.931Z] 09:26:29     INFO - 
[task 2018-10-12T09:26:29.932Z] 09:26:29     INFO - nsTraceRefcnt::DumpStatistics: 429 entries
Flags: needinfo?(jfkthame)
Ugh... that implies we'd probably also leak if a memory-pressure notification gets fired (and flushes the cache). Need to look into that.
Flags: needinfo?(jfkthame)
Oh, never mind: memory-pressure should be OK, as the Flush() method explicitly destroys the nsFontMetrics objects. We need to do the same in the partial-flush here.
Updated patch to properly destroy the nsFontMetrics objects that are being dropped from the cache.
Attachment #9016399 - Attachment is obsolete: true
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4d64ab443ea
Limit the number of nsFontMetrics entries cached by each device context, to avoid excessive growth of this cache in examples such as animated font variations or sizes. r=lsalzman
https://hg.mozilla.org/mozilla-central/rev/b4d64ab443ea
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1500126
You need to log in before you can comment on or make changes to this bug.