RFC-24 Mapscript memory management: call for comments

[Umberto Nicoletti]

Sorry for the late reply, but lately I've been more busy than usual :-(

On 1/11/07, Tamas Szekeres <szekerest at gmail.com> wrote:
> Umberto,
>
> My personal attitude to this RFC is slightly ambivalent. On one hand I
> appreciate your efforts to discover the similarities and the
> differences between the various languages
> regarding to a particular problem and it's also appealing that one of
> our ancient problem had been tempting the mapscript users for a long
> ago was included in the proposal.
> But on the other hand one of the problems enumerated is not considered
> to be a problem (by me), however a large amount of code should be
> altered so as to "fight against it", and even I doubt that this
> solution is working correctly as it stands.
>
> Referring to the problems described in the RFC:
>
> 2.1 Object equality/identity
>
> I consider this issue as a behaviour not a problem and I don't even
> know about any bug related to it. I think when the user would use the
> object after inserting to the parent, she can obtain the reference to
> it with some additional code like:
>
> mapObj map = new mapObj("mymap");
> layerObj layer = new layerObj(null);
> int index = map.insertLayer(layer, -1);
> layer = map.getLayer(index);
>
> or alternatively one should use the constructor with the parent object:
>
> mapObj map = new mapObj("mymap");
> layerObj layer = new layerObj(map);
>

this item is more related to code correctness and quality than to a
solving a present issue of mapscript. The mindset I use when I think
about it is that if I was a Java developer picking up mapscript for
the first time I would expect object identity to return true between a
layer added to a map and the same layer fetched from the map.
Also consider that this is a collateral benefit if we'll implement the
cache as described in the following item 2.2.

> 2.2 Early garbage collection
>
> That's really an issue, and a reference to the real memory owner
> object should be kept by the child to avoid the crash.
>
> 2.3 (Constructors with parent object references)
>
> That's also a serious problem and should be handled as described in
> Bug 1743 (by altering the constructors of layerObj, classObj,
> styleObj)
>

We both agree here.

>
> According to the comments above I would favour dealing only with 2.2 and 2.3 by
>
> a, altering the 3 constructors (layerObj, classObj, styleObj)
> b, keeping reference to the real memory owner objects at every object.
>
> But I would highly discourage to retain internal references to the
> child objects at the parents, because of the following reasons:
>
> 1, It requires large amount of code changes should be implemented
> simultaneously by the various bindings that is a PITA.

When you say 'keep a reference to the real memory owner' do you mean
keep a reference in the Java or csharp class to the mapObj since it is
the mapObj which is ultimately responsible for memory deallocation?
So, if I understand correctly your reasoning, we both imply that the
wrapper objects need to be modified to keep references and prevent
early garbage collection. In my case I suggest to keep a reference to
the parent and then up to the hierarchy to the mapObj, you suggest to
keep a reference to the mapObj directly.
In both cases *each* mapscript language must be modified separately
and differently, which we should do anyway to implement 2.3.

> 2, No particular problems related to these changes since the child
> objects are "disowned" by default and might be garbage collected
> safely.
> 3, The RFC does not mention how msInsetLayer will be altered when the
> following code is applied:
>
> %typemap(javaout) int insertClass {
>         // call the C API, which needs to be modified
>         // so that the classObj is not copied anymore
>         int actualIndex=$jnicall;
>         classes[actualIndex]=classobj;
>         // disown the classObj just inserted, item 3.3
>         classobj.swigCMemOwn=false;
>         return actualIndex;
> }
>
> To avoid using msCopyLayer I suppose a C struct copy to be done, so
> 'classobj' will hold different C struct as mapObj. By disowning
> 'classobj' a memory leak will possibly arise.
>

I will post a modified version of msInsertClass with some more details
on the actual implementation to clarify this. There will not be any
memory leak, because this is what will happen when insertClass is
called:

1. in msInsertClass the classObj C pointer is assigned to the next
free classes array member (no msCopyClass involved, like it is now,
just a pointer assignement)
2. classObj is disowned as memory is now handled by mapObj
3. cache is populated accordingly

Bonus: one free and one malloc of a classObj less than the actual
implementation and the classObj reference is still valid in the
current context.

As I said I'll post a modified msInsertClass so that we can have some
actual code to look at.

Umberto

> Getting back to the solution of b, (mentioned above) IMO this issue
> might be considered as a bug in the SWIG implementation rather than in
> the mapscript interface, so altering the SWIG default behaviour would
> be expected instead of an application specific modification.
> I had to deal with this issue at the GDAL C# bindings before, (because
> some of the samples were definitely fail with this problem) and
> altering how SWIG handles the ownership of the memory was a reasonable
> option to solve this problem.
> To safely handle this issue I've maintained the reference of the real
> memory owner instead of the boolean swigCMemOwn parameter inside every
> object. This reference (swigCMemOwner) is set to null if the memory
> owned by the same object and should be disposed accordingly.
> When swigCMemOwner is not null the underlying C memory is not to be
> freed at all.
> The related changes can be found in the
> gdal/swig/include/csharp/swig_csharp_extensions.i file of the GDAL
> project tree.
>
>
> Best Regards,
>
>
>
> 2007/1/10, Umberto Nicoletti <umberto.nicoletti at gmail.com>:
> > Developers,
> > I have spent by now quite some time on the writing of RFC-24 and I
> > need to hear your opinions before I take it further with more
> > examples, more details, code or anything else is needed.
> > I am therefore opening RFC 24 for comments.
> > When I have enough feedback I'll make the needed changes and then
> > submit it for voting.
> >
> > RFC-24 is available at the usual location:
> > http://mapserver.gis.umn.edu/development/rfc/ms-rfc-24
> >
> > Regards,
> > Umberto
> >
>