[RFC-24] Updated public draft

[Tamas Szekeres]

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:
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.

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.

#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.


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.
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).

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);
  }

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).

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.


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.


I hope I haven't missed something out. However I will leave the rest
to the other folks. Come on guys!


Best regards,


Tamas




2007/2/22, Umberto Nicoletti <umberto.nicoletti at gmail.com>:
> Dear devs,
> I have eventually updated rfc-24 to include the feedback collected
> from the previous post.
> In particular the following areas have changed:
>
> 1) the cache has been dropped because it requires a different
> implementation for every supported mapscript. The reference counter
> strategy has been adopted instead as proposed by Tamas. The various
> mapscript objects will anyway need to be modified to keep a reference
> to their parents to prevent early garbage collection and then
> unexpected segfaults in the hosting language. This cannot be solved
> with reference counting alone.
>
> 2) the various arrays of structures (layers, classe and styles) will
> need to be changed to arrays of pointers to ease the development of
> the new insert methods. This is also useful if int the future we plan
> to remove the limitation of having a compile time fixed number of
> layers, classes, styles, symbols, ecc
>
> 3) There is a tracker bug on bugzilla
> (http://mapserver.gis.umn.edu/bugs/show_bug.cgi?id=2032) which has an
> attachment that shows how I've implemented the previous item for the
> layers array. This tracker will be used to post further comments and
> code snippets
>
> Sadly it seems that Ruby will still be left out of the game because of
> how swig builds the extension code and will need a customized
> implementation. Tcl needs some support from somebody that actually
> knows it and moreover cares about supporting it.
>
> The proposed plan is that, after this second discussion, I will
> incorporate any further changes and then call for a vote on
> mapserver-dev. If the rfc is adopted I will implement the changes at
> the C level and in the swig interface files.
> I will also take care of specific changes for Java and maybe Perl
> mapscript. Someone else should volunteer to 'upgrade' the other
> mapscripts.
> I'll be doing this in my spare time and probably without funding, so I
> cannot make promises about a time schedule.
>
> I await your comments...
>
> Umberto
>