Bug 17832

Summary:	Memory leaks due to FcStrStaticName use for external patterns
Product:	fontconfig	Reporter:	Karl Tomlinson <bugs.freedesktop>
Component:	library	Assignee:	Karl Tomlinson <bugs.freedesktop>
Status:	RESOLVED FIXED	QA Contact:
Severity:	normal
Priority:	medium	CC:	akira, caolanm, fontconfig-bugs, freedesktop, michael.meeks
Version:	2_1
Hardware:	Other
OS:	All
Whiteboard:
i915 platform:		i915 features:
Attachments:	Replace static strings with reference counted strings Increase the number of buckets in the shared string hash table

Description Karl Tomlinson 2008-09-29 20:27:22 UTC

All strings added to patterns are interned with FcStrStaticName(),
which uses a hash table of size 31, with chaining.

Strings are never uninterned.

This is often fine if all strings come from a finite number of fonts or config
files but when they come from hundreds of web pages memory could become
unconstrained.  Fortunately there are a finite number of font family names and
few malicious web pages trying to throw garbage names.

This may become more of an issue with application fonts that are likely to each have a unique filename.

"fc-match -s sans" caused 286 strings to be interned on my system.

In a browsing session with Mozilla, ~300 strings were interned on startup, and
after viewing ~400 web pages, the total rose only to 530.

It looks like OBJECT_HASH_SIZE (31) was designed to intern fewer strings, so
if string numbers get large performance may suffer.

Also, FcStrHashed() should compare pointers rather than contents of strings when
deciding whether they should be freed.  I guess this is not currently an issue
as all strings are hashed.

Comment 1 Karl Tomlinson 2008-09-29 20:30:57 UTC

(In reply to comment #0)
> Strings are never uninterned.

Well, they are uninterned at FcFini, but I assume the application would only be expected to call FcFini on exit.

Comment 2 Behdad Esfahbod 2010-02-25 11:50:04 UTC

Have any suggested patch?  To enlarge the hash table at least or something?  Otherwise I'd like to close this until it actually becomes a problem.  May be worth a comment in the source file.

Comment 3 Karl Tomlinson 2010-03-02 18:57:45 UTC

(In reply to comment #0)
> Also, FcStrHashed() should compare pointers rather than contents of strings
> when deciding whether they should be freed.

This was fixed in bug 17661, thanks.

Comment 4 Karl Tomlinson 2010-03-02 22:24:23 UTC

Created attachment 33707 [details] [review]
Replace static strings with reference counted strings

With this, the number of strings in the hash table is limited to the number that are currently in use.

Comment 5 Karl Tomlinson 2010-03-02 22:48:56 UTC

Created attachment 33708 [details] [review]
Increase the number of buckets in the shared string hash table

This is a reasonably conservative increase in the number of buckets in the hash table to 251.  After FcInit(), there are 240 shared strings in use on my system (from configuration files I assume).  The hash value is stored in each link in the chains so comparison are actually not very expensive.  This change should reduce the average length of chains by a factor of 8.  With the reference counted strings, it should keep the average length of chains to about 2.  The number of buckets is prime so as not to rely too much on the quality of the hash function.

Comment 6 Caolán McNamara 2012-03-23 03:35:32 UTC

With the patch in comment #4 applied I get (given the random amount of fonts I've got installed) a 145k reduction in LibreOffice memory footprint (idling with a single blank writer document opened)

Comment 7 Behdad Esfahbod 2012-03-24 09:07:11 UTC

Both patches look good.  If Firefox and OO.o run happily on it, it should be good ;).

Comment 8 Akira TAGOH 2012-03-27 21:45:01 UTC

Merged patches in master branch. thanks!

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.