Re: [geos-devel] An Immodest Proposal

[Martin Davis]

The geomFactory is static for the lifetime of GEOS, right?  So why is it 
freed at all between queries - shouldn't it live until the process 
containing GEOS ends (at which point the entire process image 
disappears, so it doesn't matter if it's never freed).


If the problem is that you are trying to delete this statically 
allocated object, then you need to find a way of *not* deleting that 
object.  Perhaps check the value of the pointer you are about to delete 
against this object and don't delete it if is the same?


Paul Ramsey wrote:

So, moving the instantiation of geomFactory object into initGEOS did
nothing for me, the object is still allocated by malloc, not palloc,
perhaps because it's a global, so it's getting set up at library load
time?

Putting the whole factory and creation process inside the function
bodies could get very expensive, since the functions are called
repetitively a lot, which means each call would do a newfactory,
newgeo, destroygeom, destroyfactory (ack!).  This is presumably why
the factory was made into a global outside the functions.

I'm at an impasse. If new/delete are overridden, when the end of a
postgres comes along, the system will attempt to pfree the globals as
it cleans up the library, and since the globals are malloc'ed,
segfault ensues.

If we backtrack to not over-riding new/delete, then we have the
problem of leaking the prepared objects at the end of each
transaction.

P.

On Wed, Oct 1, 2008 at 11:08 AM, Paul Ramsey [EMAIL PROTECTED] wrote:
  

So, using a longer lived memory context I can get right to the end of
a query before the segfault arrives. The segfault happens when
deleting a geometryfactory at the end, which tells me a couple things,
I think.

First, those two std allocs that sneak in before we over-ride new are
related to the geometry factory. The segfault happens when we're
trying to pfree the geometryfactory.

The offending code is this in geos_c.cpp:

// NOTE: SRID will have to be changed after geometry creation
static const GeometryFactory *geomFactory =
   GeometryFactory::getDefaultInstance();

This global geomFactory is called a number of times by WKB readers and
some other functions.  I wonder if we can initialize it in initGEOS
and clean it up in finishGEOS?

It appears, that this is what used to happen? Here's the current code.

void
finishGEOS ()
{
   // Nothing to do
   //delete geomFactory;
}

Any help on the history behind this global?

P.



___
geos-devel mailing list
geos-devel@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/geos-devel

  


--
Martin Davis
Senior Technical Architect
Refractions Research, Inc.
(250) 383-3022

___
geos-devel mailing list
geos-devel@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/geos-devel

Re: [geos-devel] An Immodest Proposal

[Martin Davis]

So Chris is saying almost the same thing I am - except I don't think you 
ever have to free the static factory object.


Chris Hodgson wrote:
If we're willing to accept a bit of a hack, you could just check in 
your overridden free() if the thing being free()'d was the global 
geomFactory, and if so, always use regular free().


This would have no effect on programs that aren't specifying their own 
memory functions (other than the additional instructions to execute on 
every call to free() ), and I think it would solve the problem as 
you've stated it.


Chris

Paul Ramsey wrote:

So, moving the instantiation of geomFactory object into initGEOS did
nothing for me, the object is still allocated by malloc, not palloc,
perhaps because it's a global, so it's getting set up at library load
time?

Putting the whole factory and creation process inside the function
bodies could get very expensive, since the functions are called
repetitively a lot, which means each call would do a newfactory,
newgeo, destroygeom, destroyfactory (ack!).  This is presumably why
the factory was made into a global outside the functions.

I'm at an impasse. If new/delete are overridden, when the end of a
postgres comes along, the system will attempt to pfree the globals as
it cleans up the library, and since the globals are malloc'ed,
segfault ensues.

If we backtrack to not over-riding new/delete, then we have the
problem of leaking the prepared objects at the end of each
transaction.

P.

On Wed, Oct 1, 2008 at 11:08 AM, Paul Ramsey 
[EMAIL PROTECTED] wrote:
 

So, using a longer lived memory context I can get right to the end of
a query before the segfault arrives. The segfault happens when
deleting a geometryfactory at the end, which tells me a couple things,
I think.

First, those two std allocs that sneak in before we over-ride new are
related to the geometry factory. The segfault happens when we're
trying to pfree the geometryfactory.

The offending code is this in geos_c.cpp:

// NOTE: SRID will have to be changed after geometry creation
static const GeometryFactory *geomFactory =
   GeometryFactory::getDefaultInstance();

This global geomFactory is called a number of times by WKB readers and
some other functions.  I wonder if we can initialize it in initGEOS
and clean it up in finishGEOS?

It appears, that this is what used to happen? Here's the current code.

void
finishGEOS ()
{
   // Nothing to do
   //delete geomFactory;
}

Any help on the history behind this global?

P.



___
geos-devel mailing list
geos-devel@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/geos-devel
  


___
geos-devel mailing list
geos-devel@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/geos-devel



--
Martin Davis
Senior Technical Architect
Refractions Research, Inc.
(250) 383-3022

___
geos-devel mailing list
geos-devel@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/geos-devel

Re: [geos-devel] An Immodest Proposal

[Greg Troxel]

  We have recently found one such problem pattern. When using
  preparedgeometry in postgis, we create a GEOS geometry and associated
  prepared geometry, these are duly malloc'ed, but we store references
  to them in a palloc'ed struct in a memory pool which lasts for the
  life of a postgres query.  All is good (well, there are memory leaks
  in the prepared stuff still, but those are theoretically fixable)
  until the end of the query, when postgres cleans up the query memory
  pool. All of a sudden the struct with the references to the geometry
  and prepared geometry are gone -- but the objects have not been freed.

Your proposal sounds sensible to me.  An alternative, much less
palatable, is to head down the path that guile takes with SMOBS, where
each object has a free routine, and to have the cleanup function for the
pool deallocate external pointers.  This assumes the pool is cleaned
instead of just being freed en masse like an exiting process, and even
if it worked it seems more complex and harder.

One concern would be if geos objects created in this way end up shared
in multiple pools, or are ever referred to by anything outside of such a
pool.  I can certainly believe that the usage patterns in postgis mean
that my concern is at the moment unfounded, but the palloc strategy
forces an invariant onto postgis that should be clearly documented lest
someone later not understand the implications.

I am woefully unclear on threading and geos.  Probably the rule for the
init function is that one has to call it before doing any other
operation (to avoid alloc-native/free-palloc), and then one may not
change the function again, at least not while any objects are allocated.

What happens when the two 'std alloc!' allocations get freed with the
new free function at process exit?  (Or if they don't now, when the bug
that leaks them is fixed?)





pgpmR81kE1JGu.pgp
Description: PGP signature
___
geos-devel mailing list
geos-devel@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/geos-devel

Re: [geos-devel] An Immodest Proposal

[Chris Hodgson]

Very nice Paul.

Those two std allocs are a bit scary... I had a feeling we might have 
some of those, and they seem worse than I had initially thought. The 
problem is that either they are attempted to be freed, using the 
provided free function - and who knows what error that could bring - or 
they are not, and we're leaking. If they are only created once at load 
of the DLL and never actually freed, but simply go away when the DLL is 
unloaded, then I guess we're ok. But it seems likely to me that the C++ 
runtime would be trying to free them using your own provided free 
function... can we do a test where we provide pfree and see what it does 
with that? ie. if you try to pfree() a regularly malloc()'d segment of 
memory, does it actually free it without raising any errors?


It would be really nice to know what those things that are getting 
created earlier are, too...
but I guess it all just comes down to, does it work in postgres and not 
leak?


Chris

Paul Ramsey wrote:

It turns out it is a quite small modification, and works pretty well:

http://trac.osgeo.org/geos/attachment/ticket/208/memdiff.patch

My patch adds the ability to override, and also has some printfs to
show what is happening. When the overridden allocator is called
without the runtime callback set, it prints std alloc and when it's
called with the callback set, it prints geos alloc.

Here's a little program that exercises it:

#include stdio.h
#include /usr/local/include/geos_c.h
#include stdlib.h

int main() {
  printf(main\n);

  GEOSCoordSequence* cs;
  GEOSGeometry* g;
  int r;

  printf(initgeos\n);
  initGEOSMemory(malloc, free);
  printf(coordseqcreate\n);
  cs = GEOSCoordSeq_create(1, 2);
  printf(coordseqsetx\n);
  r = GEOSCoordSeq_setX(cs, 0, 1.0);
  printf(coordseqsety\n);
  r = GEOSCoordSeq_setY(cs, 0, 1.0);
  printf(createpoint\n);
  g = GEOSGeom_createPoint(cs);

  return 0;

}


And here is the output:

Heron:tmp pramsey$ ./a.out
std alloc!
std alloc!
main
initgeos
coordseqcreate
geos alloc!
geos alloc!
geos alloc!
coordseqsetx
coordseqsety
createpoint
geos alloc!

So, something in C++ is doing a couple allocations before we can slip
in and get our other function in place. But we can get all the big
stuff caught no problem.

P.
___
geos-devel mailing list
geos-devel@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/geos-devel
  


___
geos-devel mailing list
geos-devel@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/geos-devel

Re: [geos-devel] An Immodest Proposal

[Paul Ramsey]

Time to try this in real life with PostgreSQL and palloc/pfree.  I'm
going to create GEOS and PostGIS experimental branches, and commit my
work there, so that folks with other platforms can try it out.

On Tue, Sep 30, 2008 at 9:55 AM, Chris Hodgson [EMAIL PROTECTED] wrote:
 Very nice Paul.

 Those two std allocs are a bit scary... I had a feeling we might have some
 of those, and they seem worse than I had initially thought. The problem is
 that either they are attempted to be freed, using the provided free function
 - and who knows what error that could bring - or they are not, and we're
 leaking. If they are only created once at load of the DLL and never actually
 freed, but simply go away when the DLL is unloaded, then I guess we're ok.
 But it seems likely to me that the C++ runtime would be trying to free them
 using your own provided free function... can we do a test where we provide
 pfree and see what it does with that? ie. if you try to pfree() a regularly
 malloc()'d segment of memory, does it actually free it without raising any
 errors?

 It would be really nice to know what those things that are getting created
 earlier are, too...
 but I guess it all just comes down to, does it work in postgres and not
 leak?

 Chris

 Paul Ramsey wrote:

 It turns out it is a quite small modification, and works pretty well:

 http://trac.osgeo.org/geos/attachment/ticket/208/memdiff.patch

 My patch adds the ability to override, and also has some printfs to
 show what is happening. When the overridden allocator is called
 without the runtime callback set, it prints std alloc and when it's
 called with the callback set, it prints geos alloc.

 Here's a little program that exercises it:

 #include stdio.h
 #include /usr/local/include/geos_c.h
 #include stdlib.h

 int main() {
  printf(main\n);

  GEOSCoordSequence* cs;
  GEOSGeometry* g;
  int r;

  printf(initgeos\n);
  initGEOSMemory(malloc, free);
  printf(coordseqcreate\n);
  cs = GEOSCoordSeq_create(1, 2);
  printf(coordseqsetx\n);
  r = GEOSCoordSeq_setX(cs, 0, 1.0);
  printf(coordseqsety\n);
  r = GEOSCoordSeq_setY(cs, 0, 1.0);
  printf(createpoint\n);
  g = GEOSGeom_createPoint(cs);

  return 0;

 }


 And here is the output:

 Heron:tmp pramsey$ ./a.out
 std alloc!
 std alloc!
 main
 initgeos
 coordseqcreate
 geos alloc!
 geos alloc!
 geos alloc!
 coordseqsetx
 coordseqsety
 createpoint
 geos alloc!

 So, something in C++ is doing a couple allocations before we can slip
 in and get our other function in place. But we can get all the big
 stuff caught no problem.

 P.
 ___
 geos-devel mailing list
 geos-devel@lists.osgeo.org
 http://lists.osgeo.org/mailman/listinfo/geos-devel


 ___
 geos-devel mailing list
 geos-devel@lists.osgeo.org
 http://lists.osgeo.org/mailman/listinfo/geos-devel

___
geos-devel mailing list
geos-devel@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/geos-devel

Re: [geos-devel] An Immodest Proposal

[Paul Ramsey]

Help!

I've attached the modified geos_c.cpp and geos_c.h.in files for geos
for your perusal. The symptom is this:

- Modified containsPrepared to use initGEOSMemory
- Run an ST_Contains(g,g,i) prepared example
- Memory is both allocated and deallocated through the over-ridden
functions just fine
- UNTIL
- The code gets to the point where it has to deallocate a prepared
geometry and runs GEOSPreparedGeom_destroy()
- At that point, the deallocation does *not* enter the over-ridden
new/delete operators, it appears to use default operators, and
everything goes boom

So,

- the over-ride of new/delete seems to work fine inside the main geos
library, things get both allocated and freed using palloc and pfree
- but the delete inside the extern C {} block where
GEOSPreparedGeom_destroy does *not* appear to be over-ridden by our
cunning trick

Anyone have the secret sauce?

P.

On Tue, Sep 30, 2008 at 10:36 AM, Paul Ramsey [EMAIL PROTECTED] wrote:
 Time to try this in real life with PostgreSQL and palloc/pfree.  I'm
 going to create GEOS and PostGIS experimental branches, and commit my
 work there, so that folks with other platforms can try it out.

 On Tue, Sep 30, 2008 at 9:55 AM, Chris Hodgson [EMAIL PROTECTED] wrote:
 Very nice Paul.

 Those two std allocs are a bit scary... I had a feeling we might have some
 of those, and they seem worse than I had initially thought. The problem is
 that either they are attempted to be freed, using the provided free function
 - and who knows what error that could bring - or they are not, and we're
 leaking. If they are only created once at load of the DLL and never actually
 freed, but simply go away when the DLL is unloaded, then I guess we're ok.
 But it seems likely to me that the C++ runtime would be trying to free them
 using your own provided free function... can we do a test where we provide
 pfree and see what it does with that? ie. if you try to pfree() a regularly
 malloc()'d segment of memory, does it actually free it without raising any
 errors?

 It would be really nice to know what those things that are getting created
 earlier are, too...
 but I guess it all just comes down to, does it work in postgres and not
 leak?

 Chris

 Paul Ramsey wrote:

 It turns out it is a quite small modification, and works pretty well:

 http://trac.osgeo.org/geos/attachment/ticket/208/memdiff.patch

 My patch adds the ability to override, and also has some printfs to
 show what is happening. When the overridden allocator is called
 without the runtime callback set, it prints std alloc and when it's
 called with the callback set, it prints geos alloc.

 Here's a little program that exercises it:

 #include stdio.h
 #include /usr/local/include/geos_c.h
 #include stdlib.h

 int main() {
  printf(main\n);

  GEOSCoordSequence* cs;
  GEOSGeometry* g;
  int r;

  printf(initgeos\n);
  initGEOSMemory(malloc, free);
  printf(coordseqcreate\n);
  cs = GEOSCoordSeq_create(1, 2);
  printf(coordseqsetx\n);
  r = GEOSCoordSeq_setX(cs, 0, 1.0);
  printf(coordseqsety\n);
  r = GEOSCoordSeq_setY(cs, 0, 1.0);
  printf(createpoint\n);
  g = GEOSGeom_createPoint(cs);

  return 0;

 }


 And here is the output:

 Heron:tmp pramsey$ ./a.out
 std alloc!
 std alloc!
 main
 initgeos
 coordseqcreate
 geos alloc!
 geos alloc!
 geos alloc!
 coordseqsetx
 coordseqsety
 createpoint
 geos alloc!

 So, something in C++ is doing a couple allocations before we can slip
 in and get our other function in place. But we can get all the big
 stuff caught no problem.

 P.
 ___
 geos-devel mailing list
 geos-devel@lists.osgeo.org
 http://lists.osgeo.org/mailman/listinfo/geos-devel


 ___
 geos-devel mailing list
 geos-devel@lists.osgeo.org
 http://lists.osgeo.org/mailman/listinfo/geos-devel


___
geos-devel mailing list
geos-devel@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/geos-devel

Re: [geos-devel] An Immodest Proposal

[Paul Ramsey]

It turns out it is a quite small modification, and works pretty well:

http://trac.osgeo.org/geos/attachment/ticket/208/memdiff.patch

My patch adds the ability to override, and also has some printfs to
show what is happening. When the overridden allocator is called
without the runtime callback set, it prints std alloc and when it's
called with the callback set, it prints geos alloc.

Here's a little program that exercises it:

#include stdio.h
#include /usr/local/include/geos_c.h
#include stdlib.h

int main() {
  printf(main\n);

  GEOSCoordSequence* cs;
  GEOSGeometry* g;
  int r;

  printf(initgeos\n);
  initGEOSMemory(malloc, free);
  printf(coordseqcreate\n);
  cs = GEOSCoordSeq_create(1, 2);
  printf(coordseqsetx\n);
  r = GEOSCoordSeq_setX(cs, 0, 1.0);
  printf(coordseqsety\n);
  r = GEOSCoordSeq_setY(cs, 0, 1.0);
  printf(createpoint\n);
  g = GEOSGeom_createPoint(cs);

  return 0;

}


And here is the output:

Heron:tmp pramsey$ ./a.out
std alloc!
std alloc!
main
initgeos
coordseqcreate
geos alloc!
geos alloc!
geos alloc!
coordseqsetx
coordseqsety
createpoint
geos alloc!

So, something in C++ is doing a couple allocations before we can slip
in and get our other function in place. But we can get all the big
stuff caught no problem.

P.
___
geos-devel mailing list
geos-devel@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/geos-devel

Re: [geos-devel] An Immodest Proposal

[Frank Warmerdam]

On Tue, Sep 30, 2008 at 6:05 AM, Paul Ramsey [EMAIL PROTECTED] wrote:
 It turns out it is a quite small modification, and works pretty well:
...
  printf(initgeos\n);
  initGEOSMemory(malloc, free);
...
 So, something in C++ is doing a couple allocations before we can slip
 in and get our other function in place. But we can get all the big
 stuff caught no problem.

Paul,

Your approach looks fine to me.  In particular i'm pleased to see
the function to install the custom allocator is not part of the regular
init function and so does not change the old api or abi in an
incompatible way.

The initialization time allocations are a bit unfortunate.  In
fact for reasons like this and others I hate having objects
with dynamic components created during the program
startup (dll loading) process.

Best regards,
-- 
---+--
I set the clouds in motion - turn up   | Frank Warmerdam, [EMAIL PROTECTED]
light and sound - activate the windows | http://pobox.com/~warmerdam
and watch the world go round - Rush| Geospatial Programmer for Rent
___
geos-devel mailing list
geos-devel@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/geos-devel