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

Improve performance of quadtree point-to-polyline join #362

Conversation

trxcllnt
Copy link
Contributor

Rewrites quadtree_point_to_nearest_polyline via thrust in the style of quadtree_point_in_polygon to get a 5x speed boost.

Benchmarked locally with NYC taxi 169M float32 points (RMM pool mode enabled):

quadtree_point_to_nearest_polyline (before)
1min ± 10.4 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

quadtree_point_to_nearest_polyline (after)
9.49 s ± 15 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

Timings for every routine in the benchmark after these changes:

quadtree_on_points
66.2 ms ± 64.3 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

polygon_bounding_boxes
322 µs ± 375 ns per loop (mean ± std. dev. of 7 runs, 1000 loops each)

join_quadtree_and_bounding_boxes
11.4 ms ± 8.19 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

quadtree_point_in_polygon
5.48 s ± 54.4 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

polyline_bounding_boxes
258 µs ± 2.37 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

join_quadtree_and_bounding_boxes
11.5 ms ± 17.5 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

quadtree_point_to_nearest_polyline
9.49 s ± 15 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

… of quadtree_point_in_polygon to get a 5x perf boost
@trxcllnt trxcllnt requested a review from a team as a code owner February 25, 2021 06:48
@trxcllnt trxcllnt added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 25, 2021
@thomcom
Copy link
Contributor

thomcom commented Mar 26, 2021

Mind iterating on this one's passing status when you get the chance @trxcllnt ?

@trxcllnt
Copy link
Contributor Author

trxcllnt commented Mar 26, 2021

@thomcom yeah for sure. We might also want to wait on this PR because, while it's faster, it requires allocating much more temporary memory than the current implementation. I think it's possible to apply some of the techniques in the hausdorff iterator to eliminate that extra memory, but I'd need to explore it in depth.

edit: The new implementation that doesn't use as much intermediate memory done in e79caa9 🎉

@github-actions github-actions bot added the libcuspatial Relates to the cuSpatial C++ library label Mar 31, 2021
@trxcllnt trxcllnt requested a review from harrism April 7, 2021 21:33
@trxcllnt
Copy link
Contributor Author

trxcllnt commented Apr 7, 2021

@harrism I re-requested your review for the new version of quadtree_point_to_nearest_polyline.cu

@trxcllnt trxcllnt added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Apr 8, 2021
@trxcllnt trxcllnt changed the base branch from branch-0.19 to branch-0.20 April 8, 2021 17:02
@trxcllnt trxcllnt added the 2 - In Progress Currenty a work in progress label Apr 8, 2021
@github-actions github-actions bot added the Python Related to Python code label Apr 14, 2021
@trxcllnt trxcllnt removed 2 - In Progress Currenty a work in progress 5 - DO NOT MERGE Hold off on merging; see PR for details Python Related to Python code cmake Related to CMake code or build configuration java labels Apr 15, 2021
@trxcllnt
Copy link
Contributor Author

rerun tests

@harrism
Copy link
Member

harrism commented May 4, 2021

@harrism I re-requested your review for the new version of quadtree_point_to_nearest_polyline.cu

I totally missed this. I'm sorry.

Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Looks great. Performance looks great. Several comments, but only one absolutely required change (missing stream sync).

cpp/src/join/quadtree_point_in_polygon.cu Show resolved Hide resolved
cpp/src/join/quadtree_point_in_polygon.cu Outdated Show resolved Hide resolved
cpp/src/utility/point_to_nearest_polyline.cuh Outdated Show resolved Hide resolved
cpp/src/join/quadtree_point_to_nearest_polyline.cu Outdated Show resolved Hide resolved
cpp/src/join/quadtree_point_to_nearest_polyline.cu Outdated Show resolved Hide resolved
cpp/src/join/quadtree_point_to_nearest_polyline.cu Outdated Show resolved Hide resolved
cpp/src/join/quadtree_point_to_nearest_polyline.cu Outdated Show resolved Hide resolved
cpp/src/join/quadtree_point_to_nearest_polyline.cu Outdated Show resolved Hide resolved
@trxcllnt trxcllnt requested a review from a team as a code owner May 6, 2021 04:13
@github-actions github-actions bot added the Python Related to Python code label May 6, 2021
@trxcllnt
Copy link
Contributor Author

rerun tests

@trxcllnt trxcllnt requested a review from harrism May 12, 2021 05:35
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Just a couple more fixes.

cpp/src/utility/point_to_nearest_polyline.cuh Outdated Show resolved Hide resolved
cpp/src/join/quadtree_point_to_nearest_polyline.cu Outdated Show resolved Hide resolved
@trxcllnt
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 6eb044e into rapidsai:branch-21.06 May 20, 2021
@cwharris
Copy link
Contributor

cwharris commented May 20, 2021

I had questions regarding how the tests were updated due to the new sortedness of the outputs, and @trxcllnt answered those offline. basically, user's weren't guaranteed any sortedness prior to this change, and now we are sorting the output ascending, so no breaking change.

lgtm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcuspatial Relates to the cuSpatial C++ library non-breaking Non-breaking change Python Related to Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants