RFC-24 Mapscript memory management: call for comments

[Umberto Nicoletti]

On 1/13/07, Howard Butler <hobu at iastate.edu> 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

Frank,
I think it's also written in the RFC that the API (and functionality)
will be backwards compatible,  and it's my top priority not to disrupt
it. I also hate to rewrite (working) code ;-)

On the other hand we changed the API twice recently and we are going
towards a major release upgrade, so it seems reasonable to me to
exchange some functionality for 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.
> >

I agree with Howard here: we should do our best to simplify the life
of mapscript users: we simply cannot expect them to understand and
handle the implications of managing memory in a mixed environment like
Java/cSharp mapscript and native code.

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

2.3 is in fact really subtle and, as I wrote in the rfc, it was the
eagle-eyed Tamas who spotted the issue first. I've had a very
claryfying private chat on this issue with him some time ago, I will
digg it out and post it here because it makes very simple to
understand it IMO.

I agree with all of you that this rfc is quite a massive task both to
understand and implement, it has took me almost a month to iron out
most of the code that is there.

I'll take some more time to think about your comments and write tomorrow.

Regards,
Umberto

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