Opened 11 years ago

Last modified 5 years ago

#863 new enhancement

set_env/unset_env should release memory

Reported by: rblazek Owned by: grass-dev@…
Priority: normal Milestone: 6.5.0
Component: LibGIS Version: svn-trunk
Keywords: Cc: rblazek
CPU: Unspecified Platform: All

Description

set_env/unset_env should release memory allocated for previously set value if a name is reused/deleted.

Change History (7)

comment:1 Changed 11 years ago by rblazek

Type: defectenhancement

comment:2 in reply to:  description Changed 11 years ago by glynn

Replying to rblazek:

set_env/unset_env should release memory allocated for previously set value if a name is reused/deleted.

That would require G__getenv() to duplicate the value. Currently, it just returns the pointer. As the caller may hold onto the pointer indefinitely, the library cannot free it. Duplicating the value would require the caller to free it.

I'm not sure this is worth it. Do any GRASS modules call G_setenv() repeatedly?

comment:3 Changed 11 years ago by rblazek

GRASS modules probably don't set variables repeatedly, but in GDAL, for example, if working with GRASS rasters from more mapsets/locations simultaneously, needs to reset mapset/location if masking is not disabled. It is using G_set_window() which checks for MASK.

Is there any other reasonable way how to reset environment when reading rasters from more mapsets/locations?

comment:4 in reply to:  3 ; Changed 11 years ago by glynn

Replying to rblazek:

GRASS modules probably don't set variables repeatedly, but in GDAL, for example, if working with GRASS rasters from more mapsets/locations simultaneously, needs to reset mapset/location if masking is not disabled. It is using G_set_window() which checks for MASK.

Is there any other reasonable way how to reset environment when reading rasters from more mapsets/locations?

If you need to switch between two distinct environments (e.g. source and destination), there's G__create_alt_env() and G__switch_env().

If you need more than two environments, you're out of luck.

Changing this would mean that many library functions calling G_getenv() etc would need to free the memory when it's no longer needed, which means that they would need to either track when it's no longer needed or push this burden onto the caller.

In large software packages, tracking dynamically-allocated memory can be a major issue. Eliminating this issue is one of the main "selling points" of high-level languages with built-in garbage collection.

The GRASS philosophy of not worrying about memory leaks which will only occur a few times per process (for a normal GRASS module) saves us a lot of trouble.

comment:5 in reply to:  4 ; Changed 11 years ago by rblazek

Replying to glynn:

Thanks for explanation. Two environments are not sufficient because GDAL/QGIS can open any number of maps from different locations but the idea of G__switch_env() sounds good. Do you think that a patch adding possibility to create more environments and switching between them using environment id/index would be accepted? I am thinking about an array of pointers to a structure keeping ENV *env and mapset_name (maybe more?).

comment:6 in reply to:  5 Changed 11 years ago by glynn

Replying to rblazek:

Do you think that a patch adding possibility to create more environments and switching between them using environment id/index would be accepted?

That seems reasonable.

comment:7 Changed 5 years ago by neteler

See also #864

Note: See TracTickets for help on using tickets.