Fix quadtree spatial join OOMs on large numbers of input polygons #1381

trxcllnt · 2024-05-07T04:44:32Z

Description

Followup to #1346.

Fixes some typos/omissions in types and CMake.
Adds a new test that OOMs when quadtree_point_in_polygon is passed too many input polygons.
Fixes quadtree spatial join to handle overflow while counting and more conservatively allocate output buffers.

Fixes #890.

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

…am operator

…tributed instead of distributing along a diagonal

cpp/include/cuspatial/detail/join/quadtree_point_in_polygon.cuh

cpp/include/cuspatial_test/geometry_generator.cuh

cpp/tests/join/quadtree_point_in_polygon_test_oom.cu

cpp/include/cuspatial/detail/range/multilinestring_range.cuh

cpp/benchmarks/CMakeLists.txt

cpp/include/cuspatial/geometry/box.hpp

cpp/include/cuspatial_test/vector_factories.cuh

harrism

Nice improvement. I think adding some of the details from your review comments as actual code comments might be valuable. But I approve.

cpp/include/cuspatial/detail/join/quadtree_point_in_polygon.cuh

cpp/include/cuspatial_test/geometry_generator.cuh

…into fix/quadtree-spatial-join-oom

…its and allocating the exact amount of memory for the results

harrism

Thanks for fixing! One small request.

cpp/include/cuspatial/detail/join/quadtree_point_in_polygon.cuh

trxcllnt · 2024-05-21T18:33:25Z

Not sure what's going on with the conda-notebook-tests job. Can we admin merge this?

isVoid

One small code suggestion below. Otherwise lgtm!

isVoid · 2024-05-22T02:03:55Z

cpp/include/cuspatial/detail/join/quadtree_point_in_polygon.cuh

+  auto count_in_chunks             = [&](auto const& func) {
+    std::uint64_t memo{};
+    for (std::uint64_t offset{0}; offset < num_total_points; offset += max_points_to_test) {
+      memo += func(memo, offset, std::min(max_points_to_test, num_total_points - offset));
+    }
+    return memo;
+  };


This looks like a count + reduce to me.. I'm ok with using a for loop in the small scope here. One small suggestion: renaming func to count_func could increase readability.

Yeah, this is a special form of std::reduce. The difference is mainly in how it passes min(max, num_left_to_process) as the size argument, which I thought would be more confusing to read if I implemented it as an iterator of [max, max, .., leftover].

trxcllnt · 2024-05-24T13:36:44Z

/merge

trxcllnt added 6 commits May 6, 2024 21:32

Define cxx and cuda standards for benchmarks

3128ebc

add missing __host__ __device__ to satisfy clangd

8d2cf21

fix box docstring typo and include second template parameter in ostre…

574ab3c

…am operator

add to_host() to multipoint_array for debugging

1e30987

use the golden ratio as the seed so random points are more evenly dis…

16cb2e0

…tributed instead of distributing along a diagonal

add test to OOM quadtree_point_in_polygon on 24GB cards

4fa1cae

trxcllnt requested review from a team as code owners May 7, 2024 04:44

trxcllnt requested review from harrism and isVoid May 7, 2024 04:44

github-actions bot added cmake Related to CMake code or build configuration libcuspatial Relates to the cuSpatial C++ library labels May 7, 2024

trxcllnt added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change and removed cmake Related to CMake code or build configuration libcuspatial Relates to the cuSpatial C++ library labels May 7, 2024

fix lint

781fc7d

github-actions bot added cmake Related to CMake code or build configuration libcuspatial Relates to the cuSpatial C++ library labels May 7, 2024

fix new quadtree spatial join OOM test

57354cb