Skip to main content
GameDev.net gamedev.net
🔒 Locked

Multithreading (with DevIL)

Started by Physics Jan 14, 2009 at 5:41 AM 17 replies 18.2k views
Original Post
Physics
Physics
Okay, hopefully I am putting this in the right forum. This is more of a programming practice question than anything else in my opinion. This post may be a little long, since I will try to explain this as best as I can. I am the lead developer on DevIL, and I have recently been thinking about making DevIL thread-safe. Unfortunately, being away from programming for several years, I am not very familiar with thread safety and proper programming practices with it. Right now, DevIL keeps a list of images in an array that is accessed through ilGenImages/ilBindImage (similar to OpenGL). The images are pointers to structs that contain all the information about an image, including data. When you call ilBindImage, a global image pointer called iCurImage points to an entry in that array. Any functions that you call always operate on iCurImage. Obviously, this would not work at all in a multithreaded application (with multiple threads calling DevIL). The main part of making DevIL thread-safe would not be too terribly hard to implement, since I can just require an image pointer parameter to any function instead of using iCurImage. This is actually what DevIL did in its very first releases (when it was called OpenIL). I can also provide an interface to DevIL that mimics the current functionality, so hopefully people will not be put off by a new interface. Now comes the part that I am really not sure of. There are a lot of parameters that you can set via ilEnable/ilDisable and ilSetInteger. Things like setting the quality of .jpg files that are saved, compression types for files that support them, etc. These values are all currently stored in a global struct. I can see two possible paths to take here. I could require that the user pass a pointer of the struct that they fill out and control (or could be automatically filled out by a DevIL function with defaults). The other option that I see is to have the global struct like I have it now. There are not any pointers in this structure - just integers. This option would be preferable, but I am not sure how safe this is to do. What happens if a user is trying to change the value of one of the members of the struct while a function in DevIL is trying to access the same struct member to find out what it should do? The problem I see with the first option is that if I add a member to the struct in a future version of DevIL, programs compiled against an earlier version will not work properly. I have a similar problem with read/write functions. I allow the user to overload the read/write functions so that DevIL can load from either file streams or memory (along with any other method they wish to implement). I think that I will have to require that a structure with pointers to these functions will have to be passed when loading or saving, especially since one thread may want to load from memory while the other is trying to load from a file. Changing a global file access function mid-load would be pretty disastrous. Does anyone have any suggestions, comments or links to pages that describe proper thread-safe programming practices? [Edited by - Physics on January 14, 2009 6:09:08 AM]
Kambiz
Kambiz
How about making the global variables per thread and using the thread id to identify the right set to use? (Each thread has it's own independent 'copy' of DevIL)
Instead of g_state.some_option you will have g_state[get_thread_id()].some_option .

[Edited by - Kambiz on January 14, 2009 6:23:10 AM]
Physics
Physics
That makes sense, though I have a question on it. First off, how large would I make the g_state array? I could initialize it with ilInit and get users to call that before they create any threads. They could specify the maximum number of threads that they may use via a parameter.

The problem that I had that prompted me to look at making DevIL thread-safe was an issue I had with two programs using it at the same time. I have been using a program called ThumbView that generates thumbnail images in Windows Explorer with DevIL. I had a folder open that had some images in it while I was testing my image library with a program I wrote. The program wrote an image to that folder, so Windows called ThumbView to update the thumbnail. At this point, both my program and ThumbView were using DevIL. In my program, some global parameters changed in its copy of DevIL when they should not have, and I can only attribute it to ThumbView somehow using the same copy.

I always thought that programs using the same DLL would get their own copy of it in memory. It looks like I was wrong on that point.
Kylotan
Kylotan
Quote:
Original post by Physics
I could require that the user pass a pointer of the struct that they fill out and control (or could be automatically filled out by a DevIL function with defaults).

This sounds ok to me. If you can return all state back to the user, then it's their problem where they store it and where they access it from.

Quote:
The other option that I see is to have the global struct like I have it now. There are not any pointers in this structure - just integers.

The first rule of concurrent programming is to avoid trying to trick yourself into thinking that something is safe to modify across different threads/processes just because it's 'simple'. Imagine I read a height and a width field, and then attempt to write into the image based on those values. But then it turns out a second thread changed the height field but didn't get around to changing the width field yet - my data is now invalid, leading to oddness at best and crashes at worst.

Quote:
What happens if a user is trying to change the value of one of the members of the struct while a function in DevIL is trying to access the same struct member to find out what it should do?

Then the function in DevIL might do the wrong thing, because the value it read is no longer the value you wanted. Or a set of values you read become inconsistent because one or more of them changed half way through. It's not worth the risk. Give the struct to the user and let them manage it.

Quote:
The problem I see with the first option is that if I add a member to the struct in a future version of DevIL, programs compiled against an earlier version will not work properly.

Is forward binary compatibility a big deal? I wouldn't have thought so. But if it is, you can leave some reserved values in the structure which you don't currently use but which can be given meaning in future versions of the API by renaming the variable and using it accordingly. Only when you run out of these reserved values will you have to break the compatibility.

Quote:
Does anyone have any suggestions, comments or links to pages that describe proper thread-safe programming practices?

The first one is to remove all globals. Shared state is always a problem. The safest way to deal with it is for it to not exist - send all values by copying them.
Jesse Maurais
Jesse Maurais
Just lock/unlock a mutex around the sensitive memory to prevent it from being clobbered. Easiest solution I can think of.

https://computing.llnl.gov/tutorials/pthreads/

This is Unix specific but has good information on the topic.
Physics
Physics
Kylotan, thanks a lot for the explanation. Forward binary compatibility is important, so if I end up having to pass the structs around, I will just put reserved space at the end like you suggested. Jesse, a look at the mutex page definitely makes me think that this would be the most elegant way to do it. Is there much of a performance hit for locking/unlocking a mutex? This may be a silly question, but how hard is it to debug multiple threads in MSVC++? Normally, you can look at your call stack to see where you are, so I imagine it would change if you had multiple threads running.
Evil Steve
Evil Steve
Quote:
Original post by Physics
Is there much of a performance hit for locking/unlocking a mutex?
Not really, the main problem comes where you lock a mutex already locked by another thread, then the current thread stalls till the other thread releases the mutex (Unless you poll the mutex instead, but there's not often a lot you can do when waiting for a mutex).

Quote:
Original post by Physics
This may be a silly question, but how hard is it to debug multiple threads in MSVC++? Normally, you can look at your call stack to see where you are, so I imagine it would change if you had multiple threads running.
There's a "Threads" debugger window you can enable (Debug -> Windows -> Threads I think), which shows you all the threads running in your process. You can double click on one of them to bring up the call stack for that thread.
Things get a little interesting when you start doing step-over in multiple threads, since the OS context switching kicks in and triggers different threads, but it's not as confusing as you'd think - so long as you expect it to happen.
Kylotan
Kylotan
Quote:
Original post by Physics
Jesse, a look at the mutex page definitely makes me think that this would be the most elegant way to do it. Is there much of a performance hit for locking/unlocking a mutex?

I suppose it depends on your definition of elegance. Mutexes are the most primitive form of synchronisation, which are simple to employ and correspondingly simple to get wrong as soon as you step beyond the basics. They're elegant in the same way that assembly language is elegant...

In my experience, mutex locks are quite expensive. Evil Steve thinks differently. Perhaps it's worth you implementing them and profiling.

Quote:
This may be a silly question, but how hard is it to debug multiple threads in MSVC++? Normally, you can look at your call stack to see where you are, so I imagine it would change if you had multiple threads running.

Debugging programs that run in multiple threads isn't much different, as said above, but debugging multi-threading bugs is nigh on impossible. You need to write concurrent code that you can prove is correct from the start, because everything else is at the mercy of timing issues which may be completely unreproducible.
Jan Wassenberg
Jan Wassenberg
Quote:
They could specify the maximum number of threads that they may use via a parameter.

While it's easy from the library perspective to punt and ask higher-level code to supply parameters, it's terrible from the client's point of view. What if they use some other library like Thread Building Blocks or OpenMP and don't even know the maximum amount of threads?

Quote:
How about making the global variables per thread and using the thread id to identify the right set to use? (Each thread has it's own independent 'copy' of DevIL)
Instead of g_state.some_option you will have g_state[get_thread_id()].some_option .

This kind of thing would usually be accomplished via "thread-local storage", which avoids needing to know the maximum number of threads up-front. On Windows, this is implemented by setting aside a number of TLS slots in the TEB and doling out indices. Your app is then free to use the n-th slot in the (per-thread) arrays, which will typically hold a pointer to your per-thread data. If it's ok to be MSC/ICC-specific, you can use __declspec(thread) to have the management of the TLS index and per-thread data structure done for you. Otherwise, use POSIX pthread_getspecific.

Quote:
I always thought that programs using the same DLL would get their own copy of it in memory. It looks like I was wrong on that point.

The answer is, it depends. Usually both BSS and data segments are private, but if someone specifies the SHARED attribute via #pragma or a .def file, then the entire section will be shared.

HTH+HAND
E8 17 00 42 CE DC D2 DC E4 EA C4 40 CA DA C2 D8 CC 40 CA D0 E8 40E0 CA CA 96 5B B0 16 50 D7 D4 02 B2 02 86 E2 CD 21 58 48 79 F2 C3
Jesse Maurais
Jesse Maurais
As mentioned above, its true that mutexes will block if the memory is already locked, but this should not be a problem. When blocked the other threads wake up. Thats a good thing. Blocking gives process management over to the kernel. Let the kernel do its job.
gjaegy
gjaegy
I would second the advice to remove all global variables. This is actually evil and not a good-practice anyway.

I would suggest you first make Devil object-oriented. This will first remove the need of these global variables, and will consolidate Devil architecture.

Once the project has been better structured, you will be able to identify pieces of code that might need mutex protection.

This might sound like a lot of work (even if not so many classes would be required - from what I know from the source code), but I think this is actually mandatory to make Devil stable, structured and easier to use. Also, once done, making it thread-safe would be much easier than as in its current state.

Just my personal opinion though...
Gregory Jaegy[Homepage]
Physics
Physics
Quote:
Original post by gjaegy
I would second the advice to remove all global variables. This is actually evil and not a good-practice anyway.

I would suggest you first make Devil object-oriented. This will first remove the need of these global variables, and will consolidate Devil architecture.

Once the project has been better structured, you will be able to identify pieces of code that might need mutex protection.

This might sound like a lot of work (even if not so many classes would be required - from what I know from the source code), but I think this is actually mandatory to make Devil stable, structured and easier to use. Also, once done, making it thread-safe would be much easier than as in its current state.

Just my personal opinion though...


I agree. I have been thinking the past few days about rewriting it in C++, though it would still have a C-style interface so that other languages can use it. It will be a lot of work, and it will probably take awhile, since I am really busy with school. Thanks for your thoughts on it.
crawfis
crawfis
There is a lot of misguided advice on concurrent programming, so be wary of the web. The best way to achieve robust multi-threading is to make sure your software architecture is sound. Except for the global vars, everything seems fine. It also seems like these variables should indeed be global, which means having different instances per thread is not what you want.
1) Make the global variables encapsulated with proceedures.
2) On access to these variables, place a lock (mutex, Critical Section, etc.), copy the values out or set the values and then unlock. If you can copy the data over then things are much safer.

Things to avoid:
a) avoid locks everywhere. By placing a single lock in the encapsulated method that makes no calls back out to the system (or itself) you avoid the possibility of deadlock. Immutable data structures are a parallel programmers dream.
b) Worrying about performance. The performance warnings usually come from systems people who worry about microseconds and view the world as constantly calling locks. For your situation it seems that you rarely will call a lock, so it is a non-issue. Note, for the Win32 critical section, if the lock is not currently held it boils down to a no-op. Hence it is very very fast. Contention for locks will always cause performance problems, so it is best to limit the contention. The above should do that.

For the encapsulation, Object oriented would help (as would the Singleton Design Pattern), but you can get by with a single point of entry (a C function).

Protect your data and you should be okay.

crawfis
crawfis

Here is some C++ code that uses pThreads and MS Critical section. For C, encapsulate a separate _lock variable for each logic chunk of global memory you want to protect. Also hide the lock call and access to the _lock variable behind your encapsulated global variable. Except for the constructors, everything else is just a facade over the existing system support.

FYI, I am a Professor of Computer Science at The Ohio State University and do not usually check the forums. Some students were having some problems with DevIL, so I noticed your plea for help there. Great library so hopefully this is giving back a little.

Roger

-------------------Warning - student code below -------------------
#ifndef _H_CRITICAL_SECTION
#define _H_CRITICAL_SECTION

#ifdef _WIN32
#define WIN32_LEAN_AND_MEAN
#include
#else
#include
#include
#include

//Prefer non-portable (NP) recursive mutex if available
#ifdef PTHREAD_MUTEX_RECURSIVE_NP
#define PTHREAD_RECURSIVE_MUTEX_TYPE PTHREAD_MUTEX_RECURSIVE_NP
#else
#define PTHREAD_RECURSIVE_MUTEX_TYPE PTHREAD_MUTEX_RECURSIVE
#endif
#endif

//=============================================================
// Class Declaration
//=============================================================
class CriticalSection {
public:
CriticalSection();
~CriticalSection(); //NOTE: non-virtual destructor!

void lock();
void unlock();
bool tryLock(); //attempt to lock, return false on failure

#ifdef _WIN32
private:
CRITICAL_SECTION _lock;
HANDLE _currentThread;
#else
private:
pthread_mutex_t _lock;
#endif
};

//=============================================================
// Inline Implementations
//=============================================================
#ifdef _WIN32
//
// WIN32
//
inline CriticalSection::CriticalSection() {
InitializeCriticalSection(&_lock);
}

inline CriticalSection::~CriticalSection() {
DeleteCriticalSection(&_lock);
}

inline void CriticalSection::lock() {
EnterCriticalSection(&_lock);
}

inline void CriticalSection::unlock() {
LeaveCriticalSection(&_lock);
}

inline bool CriticalSection::tryLock() {
return (TryEnterCriticalSection(&_lock) != 0);
}


#else
//
// PTHREADS
//
inline CriticalSection::CriticalSection() {
pthread_mutexattr_t attr = {0};
//memset(&attr, 0, sizeof(attr));

#ifndef CRITICAL_SECTION_TEST
pthread_mutexattr_settype(&attr, PTHREAD_RECURSIVE_MUTEX_TYPE);
pthread_mutex_init(&_lock, &attr);
#else //Check for mutex initialization failure... (incurs minor overhead)
int result = pthread_mutexattr_settype(&attr, PTHREAD_RECURSIVE_MUTEX_TYPE);
if(result != 0)
{
switch(result)
{
case EINVAL: std::cout << "pthread_mutexattr_settype() failed with resultCode=EINVAL\n"; break;
case EFAULT: std::cout << "pthread_mutexattr_settype() failed with resultCode=EFAULT\n"; break;
default:
char buff[255];
sprintf(buff, "pthread_mutexattr_settype() failed with unrecognized resultCode=%i\n", result);
std::cout << buff;
break;
}
}

result = pthread_mutex_init(&_lock, &attr);
if(result != 0)
{
switch(result)
{
case EAGAIN: std::cout << "pthread_mutex_init() failed with resultCode=EAGAIN\n"; break;
case ENOMEM: std::cout << "pthread_mutex_init() failed with resultCode=ENOMEM\n"; break;
case EPERM: std::cout << "pthread_mutex_init() failed with resultCode=EPERM\n"; break;
case EBUSY: std::cout << "pthread_mutex_init() failed with resultCode=EBUSY\n"; break;
case EINVAL: std::cout << "pthread_mutex_init() failed with resultCode=EINVAL\n"; break;
default:
char buff[255];
sprintf(buff, "pthread_mutex_init() failed with unrecognized resultCode=%i\n", result);
std::cout << buff;
break;
}
}
}

inline CriticalSection::~CriticalSection() {
pthread_mutex_destroy(&_lock);
}

inline void CriticalSection::lock() {
pthread_mutex_lock(&_lock);
}

inline void CriticalSection::unlock() {
pthread_mutex_unlock(&_lock);
}

inline bool CriticalSection::tryLock() {
return pthread_mutex_trylock(&_lock);
}

#endif
#endif _H_CRITICAL_SECTION
Physics
Physics
Thanks a lot for the information, crawfis. I have some ideas on how to do this now.
gjaegy
gjaegy
FYI I am using boost::mutex and their implementation is quite good.

I guess you don't want to introduce any dependency to boost in the library, but it might give you another implementation inspiration ;)
Gregory Jaegy[Homepage]
Jesse Maurais
Jesse Maurais
The only change I would make to the code above is the use of strerror to format the return values of pthread functions. And to write them using cerr rather than cout.
Jesse Maurais
Jesse Maurais
Quote:
Original post by gjaegy
FYI I am using boost::mutex and their implementation is quite good.

I guess you don't want to introduce any dependency to boost in the library, but it might give you another implementation inspiration ;)


Can't believe I missed this before, but you could use boost::mutex without dedicating yourself to a long-term dependency problem, because it's being adopted as the standard in C++09 (as well as boost::thread). And from what I understand a portable sockets implementation for inter-process communication will be in there as well.
Antheus
Antheus
IMHO, I wouldn't do anything much about thread safety at library level.

Instead, I'd simply provide API that makes it possible to write thread-safe code.

The simplest approach to this is to provide context, and move each and every global into it (actually, only non-const/non-read-only variables). The API is then modified by requiring this context in every function call.

The change requires addition of create/destroy functions, and modification of public API with an extra parameter. The modifications could look something like this:
ILboolean initIL(ILContext * ctx);ILboolean cleanupIL(ILContext * ctx);ILboolean ilLoadImage(ILContext * ctx, char *FileName);ILboolean ilLoad(ILContext * ctx, ILenum Type, char *FileName);ILboolean ilLoadF(ILContext * ctx, ILenum Type, ILHANDLE File);ILboolean ilLoadL(ILContext * ctx, ILenum Type, ILvoid *Lump, ILuint Size);ILboolean ilLoadPal(ILContext * ctx, char *FileName);


While API breaking change, it has several advantages:
- API change can effectively be ignored by those not interested into threading
- Old-style API can be provided for compatibility where ILContext is still global
- Thread-safety is possible, but left to the user if needed
- It does not require any specific thread or OS library
- Change is fairly mechanical and simple to perform (move all statics into ILContext)
- The change itself requires minimal testing since aside from putting variables into a struct nothing changed

How to use such library in threaded environment?
- allocate ILContext inside TLS
- let user acquire locks as they choose, with the API they choose, around the operations they need to
- via C++ RAII wrapper than encapsulates entire operations
- up to user

Quote:
There are a lot of parameters that you can set via ilEnable/ilDisable and ilSetInteger.

The reason why I suggest this approach has to do with ACID problem. Even if each individual ilEnable call is made thread-safe, the sequence of operations itself isn't a transaction, and cannot be made into one without changing the API considerably.

By moving global state into user-allocated object (see any major C API, mysql or such), user gets full control over how and when internal state is modified. That gives sufficient and required control that is needed for thread-safe use.

But IMHO, API isn't suitable for retrofitting thread-safety internally into library.

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.

Sign in to reply to this topic.