SetFont() Leaks Memory
none
I have a CStatic that fills the client area of a CWnd. In the OnSize()
member of the CWnd, I resize the CStatic to keep it filling the client
area. No problems here.
I decided I'd like for the text on the CStatic to resize so that the
characters are roughly the same height as the client area, so I added
this code in the OnSize() handler:
// Get the control's current font
LOGFONT lf;
m_label.GetFont()->GetLogFont(&lf);
// client_height is just client_rect.bottom - client_rect.top
lf.lfHeight = -client_height;
lf.lfWidth = 0;
CFont f;
f.CreateFontIndirect(&lf);
m_label.SetFont(&f);
f.Detach();
Using CreateFontIndirect() and Detach() like this is kind of awkward,
but it's the only way I could think of to preserve all the existing
font's settings and change only the size.
This appears to work beautifully, but if I open task manager and look at
my process, it just continuously consumes more and more memory. I know
that this code is the problem because I can remove it and the memory
usage stays constant.
I thought (incorrectly, apparently) that doing the Detach() would allow
the font to be destroyed the next time it was changed. Does SetFont()
not destroy the existing font object before setting a new one? Am I
expected to do something like DeleteObject() on the old font?
I know that the usual suggestion is to make a CFont object a member of
the window class, but I think I would run into the same problem...
actually, I'm not sure how to change the size of an existing CFont
object.
Thanks
none
> I thought (incorrectly, apparently) that doing the Detach() would allow
> the font to be destroyed the next time it was changed. Does SetFont()
> not destroy the existing font object before setting a new one? Am I
> expected to do something like DeleteObject() on the old font?
A little more experimentation and I found that this really is what's
happening. I added a TRACE() statement to display the value of
::GetGuiResources(::GetCurrentProcess(), GR_GDIOBJECTS))
each time the OnSize() handler is called, and the number just continuously
increases. I guess I can add a DeleteObject() call to get rid of the old
font object, but that just seems to go against the design of MFC.
David Lowndes
>not destroy the existing font object before setting a new one?
No, the control doesn't own the font object - it could be shared by
lots of controls.
>Am I
>expected to do something like DeleteObject() on the old font?
Yes.
Dave
none
I gave this a try and got unexpected results. I added a GetFont() call to
get the old font before the SetFont() call to set the new one... Then
called DeleteObject() on the old font, but this causes any other controls
that were displaying the initial, non-resized font to revert to some sort
of strange terminal/system font!
So, it looks like I'll have to check all other controls to make sure they
aren't using the font object before calling DeleteObject(). Joy!
David Lowndes
>>>expected to do something like DeleteObject() on the old font?
>>
>> Yes.
>
>I gave this a try and got unexpected results. I added a GetFont() call to
>get the old font before the SetFont() call to set the new one... Then
>called DeleteObject() on the old font, but this causes any other controls
>that were displaying the initial, non-resized font to revert to some sort
>of strange terminal/system font!
Which is pretty much what I'd expect.
You should only delete a font that you've created - you'll need to
revise your code to only delete the last font that you created. If you
just keep the single font object around you should be able to do it
quite neatly.
Dave
none
> You should only delete a font that you've created - you'll need to
> revise your code to only delete the last font that you created. If you
> just keep the single font object around you should be able to do it
> quite neatly.
Ah, this is much smarter than what I was about to attempt (keeping track of
all the font objects NOT to delete). Each time I create one of these
temporary resized fonts, now, I just store the handle and delete it if I
come through OnSize() again. It seems to work, but there is still some
sort of memory leak. I verified that no GDI objects are leaking, but if I
just continuously resize the window, I see a leak of somewhere around 1KB
per second in task manager. This is much smaller than the amount of memory
that was leaking when I wasn't deleting the GDI objects, so I'm tempted to
just live with it and put a note in the documentation: "Don't resize the
window continuously for more than an hour or so." :)
Maybe CFont::CreateFontIndirect() allocates something that isn't cleaned up
when the CFont object is destroyed?
Thanks for helping.
Joseph M. Newcomer
If you need a new font, it would be created in the OnPaint/OnDraw handler. There is no
conceivable reason to create it or set it in the OnSize handler.
Note that since you never delete the font (because of the Detach), every time OnSize is
called, it creates a NEW font.
Please explain your goal.
There are many other problems with this code
joe
On Mon, 25 Jul 2011 20:19:10 GMT, none <no...@none.none> wrote:
>Hi all,
>
>I have a CStatic that fills the client area of a CWnd. In the OnSize()
>member of the CWnd, I resize the CStatic to keep it filling the client
>area. No problems here.
>
>I decided I'd like for the text on the CStatic to resize so that the
>characters are roughly the same height as the client area, so I added
>this code in the OnSize() handler:
>
>
> // Get the control's current font
> LOGFONT lf;
> m_label.GetFont()->GetLogFont(&lf);
****
Note that there is no reason to expect that GetFont returns a non-NULL object.
****
>
> // client_height is just client_rect.bottom - client_rect.top
> lf.lfHeight = -client_height;
****
Then you should write
lf.lfHeight = - client_rect.Height();
and there is no conceivable reason to have stored client_height anywhere that is not a
local variable, but since it makes no sense to do this in the OnSize handler, the correct
code in OnDraw/OnPaint would be
CRect client;
GetClientRect(&client);
...other stuff
lf.lfHeight = -client.Height();
It is poor practice to show variables in your sample code unless you give the type and the
scope. You showed neither.
****
> lf.lfWidth = 0;
>
> CFont f;
> f.CreateFontIndirect(&lf);
>
> m_label.SetFont(&f);
>
> f.Detach();
>
>
>Using CreateFontIndirect() and Detach() like this is kind of awkward,
>but it's the only way I could think of to preserve all the existing
>font's settings and change only the size.
****
This is quite normal practice.
****
>
>This appears to work beautifully, but if I open task manager and look at
>my process, it just continuously consumes more and more memory. I know
>that this code is the problem because I can remove it and the memory
>usage stays constant.
****
Yes, because in fact you consume more and more memory!
The SetFont absolutely, positively does NOT belong in the OnSize handler.
You would create the font in the OnDraw/OnPaint, select it into the DC, then deselect it
and destroy it (explicitly or implicitly)
void CWhatever::OnPaint()
{
CPaintDC dc(this);
...
CFont * f = GetFont();
if(f != NULL)
{ /* handle font change */
CFont newfont;
CRect client;
GetClientRect(&client);
LOGFONT lf;
f->GetLogFont(&lf);
lf.lfHeight = -client.Height();
int save = dc.SaveDC();
dc.SelectObject(&newfont);
...
dc.RestoreDC(save);
} /* handle font change */
There is no sane reason to change the font in the OnSize handler.
****
>
>I thought (incorrectly, apparently) that doing the Detach() would allow
>the font to be destroyed the next time it was changed. Does SetFont()
>not destroy the existing font object before setting a new one? Am I
>expected to do something like DeleteObject() on the old font?
****
Absolutely not. All the Detach() does is prevent the font from being destroyed by
CFont::~CFont(), and has absolute NO other effect. And there is no valid reason to EVER
destroy a font when a new font is selected, because that font might be used in dozens of
other places, or needs to be used again some other time. Your expectation of deletion on
selecting a new font is wrong
****
>
>I know that the usual suggestion is to make a CFont object a member of
>the window class, but I think I would run into the same problem...
>actually, I'm not sure how to change the size of an existing CFont
>object.
****
I would make it a local variable of the OnDraw/OnPaint handler, as I show above.
There is no way to change the size of an existing font object.
joe
****
>
>Thanks
Joseph M. Newcomer [MVP]
email: newc...@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
David Lowndes
>temporary resized fonts, now, I just store the handle and delete it if I
>come through OnSize() again.
That sounds more reasonable - you only create a single font object and
delete it prior to creating the new one.
>It seems to work, but there is still some
>sort of memory leak. I verified that no GDI objects are leaking, but if I
>just continuously resize the window, I see a leak of somewhere around 1KB
>per second in task manager.
Perhaps there's some other code that has a problem?
If you minimize the application window, does some memory get
reclaimed?
Do you see any leaks reported by the VC++ run-time when you close the
app down when you run the debug build under the debugger?
Dave
none
> There is no sane reason to change the font in the OnSize handler.
Well, it may not be sane, but my reasoning is that OnSize() is called when
the window size changes, and that's when I want the font size to change. I
would assume that there's considerable overhead involved in creating a
font, setting it, detaching, deleting the object, etc... so I don't really
want to do that on every repaint. The window is displaying "live" data, so
it is repainting constantly. Sizing only happens as a result of user
interaction with the window, so that's not the performance-critical path.
And, as I mentioned, there is some memory leaking resulting from the
constant creation/destruction of the fonts (even with the GDI object leak
fixed), so moving the code to OnPaint() would be really bad unless this
secondary leak can be fixed. Unless I have a bug elsewhere (entirely
possible), your sample OnPaint() code will exhibit this same slow memory
leak.
none
>>It seems to work, but there is still some
>>sort of memory leak. I verified that no GDI objects are leaking, but
>>if I just continuously resize the window, I see a leak of somewhere
>>around 1KB per second in task manager.
>
> Perhaps there's some other code that has a problem?
No, it's definitely this section of code, because I can comment out just
those lines and there is no memory leak, no matter how long I resize the
window. I'll try to isolate the exact line that causes the leak, but I
can't imagine it being anything other than the CreateFontIndirect().
> If you minimize the application window, does some memory get
> reclaimed?
Oddly enough, no.
> Do you see any leaks reported by the VC++ run-time when you close the
> app down when you run the debug build under the debugger?
No.
David Lowndes
>those lines and there is no memory leak
OK, show the code you now have, perhaps someone can spot something.
Dave
none
> David Lowndes wrote:
>
>>>It seems to work, but there is still some
>>>sort of memory leak. I verified that no GDI objects are leaking, but
>>>if I just continuously resize the window, I see a leak of somewhere
>>>around 1KB per second in task manager.
>>
>> Perhaps there's some other code that has a problem?
>
> No, it's definitely this section of code, because I can comment out just
> those lines and there is no memory leak, no matter how long I resize the
> window. I'll try to isolate the exact line that causes the leak, but I
> can't imagine it being anything other than the CreateFontIndirect().
I'm really surprised... The problem has something to do with
CStatic::GetFont() and CStatic::SetFont()! I've reduced my OnSize()
handler to just this:
// Call base handler
CDialog::OnSize(nType, cx, cy);
// m_label is just a CStatic member of this dialog class
if (::IsWindow(m_label.GetSafeHwnd()))
{
CFont f;
f.CreateFont(30, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, "Arial");
m_label.GetFont();
m_label.SetFont(&f);
}
Yes, I am aware that this code is ridiculous and useless because "f" is
immediately destroyed along with the newly created GDI font object, but the
code above is all it takes to cause a memory leak. Explicitly calling
Detach() and DeleteObject() does not affect the leak. However, if I remove
*either* the GetFont() call or the SetFont() call, the memory leak goes
away!
David Lowndes
>CStatic::GetFont() and CStatic::SetFont()! I've reduced my OnSize()
>handler to just this:
> m_label.GetFont();
> m_label.SetFont(&f);
What's your real code? The standard CWnd GetFont/SetFont methods can't
be used like that.
Dave
none
>> m_label.GetFont();
>> m_label.SetFont(&f);
>
> What's your real code? The standard CWnd GetFont/SetFont methods can't
> be used like that.
That's the exact, "real" code that I ran to reproduce the error. If you
create a CDialog based project and copy/paste that same code into an OnSize
handler, you'll see the memory leak.
The code that I am now using to resize the font makes those same two calls,
in that order, but does a few other things between the two... What's
illegal about calling GetFont() to get the old font and SetFont() to set a
new one?
David Lowndes
>>> m_label.SetFont(&f);
>>
>> What's your real code? The standard CWnd GetFont/SetFont methods can't
>> be used like that.
>
>That's the exact, "real" code that I ran to reproduce the error.
What type is your m_label variable?
CWnd::SetFont is defined as:
void SetFont(
CFont* pFont,
BOOL bRedraw = TRUE
);
and CWnd::GetFont is:
CFont* GetFont( ) const;
I don't see how your code can build.
Dave
David Lowndes
Ignore me. I was reading what I thought your code said rather than
what it was.
Having said that, note that GetFont returns a temporary object, so it
may be the cause of your memory increase. If you stop resizing and
wait a while, MFC's idle processing may clean it up.
Dave
none
> Having said that, note that GetFont returns a temporary object, so it
> may be the cause of your memory increase. If you stop resizing and
> wait a while, MFC's idle processing may clean it up.
I am chalking it up to this. Even if this isn't the cause, there's not
much I can do about it outside of not using GetFont() and SetFont(), and
that's not an option.
Thanks a lot for all the help.
Joseph M. Newcomer
superficially it appears that this makes sense, in fact it is far LESS efficient to do it
this way than doing it in the OnPaint/OnDraw. Why? Because you will get a WM_SIZE
message dozens of times as you resize, but while you are resizing, it will not paint, so
in fact you are creating fonts more often than you need to!
If you are displaying "live" data and repainting the entire window, it is worth
considering conditional updates.
Do you know how long it takes to create a font? I do. Why? Because I *measured* it,
instead of guessing.
The code that does the GetFont, GetClientRect, and CreateFontIndirect takes, on the
average, 19 MICROseconds. This is on a 1.8GHz machine running Windows Vista SP2.
It does not pay to "optimize" things which do not matter. 19 us in the "critical path"
is, in fact, not particularly critical, given the relatively immense cost of doing the
display! How immense is immense? Well, to display the simple 4-character string "Mew!"
the time is about 3.9 MILLIseconds, or more than 200 times than the average font creation
time! And that was writing only ONE short string!
There is no point to optimizing something this inexpensive by writing bad code. In this
case, poorly-structured code that malfunctions. So it is not only poor code, it was
incorrect code.
The point here is that it is the poorest form of "engineering" (if you can dignify what
has been done with that term) to optimize anything in the total absence of data. You have
absolutely no basis for performing the "optimization", but went ahead and wrote really bad
code to solve a non-existent problem!
I spent 15 years doing performance analysis, and I learned a couple things
* A programmer who "knows" where the time is going, doesn't
* You will always optimize the wrong thing without data
Read my essay: "Optimization, your worst enemy" on my MVP Tips site.
Here's the raw data, feel free to copy-and-paste it into Excel and do analysis. The
source code that produced this follows.
Font Time 0.000018159 Total Time 0.003947429
Font Time 0.000017879 Total Time 0.004032077
Font Time 0.000017321 Total Time 0.003770591
Font Time 0.000017600 Total Time 0.004022020
Font Time 0.000020114 Total Time 0.004091302
Font Time 0.000017600 Total Time 0.004044927
Font Time 0.000017600 Total Time 0.004149969
Font Time 0.000017321 Total Time 0.004031239
Font Time 0.000017321 Total Time 0.004018388
Font Time 0.000014806 Total Time 0.003827302
Font Time 0.000017600 Total Time 0.004027886
Font Time 0.000017600 Total Time 0.003979277
Font Time 0.000017041 Total Time 0.004083759
Font Time 0.000037156 Total Time 0.004149410
Font Time 0.000017321 Total Time 0.004235734
Font Time 0.000018438 Total Time 0.004141029
Font Time 0.000017321 Total Time 0.004177067
Font Time 0.000017041 Total Time 0.004114210
Font Time 0.000018997 Total Time 0.004021461
Font Time 0.000022908 Total Time 0.005073829
Font Time 0.000019556 Total Time 0.004231264
Font Time 0.000017041 Total Time 0.003455467
Font Time 0.000020114 Total Time 0.003630908
Font Time 0.000022629 Total Time 0.003381994
Font Time 0.000017321 Total Time 0.003419150
Font Time 0.000017600 Total Time 0.003506591
Font Time 0.000021232 Total Time 0.005001194
Font Time 0.000017879 Total Time 0.004073981
Font Time 0.000018159 Total Time 0.004130413
Font Time 0.000017879 Total Time 0.004306134
Font Time 0.000017879 Total Time 0.004128458
Font Time 0.000018159 Total Time 0.003942121
Font Time 0.000017600 Total Time 0.003900496
Font Time 0.000017879 Total Time 0.003495975
Font Time 0.000022349 Total Time 0.003335340
Font Time 0.000017041 Total Time 0.003413842
Font Time 0.000022070 Total Time 0.003201524
Font Time 0.000017600 Total Time 0.003254045
Font Time 0.000017041 Total Time 0.003405181
Font Time 0.000017879 Total Time 0.003331150
Font Time 0.000017321 Total Time 0.003771708
Font Time 0.000017041 Total Time 0.004082362
Font Time 0.000020673 Total Time 0.003957207
Font Time 0.000018438 Total Time 0.004309207
Font Time 0.000017600 Total Time 0.004221766
Font Time 0.000017041 Total Time 0.003700750
Font Time 0.000017600 Total Time 0.003492064
Font Time 0.000019276 Total Time 0.003892394
Font Time 0.000020394 Total Time 0.003361880
Font Time 0.000017879 Total Time 0.003801600
Font Time 0.000017600 Total Time 0.004134045
Font Time 0.000017600 Total Time 0.004067556
Font Time 0.000019276 Total Time 0.005341182
Font Time 0.000017600 Total Time 0.004042413
Font Time 0.000022908 Total Time 0.005576966
Font Time 0.000018997 Total Time 0.003922286
Font Time 0.000019556 Total Time 0.003738185
Font Time 0.000017879 Total Time 0.003690693
Font Time 0.000017600 Total Time 0.003930108
Font Time 0.000017600 Total Time 0.003641245
Font Time 0.000018717 Total Time 0.004212826
Font Time 0.000014806 Total Time 0.003557715
Font Time 0.000017321 Total Time 0.003793778
Font Time 0.000016762 Total Time 0.004098286
Font Time 0.000021511 Total Time 0.004201651
void CFontSpeedView::OnDraw(CDC* pDC)
{
CFontSpeedDoc* pDoc = GetDocument();
ASSERT_VALID(pDoc);
if (!pDoc)
return;
LARGE_INTEGER start;
QueryPerformanceCounter(&start);
CFont * f = GetFont();
if(f != NULL)
{ /* change font */
CRect client;
GetClientRect(&client);
LOGFONT lf;
if(!f->GetLogFont(&lf))
{ /* GetLogFont error */
DWORD err = ::GetLastError();
TRACE(_T("GetLogFont failed, error code %d\n"), err);
} /* GetLogFont error */
else
{ /* GetLogFont success */
lf.lfHeight = -client.Height();
lf.lfWidth = 0;
CFont newFont;
newFont.CreateFontIndirect(&lf);
LARGE_INTEGER end;
QueryPerformanceCounter(&end);
int save = pDC->SaveDC();
pDC->SelectObject(&newFont);
pDC->TextOut(0, 0, CString(_T("Mew!")));
pDC->RestoreDC(save);
LARGE_INTEGER end2;
QueryPerformanceCounter(&end2);
ShowDelta(_T("Font Time"), start, end);
ShowDelta(_T("Total Time"), start, end2);
TRACE(_T("\n"));
} /* GetLogFont success */
} /* change font */
// TODO: add draw code for native data here
}
void CFontSpeedView::ShowDelta(const CString & where, const LARGE_INTEGER & start, const
LARGE_INTEGER & end)
{
double time = (double)end.QuadPart - (double)start.QuadPart;
LARGE_INTEGER freq;
QueryPerformanceFrequency(&freq);
time /= (double)freq.QuadPart;
CString s;
s.Format(_T("%12.9f"), time);
TRACE(_T("%s %s "), where, s);
} // CFontSpeedView::ShowDelta
joe
Joseph M. Newcomer
On Mon, 25 Jul 2011 23:32:52 GMT, none <no...@none.none> wrote:
>David Lowndes wrote:
>
>> You should only delete a font that you've created - you'll need to
>> revise your code to only delete the last font that you created. If you
>> just keep the single font object around you should be able to do it
>> quite neatly.
>
>Ah, this is much smarter than what I was about to attempt (keeping track of
>all the font objects NOT to delete). Each time I create one of these
>temporary resized fonts, now, I just store the handle and delete it if I
>come through OnSize() again. It seems to work, but there is still some
>sort of memory leak. I verified that no GDI objects are leaking, but if I
>just continuously resize the window, I see a leak of somewhere around 1KB
>per second in task manager. This is much smaller than the amount of memory
>that was leaking when I wasn't deleting the GDI objects, so I'm tempted to
>just live with it and put a note in the documentation: "Don't resize the
>window continuously for more than an hour or so." :)
****
Actually, this could also be due to memory fragmentation. Since you have done nothing to
actually MEASURE what is happening, there's no way to tell. I suggest looking at
_heapwalk and doing some work with that.
****
>
>Maybe CFont::CreateFontIndirect() allocates something that isn't cleaned up
>when the CFont object is destroyed?
****
Make some measurements. Look at the storage usage.
joe
****
>
>Thanks for helping.
Joseph M. Newcomer
behavior is to manipulate the child control's parameters in the child control's subclass.
joe
Joseph M. Newcomer
joe