RFC-24 Mapscript memory management: call for comments

[Tamas Szekeres]

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.

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

>
> 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,