GNOME Bugzilla – Bug 309322
Font memory optimisation
Last modified: 2007-06-27 20:07:52 UTC
Hi, This patch reduces memory per tab by about 600kb/tab (I think). It works by releasing XftFont objects as soon as it's determined they aren't needed, rather than putting them all into the font->fonts list and waiting until exit time to free them. Here are some massif graphs: Before: http://plan99.net/~mike/files/massif.31285.ps After: http://plan99.net/~mike/files/massif.31146.ps I am about 90% sure this patch is correct, but it needs review by somebody who knows VTE internals. thanks -mike
Created attachment 48529 [details] [review] vte memory optimisation patch
Hi Mike, I've applied this patch locally and I'll keep using it. (Seems to work fine here). I attach a slightly changed one that uses the VTE_DEBUG infrastructure for the debug stuff. (So you'll need to recompile vte with --enable-debugging and export VTE_DEBUG_FLAGS=misc to see them)
Created attachment 48556 [details] [review] Changes the above patch to use VTE_DEBUG infrastructure to be more in line with vte source code
Oh whoops, actually the printf could just be deleted, I thought I had got rid of it but clearly not. There's no real reason to have any debug logging there.
Created attachment 48566 [details] [review] Patch without any debug spews
*** Bug 138374 has been marked as a duplicate of this bug. ***
I think this one causes a regression as is. I applied this to my tree locally, and when doing a cat on a file which has some odd characters (i.e. unrecognized sequences in particular. I'll attach the file to this bug) I get a segfault: Program received signal SIGSEGV, Segmentation fault. 0x00173eed in FcPatternPosition (p=0x0, object=0x85ff9a0 "file") at fcpat.c:596 596 high = p->num - 1;
+ Trace 61573
In this case I used the vte app inside vte/src. I'll attach the full bt. Basically we end up setting NULL on a *patternp and then later retrieve it and later use XftFontOpenPattern on it which gives us the crash.
Created attachment 48569 [details] Full backtrace
Created attachment 48570 [details] Here is the file that gives the segfault
Whoops. Try this patch instead, I think I understand what VTE is doing a bit better now. It's not really obvious that the fonts array is index linked to the patterns array.
Created attachment 48574 [details] [review] this one doesn't crash with the given text file nor with copy/pasted chinese characters
Hi Mike, I've applied this one to my local tree and have been using it all day for two days now, without any problems.
Commited. Thanks for your patience :-)
This patch doesn't make any sense, and breaks all font fallback. Reverting...
*** Bug 332580 has been marked as a duplicate of this bug. ***
Lemme elaborate why this change is not correct. It's closing fonts and putting them in the cache. Oops. Just reverting it seems like the best solution at this point to me. We can somehow manage to remember that we have closed this font, so it should be reopened before next use, but that means that we may open/close multiple fonts for every new (say Chinese) character we encounter... Can be disastrous...
2006-02-25 Behdad Esfahbod <behdad@gnome.org> * src/vtexft.c: (_vte_xft_font_for_char): Rever the patch to optimize memory usage by releasing fonts that aren't needed early. Because that was inserting destroyed fonts into cache. Closes bug #332580 and reverts bug #309322.
I'm sure you're correct, as I am not all that familiar with VTE or the Gnome fonts infrastructure, but from looking at the patch I don't see where a destroyed font is inserted into the cache. The code does: loop { ftfont = (load font) if the character exists in the font, add to font->fonts [is this the cache you mean?] break from loop otherwise close the font, ftfont = null } add NULL to font->fonts to keep alignment correct. The code seemed able to handle this NULL in the array just fine. I did test the patch with some chinese text, and didn't see any obvious thrashing. But it clearly does cause a rendering regression and I'd love to know why ...
Uh, the close brace was in the wrong position there. Actually the code in the patch is clear enough. Given the small changes, the only possibility I can see is either inserting NULL (which is just skipped) or inserting an opened font.
Yes, the cache is able to handle NULL's, but closing the font and inserting NULL means that this font will never be used in the future. It's like the font is not there...
Created attachment 62681 [details] [review] new patch I used vte with this patch for a while and it kept only 3 fonts open, not 10 like before. I also fixed a small bug: character from newly open font was not added to fontmap, it is now.
No, No, No. With your patch, a font will be opened and closed again and again... That's quite unacceptable for example when you have a lot of fonts and say cat a Chinese file that may involved thousands of different characters that you don't have any font covering... And you fist have got to show that there's something wrong with keeping fonts open. The original plots linked in the bug report are not available anymore.
any news here?
What kind of news are you looking for? It's marked WONTFIX.