Memory leak in script Close() #480

Merged
merged 1 commit into from Apr 18, 2013

Projects

None yet

3 participants

@mskala
Member
mskala commented Mar 26, 2013

At present, FontViewRemove checks whether the font view being removed is the only one loaded; and if so, it doesn't actually free the memory. There is an explanatory comment that "Freeing a large font can take forever, and if we're just going to exit there's no real reason to do so..." That's okay in a GUI session, because closing the last open font implicitly causes the program to exit. However, this function is called by the scripting Close() command. Scripts can execute in a state of "no font loaded" and can enter and leave that state without exiting FontForge, any number of times. The current code leaks the entire loaded font's memory (which may be tens of megabytes) on every Close() of the only loaded font. Consider a script that opens a large font, closes it, opens it again, closes it, and so on, many times.

Even on my largest fonts (Korean fonts with roughly 13000 complicated glyphs), the amount of time saved by skipping the deallocation is only a second or two, not "forever." It would be less on most fonts. Also note that you only save this for the one font that happens to be closed last, even if many others, and larger, happened to be loaded at the time - all the others but the last, with the current code, have to be deallocated the hard way.

Exacerbating factor: because Revert() doesn't work, script programmers may be inclined to do "Close(); Open();" to simulate its effect, increasing the likelihood of triggering the memory leak.

Workaround for scripts that must work with unpatched FontForge: open a small dummy font at the start of the script, and keep it open throughout, so that you never actually enter the "no fonts loaded" state after that point.

In the future, it may not be just scripts that will leak memory because if we ever comply with standard user interface guidelines, we may allow the program to remain open when there's no font loaded. Then closing the last font to return to the "no font loaded" state will leak that font's memory.

This patch deletes the test for "is this the last font open?" and makes it really deallocate the memory every time. That may make exiting slower. An alternative would be to add a further check for "are we in the GUI?" and skip the free operation if and only if it's the last font AND we are in the GUI. But keeping the deallocation skip in GUI mode will break if in the future the GUI allows closing the last font without exiting; it's only a tiny performance issue anyway; and failing to deallocate the memory because we think we know better already screws up stuff like valgrind memory leak checking. I think this optimization of skipping the deallocation, is a foolish one.

If someone HAD to have the deallocation skip behaviour, the right way to do it would be to test "Are we in the process of exiting the program?" not "Are we in the GUI?", and skip deallocation when in the process of exiting. That could be made more global than just this one check - when we are committed to exit, set a flag that says "don't bother freeing memory anymore," and then have all freeing go through a wrapper that checks that flag. I think this would be a silly thing to do (freeing memory isn't THAT expensive) but it would be the right way to implement such a feature if we thought we needed it.

@coolwanglu
Contributor

+1 for this.
I'm not sure how slow it could be to free a large font.

@mskala
Member
mskala commented Mar 27, 2013

It's a few seconds, verifiable by opening and closing fonts in the new readline interface. Long enough to notice in an interactive context. I think the reason it's so long is because the memory consumption of a font is in lots of little pieces, each separately allocated, so freeing an entire font means doing a lot of pointer-chasing and free() calls on small blocks of memory. I think that's just the cost of doing business, not a problem we need to solve.

@ultrasquid

If this is indeed normal, then we should add a note to the documentation
that this pause is not a cause to panic.

On Wed, Mar 27, 2013 at 5:40 AM, Matthew Skala notifications@github.comwrote:

It's a few seconds, verifiable by opening and closing fonts in the new
readline interface. Long enough to notice in an interactive context. I
think the reason it's so long is because the memory consumption of a font
is in lots of little pieces, each separately allocated, so freeing an
entire font means doing a lot of pointer-chasing and free() calls on small
blocks of memory. I think that's just the cost of doing business, not a
problem we need to solve.


Reply to this email directly or view it on GitHubhttps://github.com/fontforge/fontforge/pull/480#issuecomment-15520818
.

Jason Pagura
zimbach at gmail dot com

@mskala
Member
mskala commented Mar 27, 2013

The case of opening a large font file, and the case of exiting when there is more than one large font file open, each already involve longer pauses. Should we warn users about those as well?

@ultrasquid

In any case where what we've grown to expect as normal from FontForge
differs noticeably from what normal people would expect from a normal
computer program, there should be a note to not panic, with a concise
explanation.

On Wed, Mar 27, 2013 at 8:58 AM, Matthew Skala notifications@github.comwrote:

The case of opening a large font file, and the case of exiting when there
is more than one large font file open, each already involve longer pauses.
Should we warn users about those as well?


Reply to this email directly or view it on GitHubhttps://github.com/fontforge/fontforge/pull/480#issuecomment-15532206
.

Jason Pagura
zimbach at gmail dot com

@coolwanglu
Contributor

@mskala There is a no_windowing_ui in scripting.c, not sure if we can use that variable, or some similar mechanism.

@mskala
Member
mskala commented Mar 27, 2013

I really think that trying to avoid the deallocation is A. a bad idea to do at all, and B. if it's genuinely necessary, it should be conditional on exiting, not conditional on the GUI.

Although I describe a way to do it involving a global "are we exiting?" flag above, on further thought I think the real issue is that this code executes before the program has finally committed to exiting. At some later stage, after this code has executed to close the current file, it's deciding to exit as well. And a problem arises because the current code tries to guess that the exit will eventually happen, before the rest of the program has really committed to exiting. I think the best way to avoid the deallocation would be to change the GUI "close" command to detect when there is only one file open and it doesn't need to be saved, then exit hard with exit() instead of doing a close-file operation. That way neither this particular deallocation, nor any other deallocation, will run. But avoiding deallocation is not an important goal in the first place, and screws up memory-leak detection.

I think that "it takes a few seconds to close a huge file" is not a problem to begin with, and I don't think it differs from what normal people would expect from a normal computer program. Try opening a 20-megabyte word processing file with OpenOffice, close it and get back to your command line prompt in less than a second, and only then tell me that for FontForge to take two or three seconds is a surprise and a problem. The existing code was prematurely optimized.

@mskala mskala merged commit 88ad5f1 into fontforge:master Apr 18, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment