[RESOLVED] Managing pointers with STL map

[nice_guy_mel]

I am having trouble with map containers. I have an information class called CInfo, and two map containers. I have typedef MAP_ONE as a <int, CInfo*>, and typedef MAP_TWO as a <int, MAP_ONE*>

I am able to use the containers well enough, but I am having trouble cleaning it up properly. Because the container class does not free the memory of the class the pointer is pointing to, I wanted to iterate through the maps and free up the memory myself. My first attempt was the code below. It resulted in a memory error when it tried to execute the code p_map_One->clear(); I then thought maybe once I have free the memory that the pointer was using, I should set the pointer in the map to NULL, so I added in the extra lines that are commented out below. When I uncomment these 2 lines I receive no errors, but when I observe the memory usage it looks like there is still a leak. Can anyone tell me if the below code is incorrect? Did I free up the memory correctly?

	//At this point I have a member variable MAP_TWO m_map_Two, which
	//has 2 entries. And for each entry there are 2 entries of MAP_ONE*
	MAP_ONE::iterator itrOne = NULL;
	MAP_ONE* p_map_One = NULL;
	CInfo* pInfo = NULL;
	for (MAP_TWO::iterator itr = m_map_Two.begin(); itr != m_map_Two.end(); ++itr)
	{
		p_map_One = itr->second;
		for (itrOne = p_map_One->begin(); itrOne != p_map_One->end(); ++itrOne)
		{
			pInfo = itrOne->second;
			delete pInfo;
			//itrOne->second = NULL;
		}
		p_map_One->clear();
		delete p_map_One;
		//itr->second = NULL;
	}
	m_map_Two.clear();

[Yves M]

This code looks fine, but I have had similar problems as well. In fact what happened to me was that clear() was crashing, but the real cause was an insertion operation previously that didn't work properly. So my guess is that your real problem lies somewhere else.

In any case, having pointers is not always particularly helpful when using maps. If you are careful you can pretty much avoid all copying without resorting to pointers and you'll have much easier syntax and memory management. The only deep copy that is worrysome happens when you would insert an existing (and large) MAP_ONE into a MAP_TWO. But if you construct this compound structure on the fly (i.e. you insert a new small MAP_ONE when you need and then enter elements into it) then I suggest dropping the pointers.

By the way, the code tags on forums are enclosed in square brackets [ and ], not angle brackets, that's why your post didn't come out right.

[nice_guy_mel]

Thank you Yves M. I was wondering why my <code> tags were failing Below is an example of how I insert into the maps. I would never have thought that the problem could be in the insertion. Do you see any problems here? Unfortunetly I need to use pointers to avoid the costly copying of which you spoke of. But I have always found that as long as you stick to the pointer motto (if you 'new', then you must 'delete', and set to 'NULL'), there are few problems.

void CAreaMgr::AddSection(int nSection)
{
	int nArea = gn_Count++; //gn_Count is a static variable.
	CInfo* pInfo = new CInfo();
	pInfo->Init();
	MAP_ONE* p_map_One = new MAP_ONE();
	(*p_map_One)[nArea] = pInfo;
	m_map_Two[nSection] = p_map_One;
}

[Yves M]

There is no significant copying in your example. I would only be concerned if CInfo is a large class that takes time with copying.

In terms of performance it's useful to think of maps in terms of trees. You allocate a node once and then it can get moved around, but it will not be copied or reallocated. So this is not the same as with std::vector for example where reallocation and copying occurs regularly when the container grows. So a map of pointers only cuts down on one single copy: the one when the object is inserted into the container.

Now your insertion also looks allright. So that's probably not it either.

When I uncomment these 2 lines I receive no errors, but when I observe the memory usage it looks like there is still a leak

Which compiler and which STL do you use? Some STL implementations internally use non-default allocators, so they don't just use new/delete for allocating single elements. This means that memory usage can seem inconsistent with what you are actually doing in your program. The only reliable way I have found to debug memory leakage is with dedicated tools like Purify.

[nice_guy_mel]

I am using visual studio 6.0 with service pack 6. I'm not sure about the specific compiler or STL library version. I'll give purify a try to see what it turns up. With regards to my first post... could you write a piece of sample code of the same format, and then try the cleanup code that I wrote? Leave the commented lines commented, and see if your debugger gets an error when it gets to the clear() method. If it does, can you explain why?

[exterminator]

Can you post the code where you create a map and then create the other map containing the first set of maps? Also, the code that is used to fill the map elements? Looks like, you create map one objects dynamically and the map one itself is a dynamically created object.. but if it is not that of course, the way to clean up your map/map elements is flawed causing problems.

[miteshpandey]

Just a curious question:

What is the scenario that made you use map of maps?
Is it possible that you can apply a simpler design?

[Axter]

I recommend using a good smart pointer, that will automatically delete the pointer in your map container.
The smart_ptr class can do this for you, and there's an example usage for map in the following link:
http://axter.com/smartptr/

[exterminator]

I recommend using a good smart pointer, that will automatically delete the pointer in your map container.
The smart_ptr class can do this for you, and there's an example usage for map in the following link:
http://axter.com/smartptr/

Ideally, there shouldn't be a need of smart pointers because there isn't a need to store pointers to those objects. maps are node-based containers, so you don't really get anything much out of dynamic allocations and storing pointers (copying is not a problem there). Yves mentions this point and that is how the OP should proceed.

[Paul McKenzie]

Unfortunetly I need to use pointers to avoid the costly copying of which you spoke of.

1) Is this just a feeling on your part, or have you concrete evidence that this is the case (costly copying)?

2) Please post the "CInfo" class members, so that others can conclude whether there is any costly copying that's done.

3) Regardless of all of this, the goal would have been to write the program so that it works first (i.e. store objects, not newed objects). Then after the program is working, then and only then should you start to determine what parts can be sped up by using pointers or whatever.

could you write a piece of sample code of the same format, and then try the cleanup code that I wrote? Leave the commented lines commented, and see if your debugger gets an error when it gets to the clear() method.

This is what you should be responsible for doing. You may find the problem yourself if you took the time to come up with a smaller example. It really isn't fair to ask anyone else to take partial code, and try to duplicate your error.

I always point to the following link as a user-friendly way of posting code that doesn't work:

http://www.parashift.com/c++-faq-lit...t.html#faq-5.8

Regards,

Paul McKenzie

[nice_guy_mel]

Nice to see my post generated some interest. So far it looks like most responses state to either change the design, post more code, or to find the problem myself... laughing... really helpful.
But of course we all understand. You can't very well post your life story here and expect people to be all caught up with your code before posting your question. You just post the jist of it and hope that somebody else just 'gets it'. The story is that I was put in charge of a project that was working fine, but needed to be updated to handle about 3000 times more input than before. The whole goal here is SPEED. Getting rid of any uneeded copy constructors was my first priority. After enhancing all code lines that use the String class, my next priority was to change any containers using classes, to use pointer to classes. I think someone already mentioned using objects first and then try to use pointers. This is exactly what happend. As far as the design goes, I wasn't here for that, I was put in charge of it, and took on all the ups and downs from previous coders (I'm sure we can all relate to this... ha ha).
I should have mentioned that the CInfo class is a small class that only has 4 member variables, all integers. 'Costly Copying' was too strong a term to use. I just want to avoid ANY copying, even if it's a small copy like the CInfo class. The operation is performed approximately 500 times a second, so it's a matter of 'trimming the fat' to speed up the program.
The memory crash occurs even if I add the elements in manually such as

typedef map<int, CInfo*> MAP_ONE;
typedef map<int, MAP_ONE*> MAP_TWO;

MAP_TWO m_map_Two;
MAP_ONE* p_map_One = new MAP_ONE();
(*p_map_One)[1] = new CInfo();
(*p_map_One)[2] = new CInfo();
m_map_Two[5] = p_map_One;
p_map_One = new MAP_ONE();
(*p_map_One)[3] = new CInfo();
(*p_map_One)[4] = new CInfo();
m_map_Two[6] = p_map_One;

The class CInfo is:

class CInfo
{
public:
	CInfo& operator=(const CInfo& info) {
		m_a = info.m_a;
		m_b = info.m_b;
		m_c = info.m_c;
		m_d = info.m_d;
		return *this;
	}
	int m_a;
	int m_b;
	int m_c;
	int m_d;
};

From this code and the cleanup code posted previously, you can easily create a simple console application to test with. I want to stress that I'm not some student with a project that is due, and quickly looking for someone else to do my work for me (we've all read post from those guys in here... ha ha). As far as I'm concerned the matter is closed. When I uncomment the lines that set the map's pointer to NULL, the clean up code works fine, so my project is moving ahead. Next I'm taking all the constructors and giving them an initialization list (again hoping to trim the fat). I simply thought that the crash was interesting, and wanted to throw it into a forum, to see if anyone else had come across this themselves.

[Yves M]

Interesting. So how much overall speed increase do you get when changing to pointers?

[Paul McKenzie]

The whole goal here is SPEED. Getting rid of any uneeded copy constructors was my first priority. After enhancing all code lines that use the String class, my next priority was to change any containers using classes, to use pointer to classes.

Did you determine this by using a profiler to profile your code, or is this whole thing of what needs to be optimized determined "by feel"? (Basically asking the same question as before).

Bottom line -- you just can't make up what you think needs to be optimized. That usually leads down the path of making the code more buggy and less maintainable, with no speed gains whatsoever. To add, in many cases, it causes a degradation of speed, since you changed the code enough so as the compiler's own optimization strategy is compromised.

Before embarking on making the code "optimized", you should make sure that what you're doing is going to make any appreciable difference by properly profiling the code with profiling tools.

See the link below -- it pertains to Linux, but it is valid for any OS.

http://movementarian.org/linux-profi...ilingcode.html

Regards,

Paul McKenzie

[exterminator]

From the amount of information you have provided, here are my comments/guesses/questions:

1. It should not cause any problems, ideally. If that is how you are populating and cleaning up.
2. What STL are you using?
3. Do you ever use array form of new to allocate objects mixed with non-array form of new to insert into the map?
4. Looking at CInfo for which you have defined the operator=, do you have a copy constructor? Do you have pointer members in it? You don't need explicit copy assignment operator. The code looks suspicious to me or else you have provided wrong/incomplete information.
5. Are there any classes derived from CInfo?
6. What crash do you get? Is it std::bad_alloc exception? Can you post the error or exception details?
7. Are the maps cleaned up twice (ownership problems)?
8. Are you passing the map2 by value somewhere (esp. before the cleanup code)?

That code should not fail under normal circumstances..

[Paul McKenzie]

The class CInfo is:

class CInfo
{
public:
	CInfo& operator=(const CInfo& info) {
		m_a = info.m_a;
		m_b = info.m_b;
		m_c = info.m_c;
		m_d = info.m_d;
	}
	int m_a;
	int m_b;
	int m_c;
	int m_d;
};

1) You don't need a user-defined assignment operator. The default assignment operator does exactly what your code does, and it is safer. If you added a member to CInfo, and you forgot to add it to your list of assignments in your function, you will get a very hard to find error when you run your program. The default assignment operator doesn't make this mistake.

2) Given that you did write a user-defined assignment operator, your code does not return a reference to itself. This should not have even compiled.

Next I'm taking all the constructors and giving them an initialization list (again hoping to trim the fat).

Which goes to the thrust of my initial posts. Instead of "hoping to trim the fat", you must make certain that what you are doing is going to do anything in terms of speed. Otherwise, you're just wasting your time, and worse, adding to the additional number of problems that your code is encountering.

I simply thought that the crash was interesting, and wanted to throw it into a forum, to see if anyone else had come across this themselves.

Crashes occur mostly because of mismanaging pointers and memory. Mismanaging memory and pointers can be done in an umpteen number of ways. So there really isn't anything unique about a program that crashes that contains pointer manipulation and dynamically allocated memory.

Regards,

Paul McKenzie