Remove memory leaks to make trunk production ready

[mloskot]

There has been number of memory leaks recently observed and reported by users and developers.

This is a all-in-one task devoted to memory leaks cleanup in GEOS core, C API layer and tests:

• Goal: make GEOS trunk production-ready
• In particular, remove all memory leaks
• Not just prepared geometry, also check other use cases exercised by PostGIS (standard intersection, union, intersects, contains, within, etc.)

Everyone interested, please, report observed memory leaks as comments to this ticket. Don't forget to attach files with test geometries, test queries, test programs, all steps required to reproduce particular memory leak(s).

[mloskot]

File with EWKB of test polygon from Paul Ramsey. Test it for intersects, contains, within, touches against itself but using distinct Geometry objects. For bonus marks, move one copy of geometry 200 units to the west and try again. Try versions of prepared geometry of the same tests.

[mloskot]

Simple program testing geos::geom::Geometry objects and Geometry predicates on Paul's polygon loaded from EWKB stream in file pramsey-memory-leaks-test-case-polygon.txt. If built using Visual C++ and with Visual Leak Detector available, define preprocessor macro GEOS_DEBUG_MSVC_USE_VLD to enable VLD engine.

[mloskot]

I've built the test_leaks.cpp using Visual C++ 2005 (8.0) with  Visual Leak Detector 1.9d enabled and run number of times under Windows XP.

I can report that no memory leaks have been reported.

Next step is to run similar tests against PreparedGeometry objects and compare results.

[mloskot]

I've just tested test_leaks.cpp program with GCC 4.1.2 and  Valgrind 3.2.1 under Linux (Ubuntu 7.04):

$ valgrind --tool=memcheck --leak-check=full ./test_leaks

No memory leaks have been detected:

==24590== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 19 from 1)
==24590== malloc/free: in use at exit: 0 bytes in 0 blocks.
==24590== malloc/free: 341,325 allocs, 341,325 frees, 8,689,620 bytes allocated.
==24590== For counts of detected errors, rerun with: -v
==24590== All heap blocks were freed -- no leaks are possible.

[mloskot]

Simple program testing geos::geom::prep::PreparedGeometry objects and spatial predicates on Paul's polygon loaded from EWKB stream in file pramsey-memory-leaks-test-case-polygon.txt. If built using Visual C++ and with Visual Leak Detector available, define preprocessor macro GEOS_DEBUG_MSVC_USE_VLD to enable VLD engine.

[mloskot]

I have run test program test_leaks_geom_pred.cpp using two environments:

• Windows XP + Visual C++ 2005 + Visual Leak Detector 1.9g
• Linux + GCC 4.1.2 + Valgrind 3.2.1

On both platforms, execution of two predicates cause memory leaks:

• geom::prep::PreparedGeometry::contains()
• geom::prep::PreparedGeometry::covers()

Other predicates do not cause memory leaks, at least I've not observed any myself.

For both predicates, summary reports look very:

• by VLD

  Visual Leak Detector detected 136 memory leaks.
  

• by Valgrind

  ==4588== LEAK SUMMARY:
  ==4588==    definitely lost: 24 bytes in 2 blocks.
  ==4588==    indirectly lost: 4,208 bytes in 134 blocks.
  ==4588==      possibly lost: 0 bytes in 0 blocks.
  ==4588==    still reachable: 0 bytes in 0 blocks.
  ==4588==         suppressed: 0 bytes in 0 blocks.
  

In all leaking predicates and on all environments, memory leaks from indexing engine, specifically in operations by MonotoneChainBuilder? and MonotoneChain?.

[mloskot]

It looks like source of memory leaking during execution of PreparedGeometry predicates like contains and covers was known and identified a while ago.

The method MCIndexSegmentSetMutualIntersector::addToIndex calls MonotoneChainBuilder::getChains which in turns dynamically allocates memory for std::vector object segChains. Elements of segChains vector are used to compile spatial index, but it's never deallocated.

As BWJ (Ben?) has noticed and put appropriate comment, there is a kind of design problem. The segChains vector can not be deallocated immediately after the index is built. The index stores pointers to elements of segChains vector and uses these elements later. So, lifetime of segChains should be at least as long as lifetime of the index.

However, this lifetime requirement seems to be not easy to implement. It would mean that MCIndexSegmentSetMutualIntersector mananges private pool of segChains vectors. Next, during every execution of MCIndexSegmentSetMutualIntersector::addToIndex, new segChains is allocated and it should be added to this pool.

Then, it will be possible to deallocate all segChains objects from destructor of MCIndexSegmentSetMutualIntersector class. In other words, all segChains objects created by addToIndex should be cached and destroyed all at once in the destructor of MCIndexSegmentSetMutualIntersector.

Even if the general solution sounds easy, I'm not sure if it wouldn't break anything in other places, for other predicates or types. There seem to be quite complex relations between various collections internally created and used as well as their flows are hard to track.

I'd be thankful if anyone knowledgeable with these elements could comment this analysis. Otherwise, I think I've stuck.

[pramsey]

Is it possible to move this one part of the code base over to smart / reference counting pointers?

[mloskot]

Paul,

I'm still trying to figure out how to deal with this. Considering all input we've collected in the discussion on the list and that I'm very short in time before the December, I'm afraid I have really no idea how to get it done :-(

[mloskot]

Just for records, Paul has submitted memory leak fix for prepared geometry, from Hartmut Kaiser (r2196)

Thanks guys!