Best practice to free memory.

[cyberium]

Hi

I one more time invoke your help.

I am faced lot of situation like this

[ code=cpp ]
struct myStruct
{
   uint32 data;
   ...
};

typedef myStruct* pMyStruct;

pMyStruct GetMyStruct()
{
   if (m_DummyData)
   {
        pMyStruct info = new myStruct; // create mystruct
        info->data = 0; // assign something
        return info;
    }
    return NULL;
}
[ /code ]

So if i call MyFunction i know i must do :

pMyStruct info = GetMyStruct();
if (info)
{
   // Do something
   delete info; //delete mystruct;
}

It's simple. But imagine this info is imbriqued in another struct returned by another function.

struct myStruct
{
   uint32 data;
   ...
};

typedef myStruct* pMyStruct;

struct mySecondStruct;
{
   uint32 value;
   pMyStruct myInfo; 
}

typedef mySecondStruct* pMySecStruct;

pMyStruct GetMyStruct()
{
   if (m_DummyData)
   {
        pMyStruct info = new myStruct; // create mystruct
        info->data = 0; // assign something
        return info;
   }
   return NULL;
}

pMySecStruct MyFunction()
{
   if (m_OtherData)
   {
        pMySecStruct info = new mySecStruct; // create struct
        info->value = 0; // assign something
        info->myInfo = GetMyStruct();
        return info;
   }
   return NULL;
}

Now you see i must do something like

pMySecStruct info = MyFunction();
if (info)
{
   // Do something
   delete info->myInfo; //delete mystruct;
   delete info; //delete mysecondstruct;
}

This is pain in big project. So i usualy avoid this doing auto delete in destructor :

struct myStruct
{
   uint32 data;
   ...
};

typedef myStruct* pMyStruct;

struct mySecondStruct;
{
   uint32 value;
   pMyStruct myInfo;

   ~mySecondStruct()
   {
       if (myInfo)
           delete myInfo;
   }
}

typedef mySecondStruct* pMySecStruct;

pMyStruct GetMyStruct()
{
   if (m_DummyData)
   {
        pMyStruct info = new myStruct; // create mystruct
        info->data = 0; // assign something
        return info;
   }
   return NULL;
}

pMySecStruct MyFunction()
{
   if (m_OtherData)
   {
        pMySecStruct info = new mySecStruct; // create struct
        info->value = 0; // assign something
        info->myInfo = GetMyStruct();
        return info;
   }
   return NULL;
}

Now the call is usual

pMySecStruct info = MyFunction();
if (info)
{
   // Do something
   delete info; //delete mysecondstruct;
}

Recently i talked with more experienced programmer than me.

He told me to use more often Reference variable in place of returned pointer and olso i must not be affraid to use directly structure on returned value to let compilator handle all memory stuff.

So in this case code is pretty more simple.

struct myStruct
{
   uint32 data;
   ...
};

struct mySecondStruct;
{
   uint32 value;
   myStruct myInfo; 
}

myStruct GetMyStruct()
{
   if (m_DummyData)
   {
        myStruct info; //no need to do new
        info.data = 0; // assign something
        return info;
   }
}

mySecStruct MyFunction()
{
   if (m_OtherData)
   {
        mySecStruct info;
        info.value = 0; // assign something
        info.myInfo = GetMyStruct();
        return info;
   }
}

Now you see i must do something like

mySecStruct info = MyFunction();
// Do something with info

For what i know the second version have less risk (or zero) to have any problem with memory usage. But it's not optimised at all due to lot of memory copy.

My friend told me it's not a big lost than i imagine... Actual compilator are able to optmize code for me...

Can you confirm me this point with any compilator?

Do the work to handle correctly memory and pass only pointer is still needed with actual compilator?

Sory for this big post.

ps : can someone explaine me how code=language bbcode work?

[Ambal]

Use smart pointers like auto_ptr<>, shared_ptr<> and week_ptr<> for your needs. They will solve all issues with your memory management.

[faramir118]

My friend told me it's not a big lost than i imagine... Actual compilator are able to optmize code for me...

Can you confirm me this point with any compilator?

I'm not sure if compilers perform this sort of optimization. If they do, it's only when you aren't modifying the parameter.

If you want, you can just use pass-by-reference to force this 'optimization':

void passByRef(myStruct& m);                // allows modification of m to persist after passByRef returns
void passByConstRef(const myStruct& m);     // m is read-only (compile error if you try to modify it)

myStruct& retByRef();                       // allows modification of return value
const myStruct& retByConstRef();            // lhs must be declared const

Just watch out for things like this:

struct myStruct
{
   int data;
   myStruct() : data(0) {}
   ~myStruct() { cout << data << " says goodbye!" << endl;
};

void passByRef(myStruct& m)
{
   m.data = 1;
   m = myStruct();    // destructor is called on m AFTER the constructor (for me, anyway)
}

[Ambal]

Also pay attention on Copy Constructor A(A const& ) and Assign Operator A& operator=(A const&) if you are willing to transfer ownership of object internals to another object.

[cyberium]

Thank you Both.

I think i will use Reference just because i already use it lot of time where smart_ptr are completly new for me.

It's seem these auto ptr class are not so hard to learn and handle lot of case for me so i will do some learning about that asap. Thank Ambal pointing me that.

[cyberium]

Finaly using Reference variable for function result is not so elegant solution.

Using shared_ptr seem to be a very good solution.

Ambal, or anyone else, can you confirm me shared_ptr can be used on mangos code without any problem? (i mean gcc compilation or other unknow situation)

[Ambal]

The only problem using smart pointers in mangos is Vladimir

http://www.boost.org/doc/libs/1_43_0/libs/smart_ptr/sp_techniques.html - this document from Boost should help you understand how to utilize shared_ptr<> effectively.

P.S. Also take a look into this document from Boost about thread-safety of shared_ptr<>: http://www.boost.org/doc/libs/1_37_0/libs/smart_ptr/shared_ptr.htm#ThreadSafety

[darkstalker]

if you properly use destructors only 1 delete call should be needed

[maly32167]

The only problem using smart pointers in mangos is Vladimir

It will be an offtopic but If I may know, can you explain why ?

I think implementing shared_ptr would be really efficient on mangos, especially as its stable-going project and I can imagine that implementation would be succesful compared to ascent which failed at its implementation long time ago.

[cyberium]

The only good raison i see is, is not goot to mix different way to handle pointer. So the best is to have complete coherent code not using smart_ptr.

Anyway i am currently modifying my DungeonFinder implementation with the shared_ptr class because i cannot ignore the benefict of using them.

Just 3 of them

- Code is more easy to read.

- I dont bother about any delete.

- Memory management is more advanced than actual version. (anti memory leak, handle exception)

I know this can reduce chance to integrate my DF implementation but it's not my absolutly goal. Iam firstly here to learn

Greetings

[Spp_]

The only good raison i see is, is not goot to mix different way to handle pointer. So the best is to have complete coherent code not using smart_ptr.

Anyway i am currently modifying my DungeonFinder implementation with the shared_ptr class because i cannot ignore the benefict of using them.

Just 3 of them

- Code is more easy to read.

- I dont bother about any delete.

- Memory management is more advanced than actual version. (anti memory leak, handle exception)

I know this can reduce chance to integrate my DF implementation but it's not my absolutly goal. Iam firstly here to learn

Greetings

I ended with the same problem in my DF implementation. Now i am In the process of moving the use of pointers to references to avoid having to deal with new and delete. Code ended being easier to read. The first try was having warnings about returning local variables, but later fixed those problems returning cost references and problem solved.

Will take a look at the class Ambal talked about, just to learn, as i don't think going to re-redo it.

As far as i've read (haven't done any test myself) compilers can optimize all those calls using value parameters. Anyway i still prefer to use const & wherever i can

PD (offtopic) I am waiting to see your DF implementation it's always good to see how another one has solved the same problem to see any improvement you have added

[Ambal]

It will be an offtopic but If I may know, can you explain why ?

Once I suggested to use scoped pointers "auto_ptr<>" in DB code to manage SQL query results - this way we could get rid of ALL memory leaks caused by un-released resultsets, but Vladimir said that these pointers will degrade performance ( which was very funny argument since auto_ptr<> is completely inlined in release builds ).

Be very careful using shared_ptr<> - try to pass these objects by reference as an input function parameters since reference counting is expensive operation. Also note, Boost::shared_ptr<> is a VERY heavy-weight solution . It should be used in only three cases:

1) you are unable to find any other thread-safe shared_ptr solution

2) performance is not your major concern

3) you need to be able to control object lifetime, e.g. destroy the object and reset ALL pointers to the same object to NULL by calling boost::shared_ptr::reset() function - might be extremely helpful functionality but it is very expensive operation. As an example:

a) you create a mob

b) your mob is referenced in thread manager, in attacker list, as aura target etc..

c) your mob dies and you need to remove this object safely from the world.

d) you call shared_ptr::reset() function and ALL pointers to this mob are set to NULL. No crashes, no daggling pointers, no memory leaks - wooohooo

[cyberium]

Be very careful using shared_ptr<> - try to pass these objects by reference since reference counting is expensive operation.

I will try.

2) you need to be able to reset ALL pointers to the same object to NULL by calling boost::shared_ptr::reset() function - might be extremely helpful functionality but it is very expensive. As an example:

a) you create a mob

b) your mob is referenced in thread manager, in attacker list, as aura target etc..

c) your mob dies and you need to remove this object safely from the world.

d) you call shared_ptr::reset() function and ALL pointers to this mob are set to NULL. No crashes, no daggling pointers, no memory leaks - wooohooo smile

I don't think all actual solution :

- need to call delete mob somewhere.

- need to constantly be sure we have correct data pointed by copied ptr.

- no "try catch" system implemented.

is better than using shared_ptr.

If i take my previously exemple here is the final code

struct myStruct
{
   uint32 data;
   ...
};

typedef std::shared_ptr<myStruct> pMyStruct;

struct mySecondStruct;
{
   uint32 value;
   pMyStruct myInfo;
}

typedef std::shared_ptr<mySecondStruct> pMySecStruct;

pMyStruct GetMyStruct()
{
   if (m_DummyData)
   {
        pMyStruct info(new myStruct); // create mystruct
        info->data = 0; // assign something
        return info;
   }
   return NULL;
}

pMySecStruct MyFunction()
{
   if (m_OtherData)
   {
        pMySecStruct info(new mySecStruct); // create struct
        info->value = 0; // assign something
        info->myInfo = GetMyStruct();
        return info;
   }
   return NULL;
}

Now the call is pretty more simple than ever :

pMySecStruct info = MyFunction();
if (info)
{
   // Do something with info
   // No need to call any delete and no delete have seen in the code
}

No need to call any delete and the code is better than before in term of memory usage.

shared_ptr template is little more cpu consuming but it's a price to have all correct test done to avoid any bad memory usage.

But in my case i admit i use it more for code simplication/reading than for more "anti memory leak" property.

Greetings

[Ambal]

Update for Boost::shared_ptr<> - currently it does not maintain the list of all places where protected object is referenced. So you can use this implementation w/o any worries about performance. Just be sure that you use Boost version >= 1.33.0 - recent versions make use of atomic operations and are much faster to previous revisions.

[faramir118]

I just had a rough time with this scenario:

class ApiClass
{
   // some member variables
};

class StupidApiClass
{
   BadClass(ApiClass &myClass) : m_apiClass(apiClass) { ... }

   ApiClass &m_apiClass;
};

void api_func(ApiClass &apiClass)
{
   BadClass b = new (apiClass);
}

In case you are wondering, ApiClass = ACE_Message_Block

With these horrible semantics, it's no wonder we have strange crashes in network code...