Game Development Community

FrameAllocator, FrameTemp and threads - LOGGED

by Manoel Neto · in Torque 3D Professional · 03/31/2011 (7:10 am) · 10 replies

It's well known that FrameAllocator and the FrameTemp<> template (which is a wrapper around the former) are not thread safe, since FrameAllocator relies on static data.

The problem is that anything that uses them is automatically thread-unsafe too, and in many cases it's not obvious. A good example is the convertUTF8toUTF16() and convertUTF16toUTF8() functions (the ones that allocate and return a new buffer), which are used in many other places, like the filesystem functions: doing any file operations (even things that shouldn't have collateral effects like checking if a file exists) from another thread can cause problems and crashes.

The easy way out is getting rid of FrameTemp<> and FrameAllocator in core code that should be thread-safe (like converting a string to UTF16), however I'm thinking if there isn't a way to make FrameAllocator thread-safe. Maybe we can use TLS (thread-local-storage) on the FrameAllocator static data, so each thread gets its own independent FrameAllocator heap?
NextNext Thread
PrevPrevious Thread
More from Torque 3D Professional


Associate Fyodor -bank- Osokin
Dedicated Logic
#1
03/31/2011 (9:34 am)
I've spent some time trying to make it thread-safe and failed...
so at the end decided to remove usage of FrameTemp<> and FrameAllocator in places which I can use in threads.. So far no side-effects.

Associate Manoel Neto
Default Studio Name
#2
03/31/2011 (9:44 am)
Yeah, FrameTemp<> can be replaced by TempAlloc<> without side-effects (the latter uses malloc() and free() internally, which are thread-safe). In unicode.cpp, this should have little impact in performance because the only instances when T3D needs to do UTF16 conversions is when passing strings to the OS.

However, I need to test if this is all it takes to make some filesystem functions thread-safe (I'd really love to be able to open and close files in other threads).

Anyway, i read some about TLS, and it seems to have some extra overhead. Seems the best choice is to keep the FrameAllocator as it is, but never use it on anything that can potentially be used in other threads (keep it only to render-related stuff).

Associate Fyodor -bank- Osokin
Dedicated Logic
#3
03/31/2011 (9:58 am)
I have done it like this:
UTF16* convertUTF8toUTF16( const UTF8* unistring)
{
   PROFILE_SCOPE(convertUTF8toUTF16_create);
   
   // allocate plenty of memory.
   U32 nCodepoints, len = dStrlen(unistring) + 1;
#ifdef _aw_frameAlloc_fix
   UTF16* buf = new UTF16[len];
#else
   FrameTemp<UTF16> buf(len);
#endif
   
   // perform conversion
   nCodepoints = convertUTF8toUTF16( unistring, buf, len);
   
   // add 1 for the NULL terminator the converter promises it included.
   nCodepoints++;
   
   // allocate the return buffer, copy over, and return it.
   UTF16 *ret = new UTF16[nCodepoints];
   dMemcpy(ret, buf, nCodepoints * sizeof(UTF16));
#ifdef _aw_frameAlloc_fix
   delete [] buf;
#endif
   return ret;
}

Associate Fyodor -bank- Osokin
Dedicated Logic
#4
03/31/2011 (10:57 am)
btw, in
Engine/source/core/strings/stringFunctions.cpp
in
S32 dSscanf(const char *buffer, const char *format, ...)
change:
static void* sVarArgs[20];
to this:
void* sVarArgs[20];
with this change you can save a lot of time trying to find out why Con::setData() is crashing when called from a thread.

Associate Manoel Neto
Default Studio Name
#5
03/31/2011 (10:58 am)
TempAlloc<> does the same you did, but it's safer because it frees the memory when it goes out of scope:
#include "util/tempAlloc.h"
UTF16* convertUTF8toUTF16( const UTF8* unistring)
{
   PROFILE_SCOPE(convertUTF8toUTF16_create);
   
   // allocate plenty of memory.
   U32 nCodepoints, len = dStrlen(unistring) + 1;
   TempAlloc<UTF16> buf(len);

   
   // perform conversion
   nCodepoints = convertUTF8toUTF16( unistring, buf, len);
   
   // add 1 for the NULL terminator the converter promises it included.
   nCodepoints++;
   
   // allocate the return buffer, copy over, and return it.
   UTF16 *ret = new UTF16[nCodepoints];
   dMemcpy(ret, buf, nCodepoints * sizeof(UTF16));
   return ret;
}

Quote:with this change you can save a lot of time trying to find out why Con::setData() is crashing when called from a thread.
I learned to simply not touch any of the scripting stuff from other threads, ever, except from Con::executef(), which automatically posts an event if called from another thread.

Associate Fyodor -bank- Osokin
Dedicated Logic
#6
03/31/2011 (11:04 am)
Quote:TempAlloc<> does the same you did, but it's safer because it frees the memory when it goes out of scope

Oh, haven't noticed on how the TempAlloc<> works, so yeah, this can be used at some places..
But I prefer to use new/delete instead of malloc/free, as if there are some memory-related problems, new will trigger exception while malloc will silently return null.

EDIT:
Another benefit of using new/delete is that if you use classes, it will call constructors and destructors.

Associate Manoel Neto
Default Studio Name
#7
03/31/2011 (11:06 am)
Quote:Oh, haven't noticed on how the TempAlloc<> works, so yeah, this can be used at some places..
But I prefer to use new/delete instead of malloc/free, as if there are some memory-related problems, new will trigger exception while malloc will silently return null.
That is a good point. The best solution, then, would be to update TempAlloc to use new/delete instead of malloc/free :)

Associate Fyodor -bank- Osokin
Dedicated Logic
#8
03/31/2011 (11:09 am)
Quote:
The best solution, then, would be to update TempAlloc to use new/delete instead of malloc/free :)

@ GG: "THREED-xxxx" someone? :)

Associate Manoel Neto
Default Studio Name
#9
04/04/2011 (11:36 am)
BTW, I just confirmed this doesn't make the filesystem thread safe. The entire thing uses StrongRefPtr<> and WeakRefPtr<> even for simple stuff like checking if a file exists, and those aren't thread-safe.

Associate David Montgomery-Blake
David MontgomeryBlake
#10
04/06/2011 (2:55 pm)
Logged as THREED-1557 for a feature request.