Changing Brushes in Device Contexts
Markus Leutner -- markus.leutner@munich.ixos.de
Tuesday, November 26, 1996
Environment: VC++ 4.1, Win95
Hi MFC-experts,
I'm drawing polygons with different brush colors by doing things
like this (in OnDraw):
...
CBrush curBrush (RGB (nR, nG, nB));
CBrush *pOldBrush = pDC->SelectObject (&curBrush);
for (i = 0; i < nOCount; i++)
{
...
pDC->SelectObject (pOldBrush);
curBrush.DeleteObject ();
curBrush.CreateSolidBrush (nR, nG, nB);
pDC->SelectObject (&curBrush);
...
pDC->Polygon (pPts, nPCount);
}
pDC->SelectObject (pOldBrush);
...
It looks quite harmless and works well for a while (redrawing
is forced by user input with a new set of brush colors). Then
suddenly the system seems to run out of available brushes
and draws all the polygons in white. Also, the system becomes
kind of unstable as if it were in difficulties concerning its GDI
resources (redrawing faults on buttons and window frames...).
Under Windows 3.11 in 16 bit, the same code works well.
Where is the problem?
Thank You in advance,
Markus.
Manos D. Anastasiadis -- manos@ced.tuc.gr
Wednesday, November 27, 1996
[Mini-digest: 6 responses]
Regardless the environment, your code seems
to do things that it can easily avoid.
> CBrush curBrush (RGB (nR, nG, nB));
> CBrush *pOldBrush = pDC->SelectObject (&curBrush);
^^^ You can safely leave this uninitialized
^^^ and avoid the unnecessary selection
As far as the loop is concerned, the brush does not
seem to depend on the looping variable, so you
need only create it once (that is nR, nG, nB don't
seem to depend on i at first glance).
However, since this dependence is possible
to expect from your intentions as recognized by
the surrounding text, you *better* reorganize your
code as follows (if this is not the case, the only
thing needed inside your loop is the polygon drawing.)
//---------- Replace your part with this one
CBrush *pOldBrush = NULL;
for (i = 0; i < nOCount; i++)
{
....
// this object will be automatically destroyed
// with the loop's last line of code
CBrush curBrush (RGB (nR, nG, nB));
// Note the assignment
pOldBrush = pDC->SelectObject (&curBrush);
// Draw the polygon and do any other stuff
// that involves this brush
pDC->Polygon (pPts, nPCount);
// this *is* necessary
if (pOldBrush != NULL)
pDC->SelectObject (pOldBrush);
....
}
//----------- end
-----From: David Lowndes
> It looks quite harmless and works well for a while (redrawing
is forced by user input with a new set of brush colors). Then
suddenly the system seems to run out of available brushes
and draws all the polygons in white. Also, the system becomes
kind of unstable as if it were in difficulties concerning its GDI
resources (redrawing faults on buttons and window frames...).
<
Markus,
It looks to me like you're missing saving the old brush in your for
loop. Try this:
for (i = 0; i < nOCount; i++)
{
...
pDC->SelectObject (pOldBrush);
curBrush.DeleteObject ();
curBrush.CreateSolidBrush (nR, nG, nB);
***-> pOldBrush = pDC->SelectObject (&curBrush);
...
pDC->Polygon (pPts, nPCount);
}
Dave Lowndes
-----From: "Umesh Chandwani"
It is a subtle bug. I think the problem is this.
I've illustrated it in the code below.
>CBrush curBrush (RGB (nR, nG, nB));
>CBrush *pOldBrush = pDC->SelectObject (&curBrush);
>
>for (i = 0; i < nOCount; i++)
>{
> ...
> pDC->SelectObject (pOldBrush);
> curBrush.DeleteObject ();
> curBrush.CreateSolidBrush (nR, nG, nB);
THE ABOVE LINE IS THE OFFENDING LINE, curBrush IS CREATED HERE BUT
DELETE OBJECT IS NOT CALLED FOR ONE CASE OF THE ITERATION WHICH IS WHEN (i >= 0)
THIS LEADS TO GDI RESOURCE LEAKS. YOU WOULD PROBABLY HAVE TO REORGANIZE THE
CODE OR JUST
ADD curBrush.DeleteObject (); AFTER THE FOR LOOP.
> pDC->SelectObject (&curBrush);
> ...
> pDC->Polygon (pPts, nPCount);
>}
>
>pDC->SelectObject (pOldBrush);
Umesh.
-----From: "Gerald Alter"
[Moderator's note: While this solution works, DeleteObject() is
the preferred way to go.]
Markus,
If I remember correctly, the system can maintain only a limited amount of
GDI objects, such as brushes. Try the following:
CBrush* pCurBrush;
CBrush pOldBrush;
for(i=0; i < nOcount;i++)
{
...
pCurBrush = new CBrush(RGB(nR, nG, nB)); //set the colors for the brush
each time here
pOldBrush = pDC->SelectObject(pCurBrush); //select the colored brush into
the DC
...
pDC->Ploygon(pPts, nPCount); //do you poly draw
...
pDC->SelectObject(pOldBrush); //select in the old brush before deleting
the current one. Not good to delete the object while still selected into
the DC.
delete pCurBrush; //delete the colored brush
}
---------------------------------------------------
With this code, you'll only be using one brush.. ever. Also by "newing"
it, you'll have complete control over the memory used. Good luck.
Jay Alter
FASA Interactive
jalter@alt-ego.com
-----From: Dong Chen
I would try this:
CBrush* pOldBrush;
for (i = 0; i < nOCount; i++)
{
...
CBrush curBrush(nR, nG, nB);
pOldBrush = pDC->SelectObject(&curBrush);
pDC->Polygon(pPts, nPcount);
...
pDC->SelectObject(pOldBrush);
}
The problem seems at your pOldBrush pointer. The old brush object it points
to might have been deleted by the system while you are looping through the
for() loop since the SelectObject returns a pointer to a temporary object.
-----From: "John Moulder"
It look like you repeatedly select the same brush pOldBrush into the device
context.
In the loop change the line
pDC->SelectObject (&curBrush);
to
pOldBrush = pDC->SelectObject (&curBrush);
Ensure that you reselect the old brush back into the device context (once),
and delete (once) each brush when you have finished.
Jim Lawson Williams -- jimlw@mail.ccur.com.au
Friday, November 29, 1996
G'day!
Consider trying something like
CBrush* pBrush = NULL;
for (i = 0; i < nOCount; i++)
{
if (pBrush != NULL)
delete pBrush;
pBrush = new CBrush();
pBrushSheet->CreateSolidBrush(...
}
delete pBrush;
Regards,
Jim LW
Jim Lawson Williams -- jimlw@mail.ccur.com.au
Saturday, November 30, 1996
G'day David!
FYI -- in case Markus sent no copy to the list....
Regards,
Jim LW
>From: Markus Leutner
>To: "'patterso@sprynet.com'" ,
> "'jm@wg.icl.co.uk'"
> ,
> "'jimlw@mail.ccur.com.au'"
>
>Subject: Re: Changing Brushes in Device Contexts
>Date: Fri, 29 Nov 1996 13:25:52 +0100
>
>The search is over... Thank you for your help!
>
>Obviously under Win32 it's not allowed to get a pointer to the original
>brush
>only once and then use it repeatedly. It must be replaced with every
>call
>to SelectObject.
>
>
Mike Blaszczak -- mikeblas@nwlink.com
Saturday, November 30, 1996
At 10:40 11/29/96 +1100, Jim Lawson Williams wrote:
>Consider trying something like
>CBrush* pBrush = NULL;
>for (i = 0; i < nOCount; i++)
>{
> if (pBrush != NULL)
> delete pBrush;
> pBrush = new CBrush();
> pBrushSheet->CreateSolidBrush(...
>
>}
>delete pBrush;
This is woefully inefficient because you're thrashing the memory
manager. You can create one CBrush object and use it each time through
the loop. Use CBrush::DeleteObject() to destroy the Windows GDI brush
object after you're done with an iteration of the loop.
.B ekiM
http://www.nwlink.com/~mikeblas/
I'm afraid I've become some sort of speed freak.
These words are my own. I do not speak on behalf of Microsoft.
Jim Lawson Williams -- jimlw@mail.ccur.com.au
Wednesday, December 04, 1996
At 05:41 PM 30/11/96 -0800, you wrote:
>At 10:40 11/29/96 +1100, Jim Lawson Williams wrote:
>
>>Consider trying something like
>
>>CBrush* pBrush = NULL;
>>for (i = 0; i < nOCount; i++)
>>{
>> if (pBrush != NULL)
>> delete pBrush;
>> pBrush = new CBrush();
>> pBrush->CreateSolidBrush(...
>>
>>}
>>delete pBrush;
>
>This is woefully inefficient because you're thrashing the memory
>manager. You can create one CBrush object and use it each time through
>the loop. Use CBrush::DeleteObject() to destroy the Windows GDI brush
>object after you're done with an iteration of the loop.
>
>.B ekiM
>http://www.nwlink.com/~mikeblas/
>I'm afraid I've become some sort of speed freak.
>These words are my own. I do not speak on behalf of Microsoft.
>
Inefficient? Well, yes. Woefully inefficient? That's sheer hyperbole.
Markus is working with W95. In a quiet 486DX2 under W95 -- that is, no
other running applications -- the code below, per new/delete pair,
takes around 210 microsec.s for the Development version, and 20 mic.s
for the Release.
Convince me that it's worth worrying about. Spend 5 minutes working
out a routine to save those 20 mic.s and the program needs to run
15,000,000 times to give a return on that investment.
Regards,
Jim LW
***********************
{
clock_t start,finish,clocks1,clocks2;
int difference,unit;
int i,j;
int limit;
CString result;
for (limit=100000;limit<500001;limit+=100000)
{
//Set a benchmark for the loop overhead:
j=0;
start = clock();
for (i = 0; i < limit; i++)
{
//Do something stupid to beat the Compiler's optimization,
//else the loop will be optimized out:
j+=Fiddle(j);
}
finish = clock();
clocks1 = finish - start;
//Now test the new/delete overhead:
j=0;
start = clock();
CBrush* pBrush = NULL;
for (i = 0; i < limit; i++)
{
j+=Fiddle(j);
if (pBrush != NULL)
delete pBrush;
pBrush = new CBrush();
}
delete pBrush;
finish = clock();
clocks2 = finish - start;
difference= clocks2-clocks1;
unit = ((1000000/ CLOCKS_PER_SEC)*difference)/limit;
wsprintf(result.GetBuffer(40)," %d microsec.s per new/delete pair",unit);
result.ReleaseBuffer();
AfxMessageBox(result);
}
int CFREDApp::Fiddle(int j)
{
int k = j+1;
k++;
return k;
}
>From the BBC's "Barchester Chronicles":
"I know that ultimately we are not supposed to understand.
But I also know that we must try."
-- the Reverend Septimus Harding, C++ programmer
Mike Blaszczak -- mikeblas@nwlink.com
Tuesday, December 03, 1996
At 05:20 12/4/96 +1100, Jim Lawson Williams wrote:
> Convince me that it's worth worrying about. Spend 5 minutes working out
> a routine to save those 20 mic.s and the program needs to run 15,000,000
> times to give a return on that investment.
I'm not going to bother.
The problem is that you've the real point of my message: the better
fundamentals folks might get from my advice would help them understand what's
going on and avoid problems in the future. Misunderstanding that code, and the
concepts it represents, could become very expensive in other real-life
situations. So, I took the time to point out that the presented code has
problems and could be written more efficiently, since I know that the same
idea projected on to other situations can waste lots of energy--amounts that
_might_ even be meaningful to you.
The clock cycles saved are really moot: the point is that there is a much
more efficient way to realize the goal at hand. The code I suggested is
both smaller and requires a more minimal working set. Therefore, it is
more efficient--and also lots easier on the eyes.
Thes advantage of a better understanding of these techniques doesn't need
to be executed fifteen million times before it realizes an advantage.
It's a pity that you _truly_ aren't interested in repaying the investment of
my time; consistent, free, good advice isn't something for which you should
be showing such disdain.
.B ekiM
http://www.nwlink.com/~mikeblas/
I'm afraid I've become some sort of speed freak.
These words are my own. I do not speak on behalf of Microsoft.
Mario Contestabile -- Mario_Contestabile.UOS__MTL@UOSMTL2.universal.com
Friday, December 06, 1996
[Mini-digest: 6 responses]
>Inefficient? Well, yes. Woefully inefficient? That's sheer hyperbole.
>
>Markus is working with W95. In a quiet 486DX2 under W95 -- that is, no
>other running applications -- the code below, per new/delete pair,
>takes around 210 microsec.s for the Development version, and 20 mic.s
>for the Release.
>
>Convince me that it's worth worrying about. Spend 5 minutes working
>out a routine to save those 20 mic.s and the program needs to run
>15,000,000 times to give a return on that investment.
Considering you used the clock() function, I doubt these results are
accurate. Even timeBeginPeriod() only has a resolution of 20 milliseconds.
Using QueryPerformanceCounter() might of been a better choice. In any case,
the time consumed isn't _the_ problem, it's the memory usage that is.
mcontest@universal.com
-----From: "Mark F. Fling"
At 05:20 12/4/96 +1100, Jim Lawson Williams =
wrote:
> Convince me that it's worth worrying about. Spend 5 minutes working =
out
>a routine to save those 20 mic.s and the program needs to run =
15,000,000
>times to give a return on that investment.
Ah, philosphy.
C'mon guys, this is an advanced list, and we all know better than =
advocate sloppy coding practices. The U.S. automobile industry got =
slayed by the Japanese using just this design credo, "It's good enough - =
what difference does a 5 cent versus 6 cent fastener make?" Oh, how =
about a five or six billion dollars in lost market share. =20
Mike's right on this one.
Mark Fling
Strata Corporation
Santa Barbara, California
-----From: Jim Lawson Williams
At 09:48 AM 6/12/96 EDT, you wrote:
>>Inefficient? Well, yes. Woefully inefficient? That's sheer hyperbole.
>>
>>Markus is working with W95. In a quiet 486DX2 under W95 -- that is, no
>>other running applications -- the code below, per new/delete pair,
>>takes around 210 microsec.s for the Development version, and 20 mic.s
>>for the Release.
>>
>>Convince me that it's worth worrying about. Spend 5 minutes working
>>out a routine to save those 20 mic.s and the program needs to run
>>15,000,000 times to give a return on that investment.
>
>Considering you used the clock() function, I doubt these results are
>accurate. Even timeBeginPeriod() only has a resolution of 20 milliseconds.
>Using QueryPerformanceCounter() might of been a better choice. In any case,
>the time consumed isn't _the_ problem, it's the memory usage that is.
>
>mcontest@universal.com
>
Read the code and think again. Better still, test it with breakpoints. The "difference" value is accurate to +/-2 (dependent-on-the-accuracy-of-the-computer's-clock) seconds. The machine in question's clock is accurate to a few parts in 60*60*24. Therefore, after 100000 iterations the results are considerably better than "order of magnitude".
I have no idea how Win95 does is heap-management. It could possibly be a "top-down stack" with "in-use" bits set. I would not be surprised if it were based on a chain of free blocks, maybe ordered by size. A quick check through sizeof() using CBrush, CGdiObject, and CObject gave me the answers "8, 8, 4" -- i.e., none approaches a "bigger-than-a-memory-segment" figure. Therefore, regardless of how memory is managed, I see several possible sequences:
1a. The "delete" adds 3 unique blocks to the memory pool corresponding to the 3 objects.
1b. The "delete" triggers the amalgamation of each of those blocks into a larger block:
worst case, it requires joining with a block on either side. The old blocks are
removed from the pool and the new blocks added.
1c. Both unique and amalgamated blocks are added to the pool.
2a. The "new" obtains "right-sized" blocks from the memory-pool. Very possibly those
created in 1a.
2b. The "new" requires an existing block or blocks to be removed from the pool and
returned to it reduced in size.
2c. As for 1c, some combination of 2a and 2b.
Assuming I am reasonably close to the truth, I fail to see how "the memory usage" is a problem. To me it's all simple arithmetic and pointer-management, and the execution-times my test obtained from a 20MB 66Mhz machine appear consistent. Upping the number of iterations from 100,000 to 1,000,000 didn't change the result, and the machine certainly did not complain of "memory trouble". If I am way off the mark, I would be glad to be corrected.
In my view very, very few application-programs need to have the last microsecond of inefficiency squeezed out of them. Moreso in Win95 than in NT. I am quite prepared to accept that you don't agree.
Accept that, lacking evidence to the contrary, my view won't change either.
Jim LW
>From the BBC's "Barchester Chronicles":
"I know that ultimately we are not supposed to understand.
But I also know that we must try."
-- the Reverend Septimus Harding, C++ programmer
-----From: Jim Lawson Williams
At 11:33 PM 3/12/96 -0800, you wrote:
>At 05:20 12/4/96 +1100, Jim Lawson Williams wrote:
>
>> Convince me that it's worth worrying about. Spend 5 minutes working out
>> a routine to save those 20 mic.s and the program needs to run 15,000,000
>> times to give a return on that investment.
>
>I'm not going to bother.
>
>The problem is that you've the real point of my message: the better
>fundamentals folks might get from my advice would help them understand what's
>going on and avoid problems in the future. Misunderstanding that code, and the
>concepts it represents, could become very expensive in other real-life
>situations. So, I took the time to point out that the presented code has
>problems and could be written more efficiently, since I know that the same
>idea projected on to other situations can waste lots of energy--amounts that
>_might_ even be meaningful to you.
>
>The clock cycles saved are really moot: the point is that there is a much
>more efficient way to realize the goal at hand. The code I suggested is
>both smaller and requires a more minimal working set. Therefore, it is
>more efficient--and also lots easier on the eyes.
>
>Thes advantage of a better understanding of these techniques doesn't need
>to be executed fifteen million times before it realizes an advantage.
>
>It's a pity that you _truly_ aren't interested in repaying the investment of
>my time; consistent, free, good advice isn't something for which you should
>be showing such disdain.
>
>.B ekiM
>http://www.nwlink.com/~mikeblas/
>I'm afraid I've become some sort of speed freak.
>These words are my own. I do not speak on behalf of Microsoft.
>
I am going to bother, because the point's important. Advice can be made up from fact and opinion.
Where fact is concerned I utterly defer to your superior knowledge. I have learned much from your contributions to this list; hope to learn more in the future; am extremely grateful for those contributions. Neither, as you well know, am I looking for a "free ride", having been attempting to obtain a copy of your book for some months now. Where advice is confined to fact it is certainly "good", and is unchallengeable.
Opinion is a different matter. At 05:41 PM 30/11/96 -0800, you wrote:
>This is woefully inefficient because you're thrashing the memory
>manager.
To which I responded "Inefficient? Well, yes. Woefully inefficient? That's sheer hyperbole." The inefficiency is cheerfully-admitted fact. Its relevance to Markus's application -- that is, the effect it might have on the memory manager, the task, the task-mix, and ultimately the User's perception -- can only be a matter of opinion since there is but the scantiest detail.
In particular, I was concerned that list-members with less time in the computer business would take your comments absolutely literally, and not subject them to any test. As _the_ Authority, everything you say is likely to be followed blindly, unthinkingly, by many of the 2000-plus on this list. If you say "doing such-and-such will cause the memory-manager to thrash", to many this will mean "dire, catastrophic consequences" irrespective of context. I sought to demonstrate the real cost of the inefficiency, and to leave final judgement on the price of pairing "delete()/new()" to the invidual according to circumstances. It was not, is not, intended to advocate that approach above any other at any time, but as an analysis of the likely effect on Markus's particular application and any similar code-sequence.
In no way do I deprecate your recommendation for a single Cbrush. My solution to Markus's problem was technically inferior. Should I ever need a similar code-sequence I now have a superior model. However, if ever I am faced with a non-time-critical problem where the only solution which comes readily to mind is to replace objects through "new()/delete()", I will not hesitate to do so. Nor would I hestitate to recommend such an approach to someone who, lacking your unique insight, was otherwise unable to skirt a minor problem.
I regret that you have taken offence at an alternative opinion. My intention was not to convey disdain, but rather to provoke thought amongst the readership of this list. Where is "the point of diminishing returns" in any application? This code works, and has no apparent side-effects: is it really worth searching for a more computer-efficient way? Do I truly need to condense five simple lines of code into a single, highly-complex one so that the compiler can generate tighter machine-language? Am I solving the REAL problem?
With the highest regard,
Jim LW
>From the BBC's "Barchester Chronicles":
"I know that ultimately we are not supposed to understand.
But I also know that we must try."
-- the Reverend Septimus Harding, clog-dancer, C++ programmer
-----From: Mike Blaszczak
At 22:48 12/8/96 +1100, Jim Lawson Williams wrote:
>>This is woefully inefficient because you're thrashing the memory
>>manager.
>Its relevance to Markus's application -- that is, the effect it might
>have on the memory manager, the task, the task-mix, and ultimately the
>User's perception -- can only be a matter of opinion since there is
>but the scantiest detail.
Yep. I guess the difference is that you insist that the code isn't woefully
inefficient just because you don't know the details, while I point out the
code is woefully inefficient because I don't know the details.
>In particular, I was concerned that list-members with less time in the
>computer business would take your comments absolutely literally, and
>not subject them to any test.
Unfortunately, you've gone through the trouble to make a test that
produces no meaningful results. Maybe you should worry about how people
will perceive your own comments instead of worrying about how they'll
take mine.
> As _the_ Authority, everything you say is likely to be followed blindly,
> unthinkingly, by many of the 2000-plus on this list.
I'm not sure that's the case. This is a list of a couple thousand developers,
not a couple of thousand lemmings. If what you say is really true, at
least _one_ person would have bought me a hockey team by now, and I can
report that I've had no such luck.
>If you say "doing such-and-such will cause the memory-manager to thrash",
>to many this will mean "dire, catastrophic consequences" irrespective
>of context.
Since I don't know the context, I have to explain the consequences and let
the reader interpret them for themselves. What if someone interpolated to
original to CString? Maybe they'd code:
CFile file("bigfile.txt", CFile::modeRead);
//TODO: something about exceptions
CString* pstr = new CString;
while (file.ReadString(pstr))
{
PartyOnAString(pstr);
delete pstr;
pstr = new CString;
}
delete pstr;
Here, maybe it's a lot more obvious that the code is wasting time. Here,
the code is more likely to run many times in succession and more likely to
make your app look sick.
>I regret that you have taken offence at an alternative opinion.
Perhaps I'm not taking offense at the alternate opinion: perhaps I'm actually
taking offense the way it was presented. And I'm not saying that you're being
abrasive or impolite: I'm saying that you're being unscientific.
The code _is_ woefully inefficient; that isn't hyperbole. If you made
a _comparative_ benchmark, you'd be able to see it. (On my machine, your
test harness shows that a release mode version of my code executes in
_zero microseconds_, which is infinitely better than the 52 microseconds
each iteration of your code uses in an optimized build. While all the
absolute numbers mean is that the code is executing faster than
your timing method can clock it, it certainly shows that touching the
memory manager is woefully inefficient. Since the granularty of calls
to clock() on Win32 is about 50 _micro_seconds, it's not easy to decide
what good the numbers really are.)
There are other ramifications to the code you wrote that can help you
understand why it's wasting time:
+ the constructor and destructors for CBrush are inlined, but your
call to operator new and operator delete makes them out of line.
+ the call to operator new is out of line, which means that the
processor's prefetch queue is flushed.
+ the call to operator new probably involves a call into the operating system.
+ the call to operator delete probably involves a call to the operating system.
I'm not sure why your analysis didn't include these things, but once you
see them, maybe you will start to think that the new/delete dethod isn't
such a great idea. By wasting the prefetch queue, for example, you're
effectively downgrading your machine from a Pentium to a 486 for the time
it takes to run that code.
> Where is "the point of diminishing returns" in any application?
Only with the strictest enforcemnet have we been able to get people to
consistentlyadmit which operating system and product version they're asking
about. It seems like it would be impossible for us to get folks to explain
what they think is important with each post.
As a result, it's up to each person reading a note to decide what they
want to do:
a) Hey! Mike gave me something more efficient for free.
That'll help, so I'll use it. I'll fire him a nice
"Thank you note" in a private mail.
b) Oh, that's cool: you can do this a more efficient way.
That's not really important to me, but I'll print this
out and clip it into my notebook for later.
c) Ugh! Another cycle-counting weenie. Why doesn't this guy
_shut up_ and finish the next edition of his book so I can
go to the store and _not_ buy it?
You took a fourth route:
d) Hey! Mike gave me something more efficient for free.
That doesn't help me, and I'm not positive it will help
anyone else, so I'll challenge Mike's advice publicly.
If I _didn't_ post my note and someone used that code and noticed that
it was slow and didn't realize there was an alternative (because I hadn't
posted my note, maybe), they'd only come to the conclusion that "MFC
sucks because it is slow".
What I'm saying is: each person who's reading any of this can decide,
for their own bad selves, what do do with the advice. Your test didn't
demonstrate anything, since you didn't bother to compare your results
to anything. You are correct in that people need to decide what's valuable
for them. Most people can do that using their own parameters, and would
probably be able to do a more meaningful job of it without being tainted
by ineffective comparisons.
>This code works, and has no apparent side-effects: is it really worth
>searching for a more computer-efficient way?
It depends, on how much time you have to spend "searching". It took me,
literally, no time at all to decide to comment on the cdoe because I can
see, at a glance, that it's abusing MFC and the memory manager. Maybe
my view is distorted: maybe it really _is_ hard for people to read the
manual and think about how the pieces can more efficiently fit together.
So, to that end, I pointed out that the fallacy in the code so that other
people could understand it, too.
Instead of being thanked for that advice, I'm sitting around writing notes
that defend my decision to give that advice. How much sense does that make?
> Do I truly need to condense five simple lines of code into a single,
> highly-complex one so that the compiler can generate tighter
> machine-language?
I didn't propose any such exercise. (In fact, the code I suggested is
_easier_ for develoeprs to read and maintain because there is no chance
of forgetting the last delete call outside of the loop. That would cause
a memory leak.) But, in all but the rarest circumstances, you don't
need to do this. It is easy to show that the compiler will generate
the same code for:
if (bFlag)
DoSomething();
else
DoSomethingElse();
as it would for
(bFlag ? DoSomething() : DoSomethingElse());
for example.
> Am I solving the REAL problem?
To me, the question was "How do I most efficiently reuse a brush in a
CBrush object?" The _correct_ answer is: call CBrush::DeleteObject().
Destructing and reconstructing the object each time by using the memory
manager is a waste of time, but the REAL problem is that you need to
understand MFC. If you understand this tehcnique, you can replicate it
to lots of other parts of MFC--including some tools which are usually
applied in situations that would make kind of inefficiency painfully
evident in short order.
.B ekiM
http://www.nwlink.com/~mikeblas/
I'm afraid I've become some sort of speed freak.
These words are my own. I do not speak on behalf of Microsoft.
-----From: Jim Lawson Williams
At 12:29 PM 8/12/96 -0800, you wrote:
>Unfortunately, you've gone through the trouble to make a test that
>produces no meaningful results.
On the contrary. If, on my machine, with "limit" set to 100,000 the whole thing takes 2 seconds, and set at 1,000,000 it takes 20 sec.s, then a single iteration of "new() / delete()" has to be around the 20 microsec. mark. From that you can deduce the 1st, "benchmark", inner loop contributes vitually nothing to the overall time for an outer-loop iteration.
>Perhaps I'm not taking offense at the alternate opinion: perhaps I'm actually
>taking offense the way it was presented. And I'm not saying that you're being
>abrasive or impolite: I'm saying that you're being unscientific.
>The code _is_ woefully inefficient; that isn't hyperbole. If you made
>a _comparative_ benchmark, you'd be able to see it.
Running "reasonableness" checks, with a single observed max. of 22 sec.s for 1,000,000 iterations, gave me confidence in my results. I reject the "unscientific" comment. I accept that in other comparisons -- not the problem heading this discussion -- that the results could be considerably worse.
>(On my machine, your
>test harness shows that a release mode version of my code executes in
>_zero microseconds_, which is infinitely better than the 52 microseconds
>each iteration of your code uses in an optimized build. While all the
>absolute numbers mean is that the code is executing faster than
>your timing method can clock it, it certainly shows that touching the
>memory manager is woefully inefficient. Since the granularty of calls
>to clock() on Win32 is about 50 _micro_seconds, it's not easy to decide
>what good the numbers really are.)
Look more closely at the code. The calls to clock() are outside the innner loops, checked after "limit" iterations. That is, in the case of the second inner loop, for 1,000,000 iterations on my 486DX2 under W95, some 20 seconds between samplings. The granularity is irrelevant with such a time-lapse. Those numbers are good enough. Your machine is considerably faster than mine. To get meaningful results, you should have changed the outer loop values so that the "message box to message box" time was an appropriate number of seconds.
>
>There are other ramifications to the code you wrote that can help you
>understand why it's wasting time:
>
>+ the constructor and destructors for CBrush are inlined, but your
> call to operator new and operator delete makes them out of line.
>+ the call to operator new is out of line, which means that the
> processor's prefetch queue is flushed.
>+ the call to operator new probably involves a call into the operating system.
>+ the call to operator delete probably involves a call to the operating system.
>
>I'm not sure why your analysis didn't include these things, but once you
>see them, maybe you will start to think that the new/delete dethod isn't
>such a great idea. By wasting the prefetch queue, for example, you're
>effectively downgrading your machine from a Pentium to a 486 for the time
>it takes to run that code.
My machine _is_ a 66MHz 486. Pentium-owners might therefore expect no worse a figure than the 20 mic.s. under Win 95 -- the environment in question. That is not a suggestion that anyone should, merely an observation of what is likely to happen if they do. Nor was it ever a prophecy of what might happen in a busy NT machine, vigorously swapping contexts.
>You took a fourth route:
>
> d) Hey! Mike gave me something more efficient for free.
> That doesn't help me, and I'm not positive it will help
> anyone else, so I'll challenge Mike's advice publicly.
>
>If I _didn't_ post my note and someone used that code and noticed that
>it was slow and didn't realize there was an alternative (because I hadn't
>posted my note, maybe), they'd only come to the conclusion that "MFC
>sucks because it is slow".
>
>Instead of being thanked for that advice, I'm sitting around writing notes
>that defend my decision to give that advice. How much sense does that make?
The challenge was, is, solely to the adverb leading your "woefully inefficient" comment, not to the quality of your solution. You have yet to convince me that the machine's performance would be "woeful". You have yet to convince me that, while your approach is clearly technically superior, a user of Markus's application would be able to detect _any_ appreciable difference in the speed of drawing, even were the figure 100 mic.s rather than 20.
Jim LW
>From the BBC's "Barchester Chronicles":
"I know that ultimately we are not supposed to understand.
But I also know that we must try."
-- the Reverend Septimus Harding, clog-dancer, C++ programmer
| Вернуться в корень Архива
|