91956 - Eliminate nsFontCache

Status: RESOLVED WONTFIX

(Core :: Graphics, enhancement, P5)

Description

[Randell Jesup [:jesup] (needinfo me)]

We're constantly destroying our fontcache (see backtrace below).  I was tracking
down the number of nsVOidArray allocations and noticed that the number of items
in the font cache dropped from ~20 to 0.  See also bug 77942.

#0  nsFontCache::~nsFontCache (this=0x9e4fd48, __in_chrg=3)
    at nsDeviceContext.cpp:759
#1  0x280b5414 in DeviceContextImpl::~DeviceContextImpl (this=0x9ca1d08, 
    __in_chrg=3) at nsDeviceContext.cpp:108
#2  0x29727849 in nsDeviceContextGTK::~nsDeviceContextGTK (this=0x9ca1d08, 
    __in_chrg=3) at nsDeviceContextGTK.cpp:91
#3  0x2972758b in nsDeviceContextGTK::Release (this=0x9ca1d08)
    at nsDeviceContextGTK.cpp:64
#4  0x280e1fc8 in nsCOMPtr<nsIDeviceContext>::assign_assuming_AddRef (
    this=0x98cec9c, newPtr=0x0) at ../../dist/include/nsCOMPtr.h:471
#5  0x29803c69 in nsCOMPtr<nsIDeviceContext>::assign_with_AddRef (
    this=0x98cec9c, rawPtr=0x0) at ../../dist/include/nsCOMPtr.h:963
#6  0x29804975 in nsCOMPtr<nsIDeviceContext>::operator= (this=0x98cec9c, 
    rhs=0x0) at ../../dist/include/nsCOMPtr.h:583
#7  0x297df62c in nsWebShell::~nsWebShell (this=0x98cec08, __in_chrg=3)
    at nsWebShell.cpp:200
#8  0x297bda33 in nsDocShell::Release (this=0x98cec08) at nsDocShell.cpp:239
#9  0x297df884 in nsWebShell::Release (this=0x98cec08) at nsWebShell.cpp:230
#10 0x807209a in nsCOMPtr<nsIInterfaceRequestor>::~nsCOMPtr (this=0x9db8b40, 
    __in_chrg=2) at ../../dist/include/nsCOMPtr.h:489
#11 0x28de57e0 in nsHttpChannel::~nsHttpChannel (this=0x9db8b08, __in_chrg=3)
    at nsHttpChannel.cpp:96
#12 0x28dec8c7 in nsHttpChannel::Release (this=0x9db8b08)
    at nsHttpChannel.cpp:1521
#13 0x28b8f24c in XPCJSRuntime::GCCallback (cx=0x907cc08, status=JSGC_END)

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

perf, footprint

We probably should usually be using a single nsFontCache, though note that bug
81311 shows xprint/etc needs it's own copy of a (subclass of) nsFontCache.

Assigning to waterson since he did the patch for bug 77942.

Comment 2

[Jarrod Gray]

This could possibly be related to bug 90761 where dbaron says that Mozilla is
missing the font cache. 

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

*** Bug 90761 has been marked as a duplicate of this bug. ***

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

dbaron's comments from bug 90761:

:I just took a profile of a run through ibench, and I'm getting the feeling
we're
:missing the font cache a lot.  In particular, we're missing the font cache
:during paint, which scares me, since it should be cached from reflow.
:
:The push-to-the-front-of-the-list code could also be optimized by using the end
:of the list as the front and *leaving* the AppendElement that we perhaps should
:have changed before.

Comment 5

[rbs]

See "Additional Comments From Erik van der Poel 2001-04-28 13:14" in bug 77942:

"Also, we have one nsFontCache per nsIDeviceContext, and we have one
nsIDeviceContext per DC (on Windows) (I think), so it seems to me that we may
have chosen to put the nsFontCache in the wrong object. I guess we need
multiple DCs, but we don't really need multiple nsFontCaches, do we?

Note that on Unix, the underlying font engine uses a single font cache, so we
are not calling the OS too many times for each nsFont (though we do create
more HFONTs than we need to on Windows).

On Windows, it may be possible to use a single font cache for both the screen
and printing, but this may not be possible on other platforms."

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

bug 90761 was different, and I reopened it.  But making the lifetime of the font
cache longer is probably also a good idea, especially if we can guarantee that
it is invalidated when it needs to be (well, would it ever need to be?).

Comment 7

[rbs]

Yes, e.g., if a new font is installed or an exiting font is removed (bug 69117). 

Comment 8

[Roland Mainz]

jesup wrote:
> We probably should usually be using a single nsFontCache, though note that bug
> 81311 shows xprint/etc needs it's own copy of a (subclass of) nsFontCache.

This is not Xprint module specific... the same applies to PostScript module,
too. The patch in bug 81311 just gets rid of the fontcache replacement code in
printer device classes and uses the standard nsFontCache code instead...

Comment 9

[kill this account]

Will a single nsFontCache mean that a given font metrics object will be
shared (at least within a page)?

A shared font cache would reduce font fill-in lookups which can be expensive
as they sometime end up looking at every font.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

I hope we can share fontmetrics objects (aren't they already shared among an
entire DeviceContext, which is tied to the WebShell)?  Do you have any details
about the current implementation or bug numbers with details?

Comment 11

[kill this account]

I had seen multiple font searches (an area I work on a lot) when a page had
only one font. Turns out on closer inspection that there was one search for
plain text and one search for link text. So 2 searches; not one search per 
element.

Comment 12

[John Morrison]

May God have mercy on us all. The 212 bug spam-o-rama is Now!

Comment 13

[Chris Waterson]

bryner: here's one hyatt has wanted to fix forever. Keep the font cache alive
across multiple pages. If you don't want this bug, kick it back to me and I'll
investigate after sabbatical

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's currently kept alive across multiple pages -- just not multiple webshells.

(Do we have a bug somewhere on the fact that the font cache is indexed by exact
font strings, so we can have many font metrics objects for the same font in the
cache?  It might be good to fix that first.  Perhaps we could move to two
hashtables for the font cache, an outer XP one mapping the string to some opaque
key, and the inner one mapping that key to a real font metrics object.)

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Since we destroy webshells whenever a window is closed (other times?), this
hurts us on the whole window open/close sequence (at the least).  Most of the
window-close tests have used blank windows; this bug implies that for pages with
a number of fonts on them (real-world) it will have more of an impact than in
our tests.  Still probably not a major hit, but might cause a perf impact if
scaled fonts are flushed and have to be rebuilt (I don't know if they are).

Comment 16

[rbs]

This bug is still alive an well. I saw it again under a different form when
dealing with something else (bug 195038 for the curious). I put an assertion in
the destructor ~nsFontMetricsWin(), and saw that the transition from the
zombie-page (or whatever it is called these days) kills the fonts that have been
cached during Reflow(). And this precisely causes those fonts to be missed later
on when Paint() arrives -- as noted in comment #4. The ordering in the cache
isn't critical anymore (it was tuned back then). The zombie stuff seems to be of
greater importance given the current arch (i.e., a font-cache per shell). This
makes me wonder if the layout of the new page is done with the DC belonging to
the old zombie page, which is then killed, together with its DC, causing the
fonts to be recreated when painting.


Stack trace:
============
nsFontMetricsWin::~nsFontMetricsWin() line 414 + 48 bytes
nsFontMetricsWin::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsFontMetricsWin::Release(nsFontMetricsWin * const 0x0375d3b0) line 452 + 215 bytes
nsFontCache::Flush() line 752 + 12 bytes
DeviceContextImpl::FlushFontCache(DeviceContextImpl * const 0x0357bde8) line 574
DocumentViewerImpl::Destroy(DocumentViewerImpl * const 0x032b2648) line 1119
DocumentViewerImpl::Show(DocumentViewerImpl * const 0x03760cc0) line 1371
PresShell::UnsuppressAndInvalidate() line 4941
PresShell::UnsuppressPainting(PresShell * const 0x0378b568) line 4986
DocumentViewerImpl::LoadComplete(DocumentViewerImpl * const 0x03760cc0, unsigned
int 0) line 960
nsDocShell::EndPageLoad(nsIWebProgress * 0x0333933c, nsIChannel * 0x037544b8,
unsigned int 0) line 4334
nsWebShell::EndPageLoad(nsIWebProgress * 0x0333933c, nsIChannel * 0x037544b8,
unsigned int 0) line 807
nsDocShell::OnStateChange(nsDocShell * const 0x02ec523c, nsIWebProgress *
0x0333933c, nsIRequest * 0x037544b8, unsigned int 131088, unsigned int 0) line 4266
nsDocLoaderImpl::FireOnStateChange(nsIWebProgress * 0x0333933c, nsIRequest *
0x037544b8, int 131088, unsigned int 0) line 1226
nsDocLoaderImpl::doStopDocumentLoad(nsIRequest * 0x037544b8, unsigned int 0)
line 864
nsDocLoaderImpl::DocLoaderIsEmpty() line 762
nsDocLoaderImpl::OnStopRequest(nsDocLoaderImpl * const 0x0333932c, nsIRequest *
0x037ad710, nsISupports * 0x00000000, unsigned int 0) line 692
nsLoadGroup::RemoveRequest(nsLoadGroup * const 0x030023d0, nsIRequest *
0x037ad710, nsISupports * 0x00000000, unsigned int 0) line 695 + 35 bytes
PresShell::RemoveDummyLayoutRequest() line 6752 + 42 bytes
PresShell::DoneRemovingReflowCommands() line 6709
PresShell::ProcessReflowCommands(int 1) line 6568
ReflowEvent::HandleEvent() line 6348
HandlePLEvent(ReflowEvent * 0x037ad880) line 6362
PL_HandleEvent(PLEvent * 0x037ad880) line 671 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00f23f90) line 606 + 9 bytes
nsEventQueueImpl::ProcessPendingEvents(nsEventQueueImpl * const 0x00f23e98) line
391 + 12 bytes
nsWindow::DispatchPendingEvents() line 3623
nsWindow::ProcessMessage(unsigned int 512, unsigned int 1, long 5833075, long *
0x0012fc34) line 3971
nsWindow::WindowProc(HWND__ * 0x00100944, unsigned int 512, unsigned int 1, long
5833075) line 1333 + 27 bytes
USER32! 77e12ca8()
USER32! 77e12dc5()
USER32! 77e12f0f()
nsAppShellService::Run(nsAppShellService * const 0x00f67830) line 484
main1(int 1, char * * 0x00262698, nsISupports * 0x00f18030) line 1292 + 32 bytes
main(int 1, char * * 0x00262698) line 1679 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 7c4e87f5()

Comment 17

[Brian Ryner (not reading)]

Just a quick note -- I definitely agree that the font cache is owned by the
wrong object. These should probably be per-media or perhaps per display/screen
(just noticing that some Xft API's care about that, which leads me to think the
results could be different for different screens).

Comment 18

[Brian Ryner (not reading)]

It's worse than this.  We flush (but not destroy) the font cache whenever a new
page is loaded, due to DocumentViewerImpl::Destroy().

Comment 19

[Brian Ryner (not reading)]

I did some tests (without worrying about the bloat implications) of never
flushing the font cache when loading a new page.  The biggest improvement I saw
was on Xft (almost 16% on 5 runs of the pageload test), but I think much of that
has to do with all the time we spend sorting font lists unnecessarily (I'll file
another bug on that).  There was no measurable speedup on gtk1 / non-xft, a very
slight speedup on windows (1% or so), and about a 2% speedup on Mac.

Linux and Windows were tested on a 2 GHz P4 laptop, Mac on a 867 MHz G4.

Comment 20

[jag (Peter Annema)]

bryner, did you ever file that bug and if so, what's the bug #?

Also, would it be worth spending some time on this, or is the gain not that much
anymore these days?

Comment 21

[Boris Zbarsky [:bzbarsky]]

I assume that in the cairo world this bug is no longer an issue?

Comment 22

[Stuart Parmenter]

We have a separate gfxFont cache which is where I would assume most perf wins would come from.  We have this no matter what the cache state of the nsIFontMetrics caching stuff is.  I'm not sure what still uses old font apis and what uses new ones so hard to say what benefits we would get of not clearing this after every document.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We use the old font APIs all the time, e.g. for textframes.

We don't have a gfxFontGroupCache yet, so we need the device contexts' cache of nsThebesFontMetrics. So this bug could still apply.

Comment 24

[Zack Weinberg (:zwol)]

This is on my list of things that remain to be done to get rid of the device context, so claiming.

Comment 25

[Benjamin Smedberg]

Resetting owner to default per Zack's request.

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

Is this still relevant?  I notice bug 651016 is still around and so is nsFontCache in nsDeviceContext.cpp, so probably

Comment 27

[Neil Deakin]

You best ask a graphics person about this.

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

Poking david to vector to whomever is handling font issues in gfx

Comment 29

[David Bolter [:davidb] (NeedInfo me for attention)]

Maybe Jonathan Kew knows...

Comment 30

[Wayne Mery (:wsmwk)]

Or triage owner?

Comment 31

[Jonathan Kew [:jfkthame]]

Roc's comment 23 still holds, I think, despite the years that have passed. It might be nice to simplify away things like nsFontCache and nsDeviceContext some day, but it's not a high priority AFAIK (and we do still need to be caching gfxFontGroups somewhere...)

Comment 32

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 33

[Gregory Pappas [:gregp]]

Is this still relevant? It looks like nsFontCache has moved a bit since comment 31 (bug 1725940)

Comment 34

[Jonathan Kew [:jfkthame]]

(In reply to Gregory Pappas [:gregp] from comment #33)

Is this still relevant? It looks like nsFontCache has moved a bit since comment 31 (bug 1725940)

I think we should just close this - it's not really useful or actionable in its own right. nsFontCache caches the gfxFontGroups that result from resolving a bunch of font-related properties and context, and we definitely still want some such cache.

If/when nsFontCache goes away or gets replaced, I'd expect it'll be as part of a larger restructuring of layout and graphics states and contexts, not just because we arbitrarily decide to eliminate this class. But for now, it serves a purpose and although it's moved and been tweaked now and then, it's not going anywhere.