Resource Management question..

Really I just want to know which would be safer... or if one or both has a major pitfall.

Code:

namespace Varia
{
    class ResourceMgr
    {
    public:
        ResourceMgr();
        ~ResourceMgr();
        
        void AddResource(const char* p_scriptFile);

        void DeleteResource(int p_index);
    private:
        std::vector<Resource*> Resources;
    };
};

or
the same thing but using std::map instead of vector.
I was thinking that if you delete a resource in a vector then every index after the deleted resource would change. Although with the map the index would remain the same with deleting a certain resource. Really I am wondering what would be the downfalls to either approach. I know the classes are not complete, but hopefully I got the point across.

Instead of Resource*, I would hold Resource. If you need a pointer for some reason, use a shared_ptr, or use a pointer container from boost.

As far as map or vector, it sounds like the map might be more appropriate. If you can delete a resource at a specific index and the other resource indexes should not change, then you probably want a map.

If the resources will all be closely populated (few gaps between used indexes) then a vector might be ok and you would just null out the entry when a resource is deleted rather than removing it from the vector.

If you're not sure, I'd use the map because it probably takes less work to implement what you want and would be more clear.

Thank you Daved. The reason for the pointer is because I need Resource to be Dynamically Allocated.
EDIT: shared_ptr is safe to use in STL? Also, is it guaranteed to delete its memory upon deletion of the Manager?

I have another probably simple question.
How do I go through each element of a std::map, and make sure it is freed. (class Destructor)
I am thinking iterators, but I wanted to be sure. So i don't have a million memory leaks.

This is what i think should work, but i am unsure

Code:

std::map<int, m_ResourceStruct>::iterator it;
	for ( it = m_resources.begin(); it != m_resources.end(); it++ )
	{
		if ( (*it).second.m_allocated == true )
		{
			delete (*it).second.m_res;
			(*it).second.m_res = NULL;
		}
	}

The only evidence I can show you against using a map is to trace through the code in the debugger when you access the map elements. There is an awful lot going on just to get to the item in question. I've been leaning strongly towards hash maps for resource management because of what Daved said about vectors (removing items is a big problem) and because of what I've seen in std::map. In order to gain the functionality you need for a resource manager you are going to have to develop your own custom container that makes use of one, more, or no containers in the STL. Whether you use the STL or not again depends on the performance information you gather during your research into other algorithms and container types.

I highly recommend you use pointers in your containers but I also agree with Daved in that if you are going to do that you will want to use boost smart pointers or at the least auto_ptr's. I recommend pointers because it does get rid of a lot of the copying overhead inherent in STL containers. In my own experience I've ran into significant slowdowns when I did not use pointers in the containers. Again this is not a hard-fast rule and you need to research it inside your own code base. A lot of your design decisions will hinge on how your system operates or needs to operate and any guess I make here would be just that...a guess.

For starters I would recommend just getting the resource system working. It does not have to be super fancy it just has to work. Then you can address slowdowns and other possible issues once you have it working. Deciding what approach to take here and now without some hard concrete evidence as to why may lead you out into the weeds. Go code a rudimentary system and play around with it, tax it, break it, get it to crash, etc. Then slowly you can chip away the rough edges and end up with a fairly decent system.

To answer your second question you would delete anything left in the map during cleanup. If you are using pointers to objects on the heap those objects will not be cleaned up by the STL container's destructor. I'm not sure why you are checking if the object was allocated in your destructor. If you allocated memory for the object at any time using the new operator then you must call delete on it regardless of the state of it's internal variables. So the fact that you have an object and can check m_allocated means that you must delete it. In your current destructor code you will leak memory.

Code:

for ( it = m_resources.begin(); it != m_resources.end(); it++ )
{
                //If you have a pointer here then you want to call delete on it
                //Whats with the m_allocated check?
	if ( (*it).second.m_allocated == true )
	{
		delete (*it).second.m_res;
		(*it).second.m_res = NULL;
	}
}

This appears to me that your resource class has a m_res member that needs cleaned up. Problem is that even though you call delete on that class member you are not calling delete on the object containing the member. So you have effectively leaked the entire map here. The object in the map should cleanup m_res in it's own destructor so that when you call delete on the object, both it's internal object and the object itself are cleaned up.
I'm assuming this code is in the destructor of the manager class.

So if class M has a map of A objects and each A object has a B object you are cleaning up the B object. However you are not cleaning up the A object which means you have leaked the entire map inside of M.

Thank you bubba for that insight. This is the actual code. the m_allocated variable was actually to prevent calling delete on a object that was never allocated in the first place.

Code:

/*
	Author: Cody Doughty
	Date:   March 4, 2008
*/
#ifndef RESOURCE_MANAGER_H
#define RESOURCE_MANAGER_H

//For hgeResourceManager
#include <hgeresource.h>

//For STL map class
#include <map>

//For printf
#include <cstdio>


namespace Varia
{
	class ResourceMgr
	{
	public:
		ResourceMgr();
		~ResourceMgr();
		
		//This will only work if a Resource has not been allocated at the specified index
		void AddResource(int p_index, const char* p_scriptFile);

		//This will return NULL if no resource is allocated at the specified index
		const hgeResourceManager* GetResource(int p_index);

		//Return true if the resource is allocated at the specified index
		bool IsAllocated(int p_index);

		//This will only work if a Resource has been allocated at the specified index
		void RemoveResource(int p_index);

	private:
		typedef struct m_ResourceStruct
		{
			bool m_allocated;
			hgeResourceManager* m_res;
		};
		std::map<int, m_ResourceStruct> m_resources;
	};
};
#endif

and the implementation

Code:

#include "ResourceManager.h"

Varia::ResourceMgr::ResourceMgr()
{
	//nothing to construct
}

Varia::ResourceMgr::~ResourceMgr()
{
	std::map<int, m_ResourceStruct>::iterator it;
	for ( it = m_resources.begin(); it != m_resources.end(); it++ )
	{
		if ( (*it).second.m_allocated == true )
		{
			delete (*it).second.m_res;
			(*it).second.m_res = NULL;
		}
	}
}

void Varia::ResourceMgr::AddResource(int p_index, const char* p_scriptFile)
{
	//Check if the index exists
	if ( m_resources.find(p_index) != m_resources.end() )
	{
		//Check if the index is allocated
		if ( m_resources[p_index].m_allocated == true )
		{
			printf("Resource:&#37;d already allocated.\n", p_index);
			return;
		}
	}
	//allocate
	m_resources[p_index].m_res = new hgeResourceManager(p_scriptFile);
	m_resources[p_index].m_allocated = true;
	printf("Resource:%d allocated with Script:%s\n",p_index, p_scriptFile);
}

const hgeResourceManager* Varia::ResourceMgr::GetResource(int p_index)
{
	//Check if the index exists
	if ( m_resources.find(p_index) != m_resources.end() )
	{
		//Check if the index is allocated
		if ( m_resources[p_index].m_allocated == true )
		{
			printf("Resource:%d accessed.\n", p_index);
			return m_resources[p_index].m_res;
		}
		else
		{
			printf("Resource:%d not allocated.\n",p_index);
			return NULL;
		}
	}
	else
	{
		printf("Resource:%d does not exist.\n", p_index);
		return NULL;
	}
}

bool Varia::ResourceMgr::IsAllocated(int p_index)
{
	//Check if the index exists
	if ( m_resources.find(p_index) != m_resources.end() )
	{
		if ( m_resources[p_index].m_allocated == true )
		{
			printf("Resource:%d is allocated.\n",p_index);
			return true;
		}
		else
		{
			printf("Resource:%d exists but not allocated.\n",p_index);
			return false;
		}
	}
	printf("Resource:%d does not exist.\n",p_index);
	return false;
}


void Varia::ResourceMgr::RemoveResource(int p_index)
{
	//Check if the index exists
	if ( m_resources.find(p_index) != m_resources.end() )
	{
		//Check if the index is allocated
		if ( m_resources[p_index].m_allocated == true )
		{
			delete m_resources[p_index].m_res;
			m_resources[p_index].m_res = NULL;
			m_resources[p_index].m_allocated = false;
			printf("Resource:%d Freed.\n", p_index);
			return;
		}
		else
		{
			printf("Resource:%d exists but not allocated.\n",p_index);
			return;
		}
	}
	else
	{
		printf("Resource:%d does not exist.\n", p_index);
		return;
	}
}

I don't really know if this does what I want it to, but I am hoping it does.

I know it doesn't make much sense to have a ResourceManager for a ResourceManager, but its the only way I could think of to make it work right.

>> shared_ptr is safe to use in STL?
Yes, it is auto_ptr that is not safe. (Note to Bubba, auto_ptr's in STL containers are very bad.)

>> Also, is it guaranteed to delete its memory upon deletion of the Manager?
Yes. when the last shared_ptr pointing to a Resource is destroyed, it will delete the Resource.

>> How do I go through each element of a std::map, and make sure it is freed.
If you used shared_ptr or the ptr_containers from boost, then it would happen automatically and you wouldn't need a destructor. However, your destructor looks fine if you stick to raw pointer.

Based on the whole code, it appears Bubba's worry about leaking the m_ResourceStruct is not a problem, since you hold that by value in the map. My question is why is it necessary? If the m_res pointer is null, then the resource exists but is not allocated, and if it is not null then it is allocated. I think you can store the Resoure* directly in the map and remove the m_ResourceStruct completely.

But what if m_res somehow became a pointer to an existing Resource(external), and it wouldn't be NULL, but the memory would not belong to it still. right?

I highly recommend you use pointers in your containers but I also agree with Daved in that if you are going to do that you will want to use boost smart pointers or at the least auto_ptr's.

From that statement it does appear I was saying use auto_ptr's in STL containers which is not what I intended. I think my brain was going faster than my fingers and I merged two thoughts into one sentence. I was not inferring you should use std::auto_ptr's in containers but might utilize them in general in other places in your code. auto_ptr's have some very ugly copy semantics that make them horrible for use in STL containers. If I misled you on that I do apologize. My original thought was that in places that you are using raw pointers AND that are not in STL containers, using auto_ptr at the very least would be better than raw pointers. Sorry for the confusion.

Based on the whole code, it appears Bubba's worry about leaking the m_ResourceStruct is not a problem, since you hold that by value in the map.

Very true. Yet I'm confused as to why you are storing the resource struct by value but the resource struct itself then has a raw pointer inside of it that points to yet another manager? This is at best confusing.

Raw pointers are of course faster.
As for the vector concern, you could always use pointers but not remove them.
You could just delete the memory and set the pointers to NULL. No removing, no speed hit. And no need to use anything other than vector.
Of course, it's up to you what you want to use.
I always recommend using smart pointers for any pointers, however.
I can't say if boost's smart pointers have the option to explicitly decrement the reference count before the destructor, however. If not, then the program would be a resource hog...

>> But what if m_res somehow became a pointer to an existing Resource(external), and it wouldn't be NULL, but the memory would not belong to it still. right?

Yes, that's fine. If the allocated flag distinguishes between resources allocated locally and those allocated elsewhere then it has a real purpose. Another term might be "owned" since if the resource is owned by the manager than it should be cleaned up by the manager.

If your classes manage memory manually, you'll need to code the copy constructor and copy assignment operator as well (otherwise you'll have two classes deleting the same pointers, or a class full of dangling pointers, should a shallow copy occur).

In this case, however, you'll probably want to make your class non-copiable. For that you'll need a private copy constructor and a private copy assignment operator. These needn't be implemented, as they should never be used.

I thank you all greatly for the insight. I am thinking that a small rewrite of the code may be in order due to the fact that my ResourceManager(s) are very confusing to look at.

Should I make classes that allocate/deallocate all members at construction/destruction, and then have a template class for the resource manager to allocate/deallocate these individual classes?
Sorry if that doesn't make much sense, but my system seems flawed.

You could just delete the memory and set the pointers to NULL. No removing, no speed hit. And no need to use anything other than vector.

There is a problem here. I tried this in my own system and the issue is that you either must use a finite sized vector or use some algorithm to 'scan' the vector and make sure that it is being used optimally. What I found is that the vector grows in size yet you have no way of shrinking it's size. So if you are iterating from 0 to vector::size() and only operating on elements that are non-NULL, your iteration grows each time you add items to the vector and you have no way of removing items from the vector (thus reducing the size).

I solved some of this by remembering the last item that was 'removed' from the vector. In actuality the function just cleaned up the memory of the object in the vector and then set the pointer at the index to NULL. This index was then remembered and the next time a vector 'slot' was requested it would use the stored one. I further optimized it by keeping a stack of 'removed' indices into the vector. If the stack was empty then an item had to be added to the vector via push_back, otherwise the item merely had to be placed according to index stack.

It worked like this:

Add item

• Check the removed index stack.
• If stack is empty, add via push_back
• If stack is not empty, use index from top of stack and add object at this index. Pop index off of stack since it is now being used.

Remove item

• Clean up memory of object at index being removed
• Set value of vector[index] to NULL
• Push index that is being removed onto index stack

This really optimized the use of the vector and then I realized that I might as well use an array for this type of system. So I dumped the vector and changed to array. The only change to the algorithm was that when adding an object if the index stack was empty the number of elements in the array had to be incremented. So if you allocated 3000 'slots' you may only be using 5 of them at one time. Since you are using the index stack to optimize re-use of empty slots the array never became fragmented. The ONLY drawback I found is that the number of actual elements in the array could only grow and never shrink. A possible fix is to keep a lower bound variable that would track the minimum index in the array. Then you would also need a Begin() and End() function that would return the actual beginning of the array and the actual end. So iterating from Begin() to End() does not mean you are iterating from 0 to the max number of elements in the array.

Example:

1. Array is empty. Array memory size is 4000 elements.
2. Add item
3. Array begin is 0
4. Array end is 1 (end always returns 1 past the current max index in the array)
5. Number of elements in array (array size()) is 1.
6. Add 10 items
7. Begin = 0, End = 10, Size = 11
8. Remove items at index 1, 5, 7, 9 (adds 1,5,7,9 to stack)
9. Add item - since stack is not empty use stack info for index of new item - 9 would be the index
10. Add item - stack not empty, 7 is now the index

I hope this illustrates clearly how the system works. In my Asteroids game I could use well over a total of 1000 asteroids in the level and yet only use 300 or so actual array indices. It all depends on how often objects die and how often they are created.

I like the idea you provided Bubba. I will try writing it, and see if I can get it working. Also, is it reasonable(Safe) to use a template class for the Manager?