RFC-24: published implementation proposal on bugzilla

[Umberto Nicoletti]

On 3/15/07, Tamas Szekeres <szekerest at gmail.com> wrote:
> Umberto,
>
> Currently I cannot see the exact match between the actual
> implementation and the proposal described in rfc24.
> Additionally I feel that this implementation is incomplete as it stands now.
>

It surely is. As I have said earlier I wanted to have some code we can
talk over instead of an having only an rfc. We all know how difficult
it is to express an implementation in terms of natural laguange. It
would be a waste of time for me to write a complete set of code and
maybe get the rfc refused, so I chose to implement *some* of the most
relevant parts of the rfc.

> Here are some raisings about the changes (without ranking by the importance)
>
> 1. The changes affecting the target language code is mostly missing.
> In one hand you can force the ownership at the wrapper using the
> %newobject feature like:
>
> %newobject getLayer;
>   layerObj *getLayer(int i) {
>     if(i >= 0 && i < self->numlayers) {
>         MS_REFCNT_INCR(self->layers[i]);
>         return (self->layers[i]); /* returns an EXISTING layer */
>     } else {
>       return NULL;
>     }
>   }
>
> but this philosophy is not reflected by some other functions returning
> objects as:
>
> layerObj *getLayerByName(char *name) {
>     int i;
>
>     i = msGetLayerIndex(self, name);
>
>     if(i != -1)
>       return (self->layers[i]); /* returns an EXISTING layer */
>     else
>       return NULL;
>   }
>
> From this standpoint I cannot really interpret the comments described
> in "3.6 Always give object ownership to SWIG"

I have implemented 3.6 only for the methods explicitly mentioned in
the unit tests. The rest will come if the rfc is approved.

>
> 2. You've suggested to keep references to the parent objects at the
> target language. How would you implement this?

This is described in the rfc at 3.7 for Java and 3.8 for Perl, but as
of now I am not sure if we really need it, that's why I have
implemented the null parent check for the layer obj.
We might choose to delay the application of this requirement to a second phase.

> How many code changes would be needed? Would this implementation be
> mapscript specific, or you could propose some type independent
> solution like using a number of "typemap() SWIGTYPE" for example?
> I will attach an UML diagram to describe the current parent-child
> relationships. We can see that some of the objects (like rectObj)
> belongs to various class types.
> I think it would be fairly complicated to implement these changes for
> the mapscript specific classes one by one. Would these changes
> eliminate the need of using refcounts at the C side?

No, they won't because the cache simply keeps the information about
the parent, not about the children. To prevent the early garbage
collection of the children we need to use the refcount.

> I will also attach a C# specific typeless implementation for the same
> problem (taken out of the current gdal C# project)

I will look into it.

>
> 3. How the original concept of the object ownership is altered by this
> RFC. Who is responsible to create and finally remove a particular
> object? The C refcount implementation impresses that those objects are
> self owned (controlled by the refcounts), but the comments at 3.2
> shows that the original concept should be retained (So that finally
> the child should be controlled by the parent).

I think that the best and maybe only way for me to explain was to
actually write the code. Please have a look at the freeLayer, freeMap
and the unit tests and come back to me if you have further issues to
discuss.

>
> 4. I consider the decrementation of the refcount cannot be used solely
> without controlling the destruction of that item like in layerObj
> *msRemoveLayer(mapObj *map, int nIndex). What to do if the refcount
> reaches zero in this case?

msRemoveLayer will never decrement the refcount to zero but will leave
it always to at least one (depending if the layer was dinamically
added to the map).
Example:

I remove the first layer from the tests/test.map in mapserver distribution:
initially the refcount of the layer is 1 because the map has one
reference. After removal a new layerObj is created in mapscript so the
refcount is still one. If the layer is added to another map the
refcount of course increases, else if the layerObj goes out of scope
and is gc'ed the memory will be freed because the refcount drops to
zero.

>
> 5. Not only the layerObj and classObj collection arrays should be
> handled. This proposal should deal with the replacement of the other
> collection arrays like styleObj and symbolObj arrays respectively.
>

That's planned for the future, I only have 24hours a day and like to
sleep for the most part of it! If you vote for the rfc I will
implement them in a reasonable time.

Ciao,
Umberto

>
> Best regards,
>
> Tamas
>
>
> 2007/3/13, Umberto Nicoletti <umberto.nicoletti at gmail.com>:
> > I've put some more effort in preparing a demo of how rfc-24 would be
> > implemented.
> >
> > I've attached a new patch to
> >
> >  http://mapserver.gis.umn.edu/bugs/show_bug.cgi?id=2032
> >
> > that implements:
> >
> > 1. classes as array of pointers
> > 2. dynamic classes
> > 3. reference counting for most classes operations (see the python test
> > case for what's been done)
> > 4. more python unit tests to cover the new class code
> > 5. fixed some memory leaks (some introduced by the previous patches,
> > some already there)
> >
> > Apply the new patch 636 after 632 and 633 to mapserver head (inside
> > the mapserver directory):
> >
> > patch -l -p0 < 632.patch
> > patch -l -p0 < 633.patch
> > patch -l -p1 < this.patch
> >
> > Note the -p1 switch on the last patch command. Last patch will
> > complain about maplayer.c, please  answer as follows:
> > assume reverse: n
> > apply anyway: y
> >
> > You can safely ignore errors for now as they're only comments or indentation.
> > There's a lot of activity going on on cvs in these days so it could be
> > that in the next the few days it will be more difficult to apply the
> > patches.
> >
> > At this point I think there is enough code to look at to plan the
> > opening of a voting thread on rfc-24 at the end of next week (around
> > March 23). If your schedule is too busy I can consider delaying the
> > voting session of course.
> >
> > Best regards,
> > Umberto
> >
> >
> > On 3/4/07, Umberto Nicoletti <umberto.nicoletti at gmail.com> wrote:
> > > I have been working on an example implementation of rfc-24 and the
> > > fruit of my labor is attachment #632 to:
> > > http://mapserver.gis.umn.edu/bugs/show_bug.cgi?id=2032
> > >
> > > Direct link:
> > > http://mapserver.gis.umn.edu/bugs/attachment.cgi?id=632&action=view
> > >
> > > Apply this patch to cvs HEAD to see how rfc24 would be implemented.
> > > The patch only modifies the map and layer objects, the rest is still unmodified.
> > >
> > > Key points:
> > > 1. refactor map->layers to be an array of pointers instead of a array of structs
> > > 2. allocation of a new layerObj only happens when needed, thus conserving memory
> > > (allowing for large values of MAX_NUM_LAYERS with minimal waste of memory)
> > > 3. wrap map->layers access with GET_LAYER macros to ease future developments
> > > (i.e. removal of MAX_NUM_LAYERS)
> > > 4. check that layer->map is not null before using it and raise a new
> > > error if it is (MS_NULLPARENTERR)
> > > 5. reference counter for mapObj and layerObj
> > > 6. modification of mapserver core and swig interface files to do reference
> > > counting for map and layers (incr and decr)
> > > 7. created unit tests for python (mapscript/python/tests/cases/refcount.py) to
> > > test the functionality of the previous items. This has not been added to
> > > runtests.py yet
> > >
> > > Due to the large extent of the changes I have also made available
> > > items 1,2,3 and item 4 available as two separate patches.
> > >
> > > Happy hacking and tell me what you think,
> > > Umberto
> > >
> >
>
>