RFC-24 Mapscript memory management: call for comments

[Steve Lime]

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

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

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

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

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

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 
>