RFC-24 Mapscript memory management: call for comments

[Umberto Nicoletti]

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
>