Re: Is this bad coding ?
Hi,
I must admit that it was not clear for me what your problem exactly
was. First you say that because the same piece of data may be
concurrently accessed by at least two different threads, and at least
one of these threads may modify the data.
objIntersect->param = threadId; ///< NOT THREAD SAFE
pixelColor = objIntersect->shade(); ///< NOT THREAD SAFE because by
///< the time shade() is executed
///< param may have been changed
///< by another thread !!!!
This is a canonical example of a Data Race (at leat according to my
limited familiarity of threads). The standard solution to the data
race problem (although not the fastest) is to use a mutex (and a
lock):
{ // add a block to force
destructor
// call at a custom time.
scoped_lock lk( objIntersect->mutex ); // from now on, any oter thread
// that attempts to access
// objIntersect will be blocked
objIntersect->param = threadId;
pixelColor = objIntersect->shade();
} // end of block - call
destructor
// unblock other threads
Now, neither scoped_lock nor the type of objIntersect->mutex are
provided by C++, but the platform you are writing this code for surely
uses mutexes and locks or critical sections or some other equivalent.
If you need a platform-independent solution you could use Boost's
threading library.
Now, about your workaround with a copy:
if (intersection) {
Shader *shd = new Shader();
shd->Kd = threadId;
pixelColor = shd->shade();
delete shd;
}
I am surprised that the Shader constructor takes no arguments. I
thought you would make a copy of some master Shader. But whatever the
reason, if your problem is with memory allocation exclusively, you
could rewrite it to:
if (intersection) {
Shader shd;
shd.Kd = threadId;
pixelColor = shd.shade();
}
That is, make a copy on the stack rather that on the heap. THe code is
shorter, executes faster, you won't mess anything with deletes.
Regarding your note:
YES I KNOW I can pass threadId to the shade function as parameter but
lets say I CAN'T (because the code is in fact more complicated and
that I simple can not doit it;-).
If it is really comicated then you really have no other choice but to
use workarounds, but from what I know Locality, (which in this context
would mean using fuction arguments) is a very important thing when
multi-threading comes into play. It may be worth the time spent on
refactoring. Otherwise you may encounter subtle threading-related
bugs, and then you may not be able to trace them.
Sorry, I have nothing to say about your real question, which was
commenting on your walkaround, but I just couldn't get it.
Regards,
&rzej
--
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]