Many live WebRender font allocations detected by DMD

RESOLVED FIXED in Firefox 64

Status

()

P1
normal
RESOLVED FIXED
2 months ago
27 days ago

People

(Reporter: bholley, Assigned: lsalzman)

Tracking

(Blocks: 1 bug, {memory-leak})

unspecified
mozilla64
Unspecified
Windows
memory-leak
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox62 disabled, firefox63 disabled, firefox64 fixed)

Details

Attachments

(6 attachments)

(Reporter)

Description

2 months ago
I ran my normal browsing under DMD today and got a couple of interesting stacks under WebRender. I'm filing the large font-related ones here.

Technically all this means is that memory reporters don't find this memory (so that it currently shows up as heap-unclassified). But given that we're looking at hundreds of live unreported allocations, I'm somewhat suspicious that we have a leak here.

This may be the same issue as bug 1495661. Getting it on file independently for now, feel free to dupe.
(Reporter)

Comment 1

2 months ago
Created attachment 9013860 [details]
Allocation stacks
(Reporter)

Comment 2

2 months ago
This measurement was done on mac, so this is all in the parent process, but the stacks all come out of the WR worker pool.

Note that there are ~800 of each, which suggests that we're looking at ~800 live fonts.
Flags: needinfo?(lsalzman)
(Reporter)

Updated

2 months ago
Summary: WebRender font allocations detected by DMD → Many live WebRender font allocations detected by DMD
(Reporter)

Updated

2 months ago
Blocks: 1386669
(Reporter)

Comment 3

2 months ago
Created attachment 9013864 [details]
UpdateResources stacks

There are also these stacks in UpdateResources. There are about 800 of them, and it seems plausible that they're for fonts. If you think they aren't, let me know and I can file another bug.
Priority: -- → P3
(Reporter)

Comment 4

a month ago
We'd hoped this was fixed by bug 1495661, but looks like it wasn't. Ryan saw his GPU process balloon to 10GB of ram today. He ran with DMD and got stacks in add_font.
Priority: P3 → P1
(Reporter)

Comment 5

a month ago
Created attachment 9018109 [details]
Ryan's DMD log

Ryan said this corresponded to about 180MB of memory in the GPU process. He'd previously seen it at 10GB, but had to restart to enable DMD. Apparently opening new tabs was a consistent way to grow it.
Lee -- I need you to take bug as your top priority as soon as you see this.  It's a P1 which means "beta blocker."
Jonathan -- to stay on in Beta, we need to get this fixed asap.  ANYTHING you can do is much appreciated.  It's more important than anything else you're working. 
Thanks, both!
Assignee: nobody → lsalzman
Flags: needinfo?(jkew)
Created attachment 9018110 [details]
my about:support

In case, here's my system info. As Bobby said, it seems to grow pretty consistently as I open new tabs and navigate around. Doesn't go down when I close them after. The 180MB log posted here was after less than an hour.
Created attachment 9018264 [details]
another dmd log

Ran my local builds with DMD enabled today (which was thankfully a LOT easier to symbolicate). This is after ~45min uptime and GPU process currently over 1GB of heap-unclassified.
Flags: needinfo?(jkew) → needinfo?(jfkthame)
Ryan, are there particular websites you visit that tend to make the GPU process heap-unclassified grow so much? I'm having trouble reproducing anything nearly as alarming as your reports, although I'm trying to visit sites that use a bunch of fonts (both webfonts and system-installed ones).
Flags: needinfo?(jfkthame) → needinfo?(ryanvm)
Nothing in particular - Treeherder, Slack, GMail (two tabs), Bugzilla, GDocs/GSheets, etc.
Flags: needinfo?(ryanvm)
(Assignee)

Comment 11

a month ago
This is a problem with the dwrote-rs library. It looks like there was some hastily written code to manage DWriteFontFileStreams that, while seemingly working, was not conscientious about freeing memory.

I put up a PR to resolve this: https://github.com/servo/dwrote-rs/pull/22

Some small tweaks in WR will be further necessary to use this.
Flags: needinfo?(lsalzman)
(Assignee)

Comment 12

a month ago
WR PR to use the fixed version of dwrote: https://github.com/servo/webrender/pull/3217
(Assignee)

Comment 13

a month ago
Created attachment 9018422 [details] [diff] [review]
ensure IDWriteFontFileStream stays alive with NativeFontResourceDWrite

It isn't guaranteed to stay alive, so we need to hold it. Noticed this in the process of debugging the leak.
Attachment #9018422 - Flags: review?(jmuizelaar)
Attachment #9018422 - Flags: review?(jmuizelaar) → review+

Comment 14

a month ago
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11f359e1b038
ensure IDWriteFontFileStream stays alive with NativeFontResourceDWrite. r=jrmuizel
(Assignee)

Updated

a month ago
Keywords: leave-open, memory-leak
OS: Unspecified → Windows

Comment 15

a month ago
The WR patch (https://github.com/servo/webrender/pull/3217) has landed now, and will be in the next WR update in Gecko.
Depends on: 1500233
Ryan -- Can you retest this once bug 1500233 lands and mark this fixed if you can't repro?  (Thanks for your help with this one!)
Flags: needinfo?(ryanvm)
Confirmed, GPU process heap-unclassified is stable now for me in the low 20s of MB. No signs of unbounded growth after extensive usage :)
Status: NEW → RESOLVED
Last Resolved: 28 days ago
status-firefox62: --- → disabled
status-firefox63: --- → disabled
status-firefox64: --- → fixed
status-firefox-esr60: --- → unaffected
Flags: needinfo?(ryanvm)
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
\o/ - Thanks, Ryan.   And many thanks, Lee and Jonathan, for all your work on getting fonts working in WR (including in particular this bug).
You need to log in before you can comment on or make changes to this bug.