RFC-24 Mapscript memory management: call for comments

[Umberto Nicoletti]

On 1/16/07, Tamas Szekeres <szekerest at gmail.com> wrote:
> 2007/1/15, Umberto Nicoletti <umberto.nicoletti at gmail.com>:
>
> > > 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.e
> >
>
> I think keeping the arrays synchronized would require the largest
> amount of coding compared to the other 2 problems, and would make the
> code difficult to follow.
> IMHO the seriousness of this issue does not really justify the need of
> this significant change considering that quite some people are
> affected as the maintainers of the code to be modified.
>

It is a big change indeed, but it might help when handling your
example of 'explicit finalization by an overzelous user'.
I need some time (a couple of days) to code an example for this.

> >
> > 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.
> >
>
> I have no problem with the second case, and presumably it's easier to implement
> since many of the C structs also have back-references to the parents. So when
> every child object would be responsible to keep alive the parent until
> the child exists, the
> root object (the real memory owner) would also be prevented from the
> unexpected garbage collection.
>
> But I'm a bit uncertain how many code is affected by this change. May
> be it would be
> helpful to enumerate the functions to be modified in regard to this
> particular issue.
> Perhaps defining the corresponding typemaps generally, and applying
> them to the classes
> would help to keep the code quite clear.
>
> I would confirm that 2.3 requires to modify only the constructors of 3 classes.
>
> It must be noted that the explicit finalization of the parent/owner
> objects are still not handled by the proposed changes at 2.2. For
> example when the overzealous user would want to explicitly call the
> Dispose method (i think Delete is the java equivalent) of the owner,
> subsequent accesses to the childs would also cause access violation.
> This is that the area where a well constructed API at the C side would
> help, by applying some kind of reference counting so as to prevent the
> structs to be freed until there are references on it. For example
> calling the finalizer of the mapObj would delete only the wrapper
> class, but the underlying C memory would be retained until there are
> references on it. Certainly, the current SWIG approach should also be
> altered as the 'disowned' objects should also notify the API whether a
> reference was released.
> However at this time I cannot estimate the required amount of work
> needed to realize such an implementation.
>

See my comment above.

Umberto

> >
> > 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.
> >
>
> Ok, but take care of the fact, that currently the class array of the layer is a
> struct array, not a pointer array, therefore you cannot do simply a pointer
> assingment when adding the class to the layer. You might have to use
> a struct copy instead and free the original structure respectively.
> But how would you
> alter the swigCPtr of the original classObj accordingly?
>
> Best Regards,
>