Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

r.neighbors: fix integer overflow #2853

Merged
merged 3 commits into from
Feb 24, 2023
Merged

Conversation

metzm
Copy link
Contributor

@metzm metzm commented Feb 23, 2023

Fix integer overflow for regions with more than INT_MAX cells.

A very similar cast to size_t is already in r.series, but maybe other modules that were recently parallelized also need some attention.

@metzm metzm added bug Something isn't working backport_needed raster Related to raster data processing C Related code is in C labels Feb 23, 2023
@metzm metzm added this to the 8.2.2 milestone Feb 23, 2023
@metzm metzm requested a review from nilason February 23, 2023 14:16
Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

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

Looks good to me!

[...] but maybe other modules that were recently parallelized also need some attention.

Here is a list of modules with OpenMP support:

  • r.mfilter [new]
  • r.neighbors [new]
  • r.patch [new]
  • r.proj
  • r.resamp.filter [new]
  • r.resamp.interp [new]
  • r.series [new]
  • r.series.accumulate
  • r.sim
  • r.slope.aspect [new]
  • r.sun
  • r.univar [new]
  • v.surf.rst

@metzm
Copy link
Contributor Author

metzm commented Feb 23, 2023

Evaluation of the other modules could be worth an issue?

For r.neighbors and possibly for other modified modules, I don't see a speed improvement by a large cache for output rows. The output rows need to be written one by one and a large cache for these output rows only needs more RAM without any benefits. This cache for output rows could therefore always be reduced to the needed minimum = number of threads.

@nilason
Copy link
Contributor

nilason commented Feb 23, 2023

Evaluation of the other modules could be worth an issue?

Wouldn't hurt, at least for this "region cell no > INT_MAX" situation.

For r.neighbors and possibly for other modified modules, I don't see a speed improvement by a large cache for output rows. The output rows need to be written one by one and a large cache for these output rows only needs more RAM without any benefits. This cache for output rows could therefore always be reduced to the needed minimum = number of threads.

Benchmark suggests otherwise: https://grass.osgeo.org/grass82/manuals/r.neighbors.html

@neteler neteler modified the milestones: 8.2.2, 8.3.0 Feb 24, 2023
@neteler
Copy link
Member

neteler commented Feb 24, 2023

(will be backported in #2856)

@metzm
Copy link
Contributor Author

metzm commented Feb 24, 2023

For r.neighbors and possibly for other modified modules, I don't see a speed improvement by a large cache for output rows. The output rows need to be written one by one and a large cache for these output rows only needs more RAM without any benefits. This cache for output rows could therefore always be reduced to the needed minimum = number of threads.

Benchmark suggests otherwise: https://grass.osgeo.org/grass82/manuals/r.neighbors.html

No obvious improvement from 5 MB onwards? Looks like there is no benefit by caching large parts of the output.

@nilason
Copy link
Contributor

nilason commented Feb 24, 2023

For r.neighbors and possibly for other modified modules, I don't see a speed improvement by a large cache for output rows. The output rows need to be written one by one and a large cache for these output rows only needs more RAM without any benefits. This cache for output rows could therefore always be reduced to the needed minimum = number of threads.

Benchmark suggests otherwise: https://grass.osgeo.org/grass82/manuals/r.neighbors.html

No obvious improvement from 5 MB onwards? Looks like there is no benefit by caching large parts of the output.

Now I see what you mean, yes, you're quite right.

@wenzeslaus
Copy link
Member

...This cache for output rows could therefore always be reduced to the needed minimum = number of threads. ...
No obvious improvement from 5 MB onwards?

Note that in the code memory=0 translates to minimal memory required. 5MB for the data in the benchmark means a small cache (judging from difference between 0MB and 5MB).

@metzm metzm merged commit 2ad3235 into OSGeo:main Feb 24, 2023
@metzm metzm deleted the r_neighbors_integer_overflow branch February 24, 2023 15:25
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
…SGeo#2853)

* r.neighbors, r.resamp.filter, r.resamp.interp: fix integer overflow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working C Related code is in C raster Related to raster data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants