[RFC-24] Updated public draft

[Umberto Nicoletti]

On 2/25/07, Tamas Szekeres <szekerest at gmail.com> wrote:
> 2007/2/23, Umberto Nicoletti <umberto.nicoletti at gmail.com>:
> > 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)
>
> Umberto,
>
> I would prefer the the refcount member instead of using a global
> repository since the code should perform well in a multithreaded
>
> environment. We should also avoid the extra cost of the various hash
> table lookups. However - for debug purposes - it would be
>
> considerable to establish that table so as to detect the potential
> memory allocation problems (like the leaks for example).
>

I also have considered the threading issues of the hast table and in
the end it could be a suboptimal implementation with regards of the
many locking operations that it requires. But it would be easier, at
least for a start (I'm getting tired of all this discussion above
pseudo-code and would like to write some code instead).

> > >
> > > 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.
> >
>
> I mean that the garbage collector operates on a different thread as
> the normal execution. I guess the java runtime implements a
>
> similar approach as the .NET runtime in this case. Therefore the
> --refcount will possibly be executed on different thread as the
>
> ++refcount.

Java uses a so called stop-the-world approach, so while GC is running
all other threads are stopped and there is no need to synchronize.
We can implement the locking for C# (and other mapscripts which need
it) with the appropriate ifdefs.

> (It depends on the compiler how the increment and decrement operations
> are actually implemented. In some cases the the actual
>
> refcount value will be fetched into a register, the addition and the
> substraction will happen in that register and later the value is
>
> written back to the memory. In the meantime the other thread might
> change the value in the memory but those changes will
>
> eventually be lost. In addition - for example - when a DWORD value is
> not aligned to 4 byte boundaries on a 32 bit system, either
>
> the writing of the value cannot be considered as an atomic operation.
> While one part of that value is written the other thread might
>
> read that partially written value back.)
>
> > >
> > > 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*.
> >
>
> Well, I haven't looked over the code in this regard and needless to
> say I'm *totally disappointed*. Using a pointer without checking
>
> it's value is not acceptable in most cases. Now then it's not so
> difficult to write some test cases that will definitely fail with
>
> unhandled exception, like:
>
>      layerObj layer = new layerObj(null);
>      layer.connection = "data.shp";
>      layer.connectiontype = MS_CONNECTION_TYPE.MS_SHAPEFILE;
>      layer.open();
>
>
> DAMN !
>

We could find all these uncheked pointer derefences and correct them
in the C code directly. This would have the benefit of improving the
overall safety of the C code and (maybe) ease the implementation of
this rfc.
Let me check upon this.

Umberto