[RFC-24] Updated public draft

[Umberto Nicoletti]

On 2/23/07, Tamas Szekeres <szekerest at gmail.com> wrote:
> Umberto,
>
> This proposal is compelling and designates the right direction to
> follow. However I would suggest some modifications in the concept
> mainly for making the things simpler and safer. But I couldn't make an
> exhaustive analysis for every aspect of the proposal and none of the
> code fragments below were tested.
> Generally speaking the level of the difficulty of this effort is as
> large as the problems it solves  :-)
>
> 3.1 Implement a reference counter
> -------------------------------------------------
> 1) I see no problem about implementing the refcount approach by
> default, since the default behaviour will remain the same in the
> mapserver core:
Tamas,
you suggest we add the refcount member instead of the global map keyed
by memory address?

Anyway if we encapsulate the INCR/DECR properly we'll be able to
switch between them later. I'd prefer the hash because it less
invasive on the data structures and makes for an easier implementation
(less #IFDEF, etc). More over it is easier to keep it only mapscript
related, thus leaving the CGI unaffected (smaller structures, less
memory and no unnecessary refcount housekeeping)

> Every newly created object are belong to a parent (except for mapObj)
> and therefore the refcount of the children should be incremented. The
> destroy function of the parent will call the destroy function of the
> children (as it stands).
>
> If we would even want to make it configurable USE_REF_COUNT would be
> better to reflect the entity of this feature.

You mean USE_REF_COUNT instead of USE_MAPSCRIPT? We'll find an
agreement, I'm that good on names ;-)

>
> 2) The destroy function of the objects may be called from different
> threads so it should be protected by locks (when USE_THREAD is
> enabled). I would prefer using a per-object lock.
>

I don't know about C#, but Java and probably Python all have a single
thread that performs GC so this is unnecessary. Also I've never seen
this possibility mentioned on the swig docs, can you check?
If it is needed only for some languages we should wrap it with
preprocessor macros.

> #ifdef _WIN32
> #define MS_LOCK HANDLE
> #define InitLock(obj) (obj->lock = CreateMutex( NULL, FALSE, NULL );)
> #define GetLock(obj) (WaitForSingleObject( obj->lock, INFINITE );)
> #define ReleaseLock(obj) (ReleaseMutex(obj->lock);)
> #else
> #define MS_LOCK pthread_mutex_t
> #define InitLock(obj) (pthread_mutex_init(&obj->lock);)
> #define GetLock(obj) (pthread_mutex_lock( &obj->lock );;)
> #define ReleaseLock(obj) ((pthread_mutex_unlock( &obj->lock);)
> #endif
>
> Eventually the following operation sequences might be used:
>
> a) MS_REF_INCR / MS_REF_DECR
>
> #define MS_REF_INCR(obj) GetLock(obj); obj->refcount++; ReleaseLock(obj);
> #define MS_REF_DECR(obj) GetLock(obj); obj->refcount--; ReleaseLock(obj);
>
> b) When destroying and object:
>
> void freeLayer(layerObj *layer) {
>   GetLock(layer);
>   if (--obj->refcount > 0)
>   {
>     ReleaseLock(layer);
>     return;
>   }
>
> // do the destroy stuff, Call the destroy function of the children,
> Set the back references of the children to NULL.
>
>   ReleaseLock(layer);
> }
>
>
> 3.2 Add references to the mapscript wrapper object
> --------------------------------------------------------------------------
> This effort can be ignored, see below.
>

I'm afraid we can't. There are many places where the code is like this:

layer->map->layers[layersorder[i]]

the parent object is accessed by the child without checking. If this
happens and the parent has been gc'ed we obtain a segfault which
results in a crash of the hosting language, which is exactly what we
want to avoid *at any cost*.

>
> 3.3 Change arrays of structures in arrays of pointers
> ---------------------------------------------------------------------------
> Since we will change the object arrays to object pointer arrays it's
> safe to destroy the parent when the child is still alive. Therefore
> there's no need to keep a reference to the parent at the wrapper
> object.

This is wrong: see my reasoning above.

> However the the refcount of the child object should be incremented
> upon adding it to the parent and decremented when removing the
> parent-child relationship. (When destroying the parent the destroy
> function of the children will also be called by default)
> When adding the child object to the parent we should add it's pointer
> to the array instead of creating a copy of that object (Your patch did
> not reflect this behaviour as I could see).

It is written in the comments that this is not ready yet.

>
> 3.4 Keep parent references and C internal structure in sync
> -------------------------------------------------------------------------------------
>
> The only SWIG customizations I would want to see are:
>
> a) modifying the destroy  function of the wrapper so as to destroy the
> underlying object every time regardless of the actual SWIGCMemOwn
> setting.  For the C# binding it looks something like this:
>
> %typemap(csdestruct, methodname="Dispose", methodmodifiers="public") SWIGTYPE {
>     if(swigCPtr.Handle != IntPtr.Zero) {
>       swigCMemOwn = false;
>       $imcall;
>     }
>     swigCPtr = new HandleRef(null, IntPtr.Zero);
>     GC.SuppressFinalize(this);
>   }
>

We only have to evaluate if it costs more to remove the newobject
statements or to  write a typemap for every object's destructor, but I
agree on the concept.

> b) Adding the %typemap(out) to increment the refcount when passing the
> objects to the target language.
>
> %typemap(out) struct map_obj*, mapObj*, layerObj*,  ..... {
>         MS_REF_INCR(($1_ltype)$1);
>         $result = $1;
> }
>
> The parent back references of the children (in the C structs) should
> be updated when establishing or removing the parent-child
> relationships (protected by locks). These pointers will not affect the
> refcount of the parent (so as to avoid the circular references).
>

Agreed.

> 3.5 Destructors obey the reference counter
> --------------------------------------------------------------
> Should take care of the multithreading issues have been mentioned above.
> #2 should be omitted IMHO ("When the children are GC'ed earlier than
> their parent, they must NULLify their pointers in the parent") this is
> not an issue.
>

But then how would the parent know if it has to invoke the destructor
on a child or not if the
child has already been gc'ed? If they don't know they'll end up
dereferencing a NULL pointer and that-is-bad(c).
Imagine this scenario: a layer is added to a map. After a while they
are gc'ed in this order: the layer first and then the map. If the
layer does not set its reference to NULL in the parent the map will
segfault when it attempts to free the layer.

>
> 3.6 Always give object ownership to SWIG
> -------------------------------------------------------------
> See above (%typemap(csdestruct)).  Every %newobject declaration might
> remain the same since it will not affect the operation.
> The %typemap(csconstruct) will be removed but it will not affect the
> expected operation though.

We have to see which costs more to implement.

Best regards,
Umberto