RFC-24 Mapscript memory management: call for comments

[Umberto Nicoletti]

On 1/19/07, Steve Lime <Steve.Lime at dnr.state.mn.us> wrote:
> Seems like most of the issues are related to not knowing who (e.g. Swig
> or a mapObj)
> owns an object. It seems that if we removed ambiguity there then that
> would really
> help and you wouldn't have to touch the Swig stuff much.
>
> That is:
>
>   - use dynamically allocated arrays of layers, classes and styles
>

That is an array of pointers, not an array like it is now.
This is certainly recommended for the purpose of this rfc. I have
found a way for Java mapscript to work around it for now, but it is a
bit clumsy as it involves changing the swigCPtr in the inserted class
inside the insert method.
Example for Java:

	%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;
>>>>>>>		classobj.setCPtr(mapscriptJNI.layerObj_getClass(swigCPtr,
actualIndex));
		return actualIndex;
	}

The setCPtr method has to be injected as swig won't create it by default.
With this strategy the msInsertClass stays almost untouched (only one
extra freeClass to prevent memory leak and free the insertClass
memory).

>   - then change insert functions to take ownership of objects to be
> inserted (I assume
> this is much easier with a list of pointers) (the insert sets parent
> pointer)
>

the inserts set swigCMemOwn to false which means that from now the
object is not responsible anymore for cleaning up as that is delegated
to mapObj. With a list of pointers it is not necessary to copy the
original object.

>   - with that change wouldn't the deconstructors work "as is"? That is,
> ok to free
> if parent is NULL?

No, because as I have shown in my previous post if the parent object
was GC'ed earlier as soon as the child dereferences it to check if it
is NULL mapscript would crash. To avoid this the swigCMemOwn must be
properly set at all times.
I.E.:
insert sets swigCMemOwn=false
remove sets swigCMemOwn=true
new with a null parent sets swigCMemOwn=true


>
>   - change constructors not to take a parent refernce, only use the
> insert methods
> to establish that relationship
>

This is not a big deal, I think it can stay like it is now. Once the
insert method works we can simply transport the logic into the
constructor method by, for instance, calling the insert into the
constructor.

>   - hide all parent references from scripting interface
> ($layer->{map}->draw() should
> not be allowed)
>

Please note that this would still not solve the problem of early
garbage collection.The ONLY way, as far as I know, to solve this
problem is to keep Java/C#/Perl/etc references into the mapscript
objects so that the hosting language can infer what is going on and
what is reachable and what is not.

Umberto

> My main worry is maintenance of SWIG typemaps. There just aren't that
> many
> folks that can (or are willing to) operate at that level. That makes
> this area prime
> for divergence amongst the languages. Solutions at the C level are more
> palatable.
>
> Steve
>
> >>> Umberto Nicoletti <umberto.nicoletti at GMAIL.COM> 1/17/2007 1:51:46
> AM >>>
> On 1/17/07, Steve Lime <Steve.Lime at dnr.state.mn.us> wrote:
> > Sorry all, was on vacation. I'm not sure where to weigh in. I'd
> > certainly prefer fixing
> > the underlying C code if at all possible instead of trying to
> maintain a
> > bevy of
> > language specific SWIG typemaps. I personally don't see a problem
> > depricating
> > the constructors with a major release. I'm not sure how much they
> are
> > even
> > used by folks.
>
> Well, they are at least used by me ;-)
>
> But, as I said to Frank earlier I would certainly exchange backwards
> compatility or functionality if I could. Also the major release number
> change could be used to justify the change.
> Anyway, the proposal in rfc-24 should (I have not tested all cases)
> retain backwards compatibility.
>
> >
> > Does anyone have a good idea of what changes to the underlying
> C-structs
> > would
> > be particulary helpful? There is an RFC out there to ditch arrays of
> > structs for arrays
> > of pointers and I thought Daniel might have had a contract to
> implement.
> >
>
> I'm afraid that we can't keep all the changes into the swig wrappers,
> but there needs to be some customization also at the language level
> and that must be, of course, customized.
> I think the swig ruby documentation offers a quite detailed overview
> on the matter of memory management and swig wrappers.
>
> http://www.swig.org/Doc1.3/Ruby.html#Ruby_nn57
>
> Umberto
>
> > Steve
> >
> > >>> Sean Gillies <sgillies at FRII.COM> 01/16/07 11:25 AM >>>
> > Howard Butler wrote:
> > > On Jan 13, 2007, at 1:26 AM, Frank Warmerdam wrote:
> > >
> > >> Tamas Szekeres 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.
> > >> Umberto,
> > >>
> > >> I'm concerned about the substantial disruption to backwards
> > >> compatibility
> > >> that seems to be implicit in this proposal.  I would appreciate
> it
> > >> if the
> > >> RFC could have a section discussing the compatibility problems
> that
> > >> will
> > >> be caused by the proposed changes.
> > >>
> > >> On the one hand, I appreciate the effort you have put into this
> > >> RFC, and
> > >> your willingness to take on a thorny problem.   On the other hand,
> my
> > >> understanding of mapscript use and the different language bindings
> is
> > >> sufficiently weak that I don't really see the problem(s) being
> > solved.
> > >>
> > >> /me re-reads, and really tries to grok the RFC...
> > >>
> > >> 2.1 Object equality/identity
> > >>
> > >> OK, I agree with Tamas on this. I think it is unfortunate that
> the
> > >> insertLayer() is really just copying the original layer rather
> than
> > >> literally inserting that instance, but I think this is just
> something
> > >> we can document and it is easily worked around.  Generally
> speaking
> > >> I think we should be discouraging creating "free standing"
> > layerObj's.
> > >>
> > >> 2.2 Early garbage collection
> > >>
> > >> Tamas writes:
> > >>> That's really an issue, and a reference to the real memory owner
> > >>> object should be kept by the child to avoid the crash.
> > >> My opinion is different.  The map is the main object, and I think
> it
> > >> is up to applications to keep a handle to the map as long as they
> > want
> > >> to do stuff, and once the map is released (dereferenced) the
> > >> application  should not use any part of the map.
> > >>
> > >> 2.3 Dynamically populated layers, classes, etc
> > >>
> > >> I'm afraid I'm not clear on what the issue is here.  I tried
> digging
> > >> through the bug reports, but this did not turn out to be an easy
> way
> > >> to understand the problem.  I'd encourage you to explain the
> problem,
> > >> and perhaps refer to the bug reports as supporting information.
> > >>
> > >> Tamas's suggestion seems to make sense to me, but only in a rough
> > >> sort of way since I was unable know the details.  Certainly I
> > >> can understand that we need a way in a constructor to distinguish
> > >> between returning a reference to a pre-existing object as opposed
> > >> to a brand new one "owned" by the reference.
> > >>
> > >> I'm afraid I got quite lost in the proposed implementation.  It
> seems
> > >> the changes are quite dramatic and I too worry about whether it
> would
> > >> ever work properly and whether the changes will be so dramatic
> that
> > >> they break all existing mapscript applications.
> > >>
> > >> I wish I understood swig better.  The mapscript bindings, and the
> > >> "next generation" GDAL/OGR bindings seem so complicated that I am
> > >> quite lost.  This no doubt means I'm a bit dumb, but I'm sure I'm
> > >> not the only one getting overwhelmed by it.  If things get so
> > >> complicated that only very few developers can grok them then
> > >> getting maintenance done becomes increasingly problematic.
> > >>
> > >> I know that Howard is at least in a general sense enthusiastic
> > >> about RFC 24, but I certainly feel uncertain about it so far.
> > >> Perhaps some additional work can be done to clarify the RFC (ie.
> > >> a compatibility section), and to try and iron things out with
> Tamas
> > >> who clearly has a much better understanding than I do.
> > >>
> > >>
> > >
> > > Umberto,
> > >
> > > I also echo the kudos for writing such a detailed RFC.  Memory
> > > management in MapScript (as both a user and a developer) has
> always
> > > been rather treacherous, and your attempt to clear things up is
> > > admirable.  Frank is right as far as my enthusiasm for the
> *intent*
> > > of RFC 24, with respect to the specific implementation, I think
> > > putting all of this logic in the SWIG bindings and reimplementing
> it
> > > in n languages worth of customization is putting it in the wrong
> > place.
> > >
> > > MapServer CGI is in my opinion, one very big, powerful, and
> > > specialized MapScript application.  It has driven much of the
> feature
> >
> > > development of MapServer, and how it works has driven much of
> > > MapServer's design.  MapServer CGI has some luxuries that
> MapScript
> > > doesn't have, and not having to worry so much about object
> > > referencing and dynamic memory management is probably its
> greatest.
> > >
> > > There has long been a desire for a MapServer 'C' API to congeal
> the
> > > commonalities of both MapScript and MapServer CGI.  Right now, our
> > > 'C' API is essentially the stuff in the SWIG interface files, and
> if
> > > you're looking to program with MapServer in 'C', this is the best
> > > place to look for examples and ideas.  Two very big components of
> > > MapServer don't utilize this pseudo-C API, however -- MapServer
> CGI
> > > and PHP/MapScript.  They currently exist in their own little
> worlds,
> > > and all three components (SWIG MapScript, PHP/MapScript, and
> > > MapServer CGI) reimplement pieces of functionality that might live
> in
> >
> > > a common location.
> > >
> > > I agree and am generally enthusiastic about the *intent* of RFC
> 24,
> > > but with respect to the specific implementation, I think
> implementing
> >
> > > it as customizations for each MapScript language (and depending on
> > > SWIG's uneven support for customizing each MapScript language) is
> the
> >
> > > wrong place to put it.  This approach will create a lot of work
> for
> > > each MapScript maintainer and, in my opinion, lead to even more
> > > uneven MapScript implementations.  It won't do anything for PHP/
> > > MapScript (which also has the same problem).
> > >
> > > I think that object lifetimes should be MapServer's duty to manage
> --
> >
> > > not the SWIG layer's and most definitely not the user's code
> > > (contrary to Frank's opinion).  The whole point of garbage
> collection
> >
> > > is to abstract away object memory management, and implementations
> > > that do it half-way, like GDAL's language bindings and SWIG
> > > MapScript, are in some ways worse than having no object management
> at
> >
> > > all.  Most users of our MapScript bindings don't come to the party
> > > with backgrounds in worrying about who "owns" anything, and if
> they
> > > are looking for that level of granularity, performance, and
> control,
> > > they shouldn't be looking at writing their stuff in MapScript in
> the
> > > first place.  Ease should be the focus, and the definition of ease
> is
> >
> > > not having to worry about reassigning a mapObj to a new variable
> and
> > > things blowing up when you do so.
> > >
> > > As it stands now, I am +1 on the intent of RFC 24 but -1 on where
> it
> > > is being implemented.  I think this is MapServer 'C' API stuff, and
> I
> >
> > > am very concerned that implementing it all in the SWIG bindings
> will
> > > lead to even more uneven MapScript implementations.  Not every
> > > language supports the %feature("*code") feature, and even if it
> did,
> > > we would still be repeating ourselves across all of the languages.
> > > If we've given MapServer enough information to know about its
> > > objects, MapServer should take care of doing the right thing with
> > > them.  Another strike against implementing it where RFC 24
> currently
> > > proposes is that it does nothing for PHP/MapScript or MapServer
> CGI
> > > (hey, that's becoming more dynamic every day ;).
> > >
> > > Howard
> > >
> >
> > Happy New Year, all.
> >
> > Ideally, the implementation should be at the lowest level to provide
> the
> >
> > most benefit for all, but it seems like doing it in the SWIG layer
> for
> > only the languages whose users give a damn is the low-hanging fruit.
> > MapServer has always been about the low-hanging fruit.
> >
> >
> >
> > The root of the problem, of course, is the parent references that
> were
> > added to layer and class for convenience in msLayerDraw and other
> > places, and particularly the form of the child object constructors:
> >
> >    child = new childObj(parent)
> >
> > When I added the insert* methods to mapscript, it was my hope that
> we
> > would one day be able to deprecate the old constructors, make the
> parent
> >
> > references immutable, and then leave behind this particular issue of
> > memory management. For example, the only usage would be:
> >
> >    parent.insertChild(new childObj)
> >
> > The consequences of nullifying this parent should be crystal clear
> to
> > any user.
> >
> > (The reason why insertChild currently inserts a *copy* into the
> parent
> > is because MapServer's mapObj->layer is an array of structs instead
> of
> > an array of pointers. Same situation for layerObj->classes. At the
> time,
> >
> > I was optimistic that the project would one day be rewritten to
> > implement dynamic arrays of child pointers, and so it seemed to me
> that
> > the insertChild methods would be totally foolproof in the future,
> and
> > merely require extra memory in the present.)
> >
> > If you want to keep expanding MapScript, you have to either
> deprecate
> > the old constructors, or add a bunch of new code to guard the
> integrity
> > of parent/child references. I take it that the first option has been
> > ruled out?
> >
> > Having said all that, I have to say that lately I am actively
> > discouraging people from using all of MapScript so that they do not
> trip
> >
> > over its memory management (and other) issues. I don't recommend any
> > mapscripting other than to create mapObjs from a file, and making
> image
> > responses to custom web requests like
> >
> >    # parse http request
> >    ...
> >    # write a one-off mapfile
> >    ...
> >    mapper = mapObj(file)
> >    image = mapper.draw()
> >    start_response('200 OK', [('content-type',
> image.format.mimetype)])
> >    return [image.getBytes()]
> >
> >
> > Cheers,
> > Sean
> >
> > --
> > Sean Gillies
> > http://zcologia.com/news
> >
>