30382 - Should cache X font information

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P3)

Description

[tor]

The change checked in for bug #30160 replaces the expensive XListFontsWithInfo
by XListFonts, but we're still doing about 200 round trips to the X server for
font querying for a page like http://www.linux.com/.  This seems like an ideal
place for some caching.

Comment 1

[Erik van der Poel]

Yes, we are already caching the results of XListFonts in nsFontMetricsGTK.cpp.
Changing XListFontsWithInfo to XListFonts was a good short-term fix. Thanks!
In the future, we will make nsDeviceContextGTK call nsFontMetricsGTK to reuse
its caching.

Comment 2

[Christopher Blizzard (:blizzard)]

Don't let erik's evil M17 keep you from doing this yourself, tor. :)

Comment 3

[tor]

Attachment: patch to make nsDeviceContextGTK::CheckFontExistence use cached data

Comment 4

[tor]

Here's a first shot at making nsDeviceContextGTK::CheckFontExistence use the
cached information from nsFontMetricsGTK.  Examination of an xscope trace shows
the XListFonts* roundtrips have been eliminated.

Any comments?  Should FindFamilyGTK() be thrown into nsFontMetricsGTK as a
static member to help keep the global namespace tidy?

Comment 5

[tor]

Accidentally removed erik from the recipient list when I took the bug.
Adding him back to the CC list.

Comment 6

[Erik van der Poel]

Hi tor, first of all, thanks for taking on this work. I had a look at the diff,
but that won't work. Take a look at FindFont. It calls XListFonts for whatever
family names are requested first. If none of those fonts contain a glyph for
the character that we are trying to measure/draw, then it will call XListFonts
for the whole list. So if CheckFontExistence is being called for a family that
hasn't been added to the hash table yet, your diff won't work. FindFont doesn't
call XListFonts on the whole list initially because that is a very expensive
operation on some platforms (11 seconds on HP-UX). So I avoid querying the whole
list until I really have to. It is the last resort. So, what you would need to
do is first check the hash table for the presence of that family. If it's there,
then you're done. If it's not there, then you need to call XListFonts on that
family name, and add it to the hash table in the same way that FindFont does.
Hope this makes sense.

Comment 7

[tor]

It sure acts like it works... I'll investigate further.

Comment 8

[tor]

My patch is doing what you describe.  It uses the guts of the existing
FindFamily function.  FindFamily first checks in the gFamilies
hashtable to see if there is already an entry.  If there isn't, it calls
GetFontNames with the font family being queried.  GetFontNames does a
XListFonts with that pattern and adds entries into the appropriate
hastables if it finds anything.  Back in FindFamily, if the font
wasn't found a dummy entry is inserted into the hashtable to avoid
querying the server again.

There was a bug in the previous version of the patch, in which I wasn't
checking for dummy entries.  An updated patch will be attached shortly.

Comment 9

[tor]

Attachment: updated font caching patch

Comment 10

[Erik van der Poel]

You're right. I couldn't really tell from the diff alone, but now that I've
looked at the file itself, I agree.

However, after calling FindFamily, you're now calling TryFamily without checking
for family == nsnull. TryFamily itself just dereferences the family arg, so you
need to make sure that you're passing a non-NULL pointer there (or avoid calling
TryFamily if family is nsnull).

Also, it might be nice if the new function became a static method of the
nsFontMetricsGTK class, called FamilyExists(), perhaps.

Please attach a new patch, for final review. Thanks a bunch! Open source works.

Comment 11

[tor]

I meant to write the nsnull check, really.... A new patch is being attached
that incorporates your naming suggestion.

There is one other small issue that this cache lookup will only work if
FONT_SWITCHING is defined.  For now I added an #ifdef which will emit a
#error if someone tries compiling with that option off.  Should I add the
XListFonts code back into this #ifdef, or is it safe to assume that nobody
is going to compile without FONT_SWITCHING?

Comment 12

[tor]

Attachment: updated patch, now with nsnull check and erik's naming scheme

Comment 13

[Erik van der Poel]

Thanks again. The new patch looks good, but I forgot to mention that the hash
table expects lower-cased family names. It's probably not a good idea to lower
case the string passed in, so we ought to make a copy, lower case it, and then
look it up in the hash table. The names are already being lower-cased for the
other code path (look for ToLowerCase), so we would only need to add lower
casing to the new method FamilyExists().

Also, don't bother with the FONT_SWITCHING stuff. The other side of that ifdef
will be removed soon (hopefully). It should have been removed long ago.

Comment 14

[tor]

Attachment: updated patch - added ToLowerCase, changed to using reference, removed FONT_SWITCHING #error

Comment 15

[Erik van der Poel]

Thanks! r=erik

Comment 16

[Christopher Blizzard (:blizzard)]

Since tor did this in short time, can we get this in for M15, after the beta1
branch is cut?

Comment 17

[Erik van der Poel]

M15, after Beta 1 branch, sounds good to me. Tor, if you agree, please set the
Target Milestone to M15.

Comment 18

[tor]

Isn't the tree going directly to M16 development after the beta1 branch?

Comment 19

[Erik van der Poel]

I don't know. How about setting the target to M15 for now, and waiting for the
Beta 1 branch. Then check in the fix (right after the branch), and mark the
target appropriately at that time.

Comment 20

[chris hofmann]

this sounds like it would be a great candidate to 
go into the tree now.  

If I understand right:
  fairly small change, 
  risk/benefit restricted to linux/unix impliementations, 
  change has gone through several rounds of clean up 
   and evaluation and review.
  possible good performance win on that platform
   (although it doesn seem that we have quanitifible data
   to tell us by how much we would improve. can anyone help on that?)

how much testing, and how much widespread testing converage has been
done?  what kind of regressions might we risk seening if/when this
change gets checked in?

come on guys, get with the program, I say lets put it on the 
beta radar and let the cards fall where they may...

I'm I missing something?  lets try and make some great software
and not get booged down just because the bar for scurtinty on
changes gets adusted highter during some milestones.

Comment 21

[tor]

Addressing your points:

  * scope of change - small code change, but in fairly heavily used function.
    Only affects unix platforms.

  * testing - I have had various versions of the patch in my tree since Sunday
    and haven't seen any obvious regressions during general browsing.  Are 
    there any pages that QA uses to test font handling?

  * possible regressions - erik can probably give a better answer, but my
    understanding is that if the change fails then mozilla will just fall
    back to the next font in its list of alternatives.

  * performance win - I think mozilla feels a little "snappier", but don't
    have hard numbers to back that up.  This will be a bigger win for people
    running with remote X displays.

Comment 22

[Erik van der Poel]

I've reviewed the change carefully, and I don't think there will be any
regressions. The only info that's missing is the benefit info. We don't have
any numbers for the performance enhancement.

Comment 23

[tor]

Moving off beta1 radar and pushing off to M16 for now.  These changes have some
problems I'm working on resolving.

Comment 24

[Phil Peterson]

PDT-  Sounds like a great improvement for M15

Comment 25

[tor]

Attachment: updated patch with initialization changes (see comment)

Comment 26

[tor]

New patch needed to fix a crash when running the profile manager.  It was
possible for FindFamily to get called before FindFont had initialized the
needed hash tables.  This patch moves the initialization into a seperate
InitFontTable() static method which is called when needed.

Comment 27

[tor]

Attachment: updated patch - move InitFontTables from static class method to static function in nsFontMetricsGTK.cpp

Comment 28

[Erik van der Poel]

I'm sorry that my review didn't catch the crashing problem. I should have run
the code myself. I ran it today, and it seems fine. However, I noticed that you
are returning nsnull from InitFontTables(). It would be nice if that function
did error checking for all of the memory allocations (including hash table
creation), and returned an error condition if something went wrong. The callers
would then also have to look at the return value.

However, at this point, I feel that I am asking too much of you. If you'd rather
not do all that yourself, feel free to leave it to me. And thanks for all the
work you've done so far.

Comment 29

[Erik van der Poel]

Tim, I haven't heard from you for a while. Would you mind if I reassigned this
bug back to myself, and used your code as a starting point to finish this?

Comment 30

[tor]

Attachment: update patch to tip

Comment 31

[tor]

Feel free to take this bug - I don't have time to work on mozilla stuff
right now.  I've attached an updated patch that applies to the current tree.

Comment 32

[Erik van der Poel]

Thanks for the new patch. Re-assigning bug back to myself.

Comment 33

[Erik van der Poel]

Added Tim's code, and tweaked it a bit.