[Gdal-dev] Multi-threading Support

[Marek Brudka]

Frank,

>I'm back from vacation and will try to deal with outstanding issues
>as I can. 
>  
>
Fine. I hope you enjoyed you vacations :-).

>>In my company we assumed we cannot change PROJ.4, in particular
>>we cannot neither place *pj*_errno in thread specific storage nor
>>modify PROJ.4 to return error codes rather than to modify
>>global variables.
>>
>>This implied a global PROJ.4 mutex in ogrct.cpp and locking in the
>>following function/methods:
>>
>>LoadProjLibrary(),
>>char *OCTProj4Normalize( const char *pszProj4Src ),
>>OGRProj4CT::~OGRProj4CT(),
>>int OGRProj4CT::Initialize( OGRSpatialReference * poSourceIn,
>>                            OGRSpatialReference * poTargetIn )
>>int OGRProj4CT::TransformEx( int nCount, double *x, double *y, double *z,
>>                             int *pabSuccess ).
>>
>>Because all PROJ.4 related methods are enclosed in ogrct.cpp, PROJ.4 lock
>>has not to bo exposed outside GDAL. This was sufficient to achieve
>>reentrancy of all OGRSpatialReference related methods in OGR (at least in
>>our tests :-) )
>>    
>>
>
>Yes, I likely ought to incorporate such a change at my end as well.
>As mentioned on the proj mailing list, low level reprojection code in
>PROJ.4 is re-entrant, but aspects of the datum transformation and error
>handling are not. 
>  
>
The solution above works nicely for us. It costs 30 minutes (with
compilation) of work if CPLMutex and CPLMutexGuard are avalaible.

>>>For background, I have made a number of additions to cpl_multiproc.cpp/h
>>>including the addition of a mutex holder class.
>>>      
>>>
>>Great! I made a similiar class when was working on GDAL thread safety
>>few weeks ago. I even planned to post my changes, but I see I'm too
>>late :-). Let me share then my multithread experiences to make
>>GDAL a better software:
>>
>>1. It is better to separate the creation of the mutex from its locking
>>(see eg. boost:thread, ACE::ACE_Mutex). These activities are quite
>>different and it is better to not mix them.
>>Currently, in GDAL mutexes are locked during creation.  It is not very
>>convenient and GDAL by itself is a proof for this statement. In
>>1.2.6 mutexes were employed only in GDALwarp::alg, and they were
>>released just after they were created! Why to make such simple
>>operations so complex then?
>>    
>>
>
>This has indeed been a point of confusion for me as well.  However,
>I am now somewhat loath to change the convention.
>
Changing this convention in GDAL 1.2.6 ( in GDALwarp) costed me an
hour of work. But even in GDALwarp, in the only place in 1.2.6 where
mutexes were employed, the first step after mutex creation was to
release it. This convention seems to be not a very practical one then..

>  In fact, I now
>normally use the CPLCreateOrAcquireMutex() to acquire mutexes 
>deferring mutex creation to the first attempt to acquire them.  Otherwise
>it was sometimes messy to ensure that they were created in advance at
>a point with no other race conditions.  The CPLCreateOrAcquireMutex()
>is intended to ensure that two threads that try to create a given mutex
>at the same time will not have a race condition. 
>  
>
You will never avoid such races without additional (static) mutex created
without locking during init. It is impossible. I see in
CPLCreateOrCreateMutex
that you know this.

But what does it mean? It means that a convention is not very convenient.
Please forgive I make some statements "ex cathedra", but I
wrote too many programs and taught too many programmers and I am also
a little tired when talking about fundamentals which should obvious for any
intermediate multithread programmer.  Nevertheless, here is more elaborated
rationale for such mutex design:
http://www.boost.org/doc/html/threads/rationale.html#threads.rationale.locks

Let me recall also that boost library
http://www.boost.org/doc/html/threads.html
is going to be a part of the forthcoming C++ standard release. If about
mutex guard,
it is from ACE, namely:

[SchmidtStalRohnertBuschmann] Pattern-Oriented Architecture Volume 2.
Patterns
for Concurrent and Networked Objects. POSA2. Douglas C. Schmidt, Michael,
Hans Rohnert, and Frank Buschmann. WileyCopyright © 2000,
or
http://www.dre.vanderbilt.edu/Doxygen/Stable/ace/classACE__Guard.html

>>2. Usually it is good to have a class for holding mutex (similiar to
>>CPLMutexHolder). But this class should be responsible for holding
>>a mutex only. It should expose and interface for direct
>>acquire/try_acquire/release, but should not lock a mutex by
>>itself in constructor (see eg. boost::thread, ACE::ACE_Mutex). In
>>fact such mutex holder should be a C++ wrapper around
>>native OS functions.
>>    
>>
>
>Well, this is a C++ wrapper around native OS functions, but I found
>it convenient to have the constructor acquire the mutex in most
>situations.   This means I just need to instantiate the holder in an
>appropriate local context such that it is released when the object
>falls out of context. 
>  
>
This is the role of CPLMutexGuard described below.

>I don't mean to imply that the CPLMutexHolder is the best abstraction
>of a mutex holder that could be developed.  But it is simple, and easy 
>for my purposes.
>  
>
I've been using classes similiar to CPLMutex and CPLMutexGuard for a
long time and they are also simple and convenient. Here is a sample from
modified ogrct.cpp using CPLMutex and CPLMutexGuard:
/* ==================================================================== */
/*      PROJ.4 interface stuff.                                         */
/* ==================================================================== */
...
// this is new line, plain static mutex invisible outside ogrct.cpp
static CPLMutex projLock; // A mutex to synchronize the access to PROJ.4
methods
...
static int LoadProjLibrary()

{
    CPLMutexGuard guard( projLock );
    .... //  LoadProjLibrary() implementation
} // here mutex is released by guard

>I would note that it makes no attempt to recover in the case that an 
>attempt to acquire a mutex times out.
>  
>
CPLMutex::acquire is parameterized by timeout. Constructor
is not, because it should not acquire mutex!

>>3. It is good to not use mutexes directly, but via guard class (see eg.
>>boost::thread,  ACE::ACE_Mutex) . The guard class ("the creation
>>of the object is resource allocation", Stroustrup) should acquire a
>>mutex when created and release it  when destroyed. Additionally,
>>a guard should expose a release method. In general, in larger
>>projects it is better to use guards rather than direct
>>direct locking, as this prevents some deadlocks.
>>
>>4. To serve specific purposes better, a guard class should have a
>>copy semantics aka. std::auto_ptr, namely left hand side guard
>>during copying (copy ctor, assignment operator) should grab
>>a mutex (disown) from RHS guard. This way, one
>>may return a guard to synchronize the access to an object and
>>be (almost) sure the mutex will be released. It is better solution
>>than exposing a mutex directly.
>>
>>I've attached an examplary cpl_mutex classes ( which fullfills 1-3
>>requirements) to this e-mail. If you find it usefull, please consider
>>using them in GDAL.
>>    
>>
>
>I read through the CPLMutex class you provided but I am not clear on
>it's utility or the rationale of a "guard".   I appologise for not having the
>time to follow up on some of your references. 
>  
>
Guard is a part of functionality related with acquiring and releasing
the mutex. I hope the code snapshot above explains everything.

>>Thread specific storage is avalaible on many platforms, but
>>unfortunatelly one cannot expect that a single macro solves
>>the problem in portable way. A more serious and portable
>>attempt is to create some wrappers for accessing thread specific
>>keys and then to define all those thread variables as a function
>>calls which get values from thread specific storage. Obviously, such
>>solution is more advanced, and requires much more work than
>>simple declspec or __thread. However, it seems that this is the
>>only way to achieve true portability across many platforms
>>(eg. see ACE).  Do you plan to restrict multithread GDAL to
>>win32/gcc 3.3 only?
>>    
>>
>
>I am aware of methods using pthreads for managing thread local
>data that would be more portable.  However, it requires a non-trivial
>amount of additional work for each thread-local variable, and am not
>currently keen on doing that.   For the time being I am prepared to 
>accept that proper multi-threading support for GDAL is only supported
>on win32 and platforms with modern gcc support - assuming that
>proves sufficient for the end-client. 
>
>If nothing else, having the CPL_THREADLOCAL macro in the code
>makes it easier in the future to identify variables that need to be
>addressed if a more comprehensive approach is needed.
>  
>
Thread specific storage is a part of boost::thread library. It's easy
to use and avalaible for many platforms.

I have a proposition, I made already once. Frank, make
GDAL dependent on boost library. This library contains
solutions for next C++ standard release. Boost is
avalaible on many more platforms than GDAL, and is
really easy to learn and use. Boost is written much better
than anything I saw (well, maybe except some parts of ACE :-) )
and can replace many parts of GDAL::port library. This way
porting GDAL to new platforms will be easier. Boost have
packages for most linuxes, there are distributions for windows.
Boost has commercial support and is recognized a place from
where C++ standard emerges now. I suppose boost would serve
GDAL very well and made GDAL better and avalaible on more
platforms.

I have also one question. Could you give me write access to gdal
CVS, please? Why? In my company we GDAL in our CVS repo,
because we have to remove GDAL bugs by our own (especially in
multithread, memory management and reference counting). I do not
need GDAL CVS access for this, but it's just a pity these changes
are not avalaible for GDAL community. I also do not like merging our
changes
with each new GDAL release. Let me mention also I'm too
busy to report and discuss such bugs each time I recognize or fix them.
A bug report and a discussion which usually follows it takes more time
than fixing bugs! It is not the best way to develop the software.

BTW. There is a serious design problem with reference counting and
memory management in multithread GDAL applications. It makes
the read only sharing of OGRSpatialReference ( and remaining
reference counted classes ) between threads almost impossible . This
problem is a direct implication of lack of correct reference counting via
intrusive smart pointer classes as well as by invalid convention for
mutexes.

Thanks

Marek