Post by Zhan WeimingHi,
I have simplified codes as below,
Simplified too much, I'm afraid. Since it doesn't compile, it
probably isn't representing your problem very well either. Certainly,
the problem cannot be reproduced by running this code.
Perhaps you left out something important to the problem you're
describing?
Post by Zhan Weimingtypedef struct
{
int x;
int y; }TypeA;
No need for typedefs on structs, unless this is extracted from a C
header.
Post by Zhan Weimingtypedef vector<TypeA> TypeB;
typedef vector<TypeB*> TypeC;
Storing raw pointers to allocated objects is a frequent source of
memory related errors. If a vector holds raw pointers, it's best that
it not also have ownership responsibility, but be an observer. Even
then, you have memory issues if the object is deleted elsewhere and
left in the vector.
Holding smart pointers will solve this problem. Using a shared_ptr
the vector element can own and manage the allocated underlying
object. Using weak_ptr, the vector can safely be an observer. (Weak
pointers refer to objects in a shared_ptr, but do not keep them
alive. Additionally, if the shared_ptr destroys the object, all weak
pointers that refer to it become null automatically.)
Post by Zhan WeimingTypeA a;
TypeB b;
TypeC c;
Global variables?
Post by Zhan WeimingMainLoop()
{
do
{ //insert one object to c
InsertOneRecord();
ok so far.
Perhaps you're using a lot of memory in some of these actions?
Post by Zhan Weiming//erase object in c
DeleteOneItem(idx); //expect to erase memory alloacted.
Conceptually sound, but the implementation is far from safe.
Post by Zhan Weiming} while(somecondition);//somecondition keeps true for a long time.
}
InsertOneRecord()
{
GetB(b); // push some items to b
TypeB *pb = new TypeB;
FillB(b, pb); //push items in b to *pb
c.push_back(pb); }
Using a global "b" object is a bad choice. You are giving up
multi-threadded options, among other issues. Since this function
needs a TypeB object, create it here. Or have GetB create and return it
to you, so there isn't the "create it, initialize it, then overwrite
that by calling GetB() immediately afterwords.
Further, you allocate a TypeB and store it in a raw pointer. After
that, making calls to FillB or push_back may throw, and so the object
will be leaked if an exception occurs.
FillB would be best a member function of TypeB, since then the new
operation could allocate and initialize it in one step, rather than
allocating and default initialize it, only to immediately throw away
that default initialization and overwrite it with real values in a
function call.
Again, using a smart pointer would make this function exception safe
too. (pb would become a smart pointer instead of a raw pointer. Then
if an exception is thrown, the smart pointer's destructor would
deallocate the underlying object, averting a memory leak.)
Post by Zhan WeimingDeleteOneItem(int index)
{
TypeB *pb = c[index];
delete pb;
vector <TypeB*>::iterator itr;
itr = c.begin();
itr += index;
c.erase(index);
}
Technically, you want to delete the pointer only after erasing it from
the vector. Otherwise there is a period of time where the vector
holds a dangling pointer, and if you're pedantically following the
language standard, even reading the address held in an invalid pointer
without dereferencing it can produce undefined behavior.
Also, you probably want range checking here, and may benefit from
using the std::advance function too.
Your code shows a design habit of declaring variables in one place and
initializing them later. This is frequently necessary in C, but in
C++ much less so due to constructors. The danger is the window of
code where the values are wrong and if accidently used would result in
misbehaving code. The other danger is needless performance
degradation. Unless the constructor is trivial, it's faster to create
the object "correctly" the first time, directly creating it with
initially correct values. It doesn't make sense to create it with a
default (but wrong) value and then write new values on top of that.
When done with initialization and smart pointers, the code is much
safer, concise, and easier to read. (I think.) Here is a safer
rewrite of your DeleteOneItem function:
#include <iterator>
#include <stdexcept>
#include <memory>
void
DeleteOneItem(int index)
{
if (index < 0 || index >= c.size()) {
throw std::range_error("DeleteOneItem: index out of bounds");
}
std::auto_ptr<TypeB> killer(c[index]);
c.erase(std::advance(c.begin(), index));
}
auto_ptr is a smart pointer that deletes whatever it holds when it
goes out of scope. In the future this should be a scoped_ptr, but
that's not yet part of the C++ language (and auto_ptr is going to be
labeled as deprecated.)
Post by Zhan Weimingafter running for some time, it raised "out of memory".
Perhaps your vectors are simply growing too much?
Perhaps the rest of your application is doing something with a lot of
memory?
Post by Zhan WeimingAny body knows where the memory leak?
Nobody but you can answer that, since only you have your source code.
(Just to be clear, I'm not asking for your whole application, but for
representative code, it needs to be complete enough to actually target
and show the problem.)
Also, it'd be useful if you gave a description of how this is used.
As fast as the CPU can go, or just every now and then? How big are
the vectors anyway? How much data are you trying to store? Etc.
Post by Zhan Weimingor how to clear the memory as expect?
How do you know that this vector memory management is the actual
source of the problem? Are exceptions occurring? If so, that could
cause leaks which, if done often enough, could accumulate into a lot
of memory.
--
Chris (TeamB);