[GRASS-dev] Re: [GRASS GIS] #837: Memory leaks in r.example

[GRASS GIS]

#837: Memory leaks in r.example
----------------------+-----------------------------------------------------
  Reporter:  sprice   |       Owner:  grass-dev at lists.osgeo.org
      Type:  defect   |      Status:  reopened                 
  Priority:  normal   |   Milestone:  6.4.0                    
 Component:  default  |     Version:  svn-releasebranch64      
Resolution:           |    Keywords:                           
  Platform:  MacOSX   |         Cpu:  OSX/Intel                
----------------------+-----------------------------------------------------
Comment (by glynn):

 Replying to [comment:3 sprice]:
 > Judging by the 1616 leaked nodes in get_map_row() this is on a per row
 basis. But I've never heard of a well written program containing leaks
 anyway.

 No program leaks, insofar as all of its memory is returned to the system
 upon termination. It's just a question of how much memory it uses while
 it's running. Most programs could have their memory consumption reduced;
 it's a question of whether it's worth the effort and (more importantly)
 added complexity.

 > Here's your fix for the first leak:
 > Change line 58 in lib/gis/gisinit.c from "G_location_path();" to
 "G_free(G_location_path());"

 This is harmless (it's a fixed amount of memory for the lifetime of the
 process), but it's also trivial to fix.

 > The second leak:
 > Add "G_free(dummy);" at line 1003 in file lib/gis/get_row.c

 This is a '''real''' leak, but I'm not sure that it can be fixed this way,
 as the return value from G_find_* may not be owned by the caller. The
 return value may be a mapset from the current mapset list, in which case
 freeing it will break stuff.

 Historically, the view has been that memory consumed by G_find_* or
 G_open_* isn't an issue; it just means that a process consumes a certain
 amount of memory per map. But the null value handling was hacked on later;
 the fact that it opens and closes the null value bitmap every few rows was
 always known to be inefficient, but I don't think anyone spotted the leak
 before.

 Possible solutions:

 1. Change the null bitmap handling to keep the file open.

 The reason why this wasn't done originally is that it doubles the number
 of open files, which can cause e.g. r.series to run into OS limits on the
 number of open files. It also doubles the number of required fileinfo
 slots, but this is less of a problem now that the fileinfo array is
 dynamically sized.

 2. Make G_find_* return unique allocations, so that they can be freed.
 This may increase fixed memory consumption slightly for some modules, as
 nothing currently frees the return value. But it will reduce it in the
 long run (and in the short run for raster modules by fixing this leak),
 and the worst case with it fixed will still be a lot better than the
 current worst case.

 3. Make open_null_read free the return value. I '''think''' that it will
 always be unique in this case, as a mapset is always being passed in.
 AFAICT, the return value is only shared if the name is unqualified and the
 mapset is NULL or the empty string, indicating that the map should be
 looked up using the mapset search path. A fully-qualified name or an
 unqualified name with an explicit mapset should always return a unique
 value. But if I've overlooked something, freeing the return value will
 cause no end of problems.

 If this was the only issue, my inclination would be for !#2, as we're
 trying to get 6.4 released. In that context, !#1 is too disruptive and !#3
 is a hack which, if it's wrong, may not get discovered until it's too
 late.

 But ...

 > The third, fourth, & last leaks (in lib/gis/opencell.c) are a bit more
 difficult to take care of all permutations:

 [snip]

 This is a large part of the reason why we just resign ourselves to each
 map requiring a certain amount of memory. And I'd leave it at that, if it
 wasn't for the null bitmap handling calling this function repeatedly.

 This specific fix can't be applied without first making G_find_* always
 return unique allocations.

 The fix would be simpler if G_open_cell_old() called G_fatal_error() on
 errors. This would make life simpler for modules, which currently need to
 check the return value. Off the top of my head, I don't know of a single
 case where the caller actually tries to recover from G_open_cell_old()
 failing. Doing so would also be a benefit to a small number of modules
 which forget to check for errors.

 An intermediate solution would be to free the memory under normal
 circumstances and not bother about the leak if G_open_cell_old() fails.
 It's 99.99% likely that the program is going to terminate immediately in
 that situation.

 > The next leak in lib/gis/gdal.c:

 This isn't a problem. Each map which is opened requires some memory. There
 are all sorts of ways in which that memory consumption could be reduced;
 some of them are worth the effort, some of them aren't. Again, this
 specific fix can't be applied without first making G_find_* always return
 unique allocations. If that is done, this would be worthwhile.

 Right now, the only thing that I'm certain of is that the null bitmap
 handling has to change for 7.0, but it's probably too disruptive for 6.x.

-- 
Ticket URL: <http://trac.osgeo.org/grass/ticket/837#comment:4>
GRASS GIS <http://grass.osgeo.org>