rcAllocSetCustom and dtAllocSetCustom not threadsafe

[GoogleCodeExporter]

I know that you've tried to minimise impact on the API, however having 
static/global variables used for custom memory handling isn't ideal as it's not 
threadsafe. You'd be better off passing in a struct to functions that allocate 
memory - the struct would have the alloc/free function ptrs and a userdata*. Or 
add it into the rcBuildContext struct (and do something similar for detour).

Original issue reported on code.google.com by armstron...@gmail.com on 23 Aug 2010 at 4:07

[GoogleCodeExporter]

Can you describe what kind of problems it introduces? I understand that setting 
the global vars is not thread safe. If they are set once early in the 
initialization, are there problems afterwards?

Custom memory management in C++ is so bad and crippled. I tried a version where 
alloc was part of the build context, and it was ugly and huge pain to maintain.

It is unlikely that I will change it now, I will consider it.

This might be the reason for me to rewrite everything in C, though ;)

Original comment by memono...@gmail.com on 23 Aug 2010 at 4:29

[GoogleCodeExporter]

>>This might be the reason for me to rewrite everything in C, though
No no no...C++ is fine :)

Imagine I have two path finders (dtNavMeshQuery) running on different threads. 
I will probably want them to use their own heaps so that there isn't contention 
for the memory. At the moment they'd both allocate/dealloc via dtAlloc/dtFree 
and, due to the lack of context info that comes through to these functions, 
they'll probably have to share the same heap. This heap would then need to be 
thread safe - slowing down memory alloc/dealloc. If you could get some context 
info (userdata*) through to these functions then the user could easily 
ascertain which heap to use.

I hope that makes sense? 

I'm not really sure why a virtual alloc/dealloc function on the rcBuildContext 
class is ugly? It should look very similar to your current code:

return new(dtAlloc(sizeof(dtNavMesh), DT_ALLOC_PERM)) dtNavMesh;

would become:

return new(ctx->alloc(sizeof(dtNavMesh), DT_ALLOC_PERM)) dtNavMesh;

Original comment by armstron...@gmail.com on 23 Aug 2010 at 5:25

[GoogleCodeExporter]

That is a good case. I try to find solution to that.

C++ is crappy in a sense that there is placement new, but not placement delete 
(plus some other stupidities). In above case there is no practical way to 
associate that new to a delete so you need to store pointer to the allocator 
and do silly things.

That means that I need to arrange the code so that there is no constructors and 
destructors. It is kind half way there anyways.

Anyways, all these are lame excuses :) C++ custom memory management is stupid, 
I will try to find solution. I cannot take immediate action, though.

Original comment by memono...@gmail.com on 23 Aug 2010 at 5:39

• Changed state: Accepted

[GoogleCodeExporter]

What would you expect placement delete to do? You can directly call a 
destructor and not free the memory...which would be the equivalent of placement 
new. Typically delete can get the extra info required from the ptr that's being 
freed.

Original comment by armstron...@gmail.com on 23 Aug 2010 at 8:56

[GoogleCodeExporter]

I would have loved to use:

nm = new(allocator) dtNavMesh;
....
delete(allocator) nm;

but that delete does not exists in ay meaningful manner. Or on some compilers 
it might, but the pointer that is passed to the function is different on 
different platforms. Sometimes you can dig up the actual pointer by ptr-4, but 
sometimes not. Plus there are some cases where the destructor simply is not 
called at all.

The result is that you end up calling the destructor by hand, and there, 
suddenly your are manually managing everything C++ promised to do for you. And 
then you notice that the only reason you are using C++ is because it allows you 
to write a type safe version of min and max. </ rant>

Original comment by memono...@gmail.com on 24 Aug 2010 at 6:58

[GoogleCodeExporter]

typesafety is grrrreat! :)  (you do get polymorphism too, and templates). C++ 
definitely has a lot of flaws...so I'm going to stop well short of saying "it's 
perfect" :)

Further to my example of why passing in the alloc/dealloc functions would be 
better than global functions:
What happens if I want the dtNavMesh class to use a different heap from the 
dtNavMeshQuery? How can I do that?

Original comment by armstron...@gmail.com on 24 Aug 2010 at 8:47

[GoogleCodeExporter]

I hear you. I will try to find a way to make it happen.

Original comment by memono...@gmail.com on 24 Aug 2010 at 8:52

[GoogleCodeExporter]

Would it be possible to pass FILE and LINE info through to the alloc/free 
functions? Using a Recast #define to turn behaviour on/off (because I wouldn't 
really want it in production builds...i.e. you could just pass null and 0, no 
need to change the actual function signature in production builds)

Original comment by armstron...@gmail.com on 25 Aug 2010 at 3:20