Skip to content

Comments

r.terraflow: remove print "Free Memory" to stats file#1306

Merged
nilason merged 1 commit intoOSGeo:masterfrom
nilason:rm-r-terraflow-freemem-check
Feb 5, 2021
Merged

r.terraflow: remove print "Free Memory" to stats file#1306
nilason merged 1 commit intoOSGeo:masterfrom
nilason:rm-r-terraflow-freemem-check

Conversation

@nilason
Copy link
Contributor

@nilason nilason commented Feb 4, 2021

Generating a statistics file with r.terraflow (e.g. r.terraflow stats=stats.txt [...]) the first line in the stats file (is supposed to) state the amount of free memory:

head stats.txt 
Free Memory=-1
[0.0] Wed Jan 27 10:31:51 2021
Command Line: 
r.terraflow elevation=elev_srtm_30m@PERMANENT filled=filled_rast stats=stats.txt 
input elevation grid: elev_srtm_30m
output (filled_rast) elevations grid: filled_rast
[0.0] MFD flow direction
[0.0] D8CUT=999999986991104.000000
[0.0] Memory size: 300.00M (314572800) bytes
region size = 219.73K (225000) elts (450 rows x 500 cols)

with the value "-1" given if it failed to calculate the size.

Current implementation is based on getrlimit(), see: r.terraflow/stats.cpp and the value of rlimit.rlim_cur.

On a 64 bit Mac and probably Linux too (#712 (comment)) rlimit.rlim_cur == RLIM_INFINITY and the calculation fails. I may add that this code is already excluded from __MINGW32__ builds.

Part of the calculation is based on the deprecated sbrk(), originally reported with #712.

There is a lot of things to address with present code and all it does is printing one line in the stat file.

With this PR I propose to remove this part of the code, as I couldn't come up with a platform independent way to calculate "free memory" (also a vague concept considering virtual memory).

Fixes #712

Copy link
Contributor

@metzm metzm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes make sense, please merge.

r.terraflow has been designed for past conditions, was never really portable, and is by now outdated.

@nilason
Copy link
Contributor Author

nilason commented Feb 5, 2021

The changes make sense, please merge.

r.terraflow has been designed for past conditions, was never really portable, and is by now outdated.

Thanks, merging!

@nilason nilason merged commit 9daba65 into OSGeo:master Feb 5, 2021
@nilason nilason deleted the rm-r-terraflow-freemem-check branch February 5, 2021 07:17
@lbartoletti
Copy link
Contributor

lbartoletti commented Feb 7, 2021

Thanks @nilason !
Tested on FreeBSD aarch64 (arm64) and it built fine.

marisn pushed a commit to marisn/grass that referenced this pull request Mar 22, 2021
@neteler neteler added this to the 8.0.0 milestone Dec 9, 2021
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Use of deprecated sbrk

4 participants