1498316 - Limit the number of nsFontMetrics objects cached by each nsDeviceContext

Status: RESOLVED FIXED

(Core :: Graphics: Text, enhancement)

Description

[Jonathan Kew [:jfkthame]]

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.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: 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

Comment 2

[Pulsebot]

Pushed by [e-mail]:
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

Comment 3

[Raul Gurzau (:RaulG)]

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

Comment 4

[Jonathan Kew [: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.

Comment 5

[Jonathan Kew [: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.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: 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

Updated patch to properly destroy the nsFontMetrics objects that are being dropped from the cache.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/b4d64ab443ea